Mike, Thanks for the comments, I updated the patch, please help to review the new patch.
Thanks, Eric -----Original Message----- From: Kinney, Michael D Sent: Thursday, August 24, 2017 5:51 AM To: Dong, Eric <[email protected]>; [email protected]; Kinney, Michael D <[email protected]> Cc: Ni, Ruiyu <[email protected]> Subject: RE: [Patch] UefiCpuPkg/MpLib: fix potential overflow issue. Hi Eric, With this patch GetPerformanceCounterProperties() is called twice. I think you can use TimestampCounterFreq in the else clause. Also, the comment blocks are no longer correct. The original comment block goes with the else clause, and you need a new comment block for the if statement that describes the check for an overflow. Mike > -----Original Message----- > From: Dong, Eric > Sent: Tuesday, August 22, 2017 10:30 PM > To: [email protected] > Cc: Kinney, Michael D <[email protected]>; Ni, Ruiyu > <[email protected]> > Subject: [Patch] UefiCpuPkg/MpLib: fix potential overflow issue. > > Current calculate timeout logic may have overflow if the input timeout > value too large. This patch fix this potential overflow issue. > > Cc: Michael Kinney <[email protected]> > Cc: Ruiyu Ni <[email protected]> > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Eric Dong <[email protected]> > --- > UefiCpuPkg/Library/MpInitLib/MpLib.c | 30 > +++++++++++++++++++++++------- > 1 file changed, 23 insertions(+), 7 deletions(-) > > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c > b/UefiCpuPkg/Library/MpInitLib/MpLib.c > index ed1f55e..005dec4 100644 > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c > @@ -1001,6 +1001,9 @@ CalculateTimeout ( > OUT UINT64 *CurrentTime > ) > { > + UINT64 TimeoutInSeconds; > + UINT64 TimestampCounterFreq; > + > // > // Read the current value of the performance counter > // > @@ -1019,13 +1022,26 @@ CalculateTimeout ( > // in Hz. So multiply the return value with TimeoutInMicroseconds > and then divide > // it by 1,000,000, to get the number of ticks for the timeout > value. > // > - return DivU64x32 ( > - MultU64x64 ( > - GetPerformanceCounterProperties (NULL, NULL), > - TimeoutInMicroseconds > - ), > - 1000000 > - ); > + TimestampCounterFreq = GetPerformanceCounterProperties > (NULL, NULL); > + if (DivU64x64Remainder (MAX_UINT64, TimeoutInMicroseconds, > NULL) < TimestampCounterFreq) { > + // > + // Convert microseconds into seconds if direct > multiplication overflows > + // > + TimeoutInSeconds = DivU64x32 (TimeoutInMicroseconds, > 1000000); > + // > + // Assertion if the final tick count exceeds MAX_UINT64 > + // > + ASSERT (DivU64x64Remainder (MAX_UINT64, TimeoutInSeconds, > NULL) >= TimestampCounterFreq); > + return MultU64x64 (TimestampCounterFreq, > TimeoutInSeconds); > + } else { > + return DivU64x32 ( > + MultU64x64 ( > + GetPerformanceCounterProperties (NULL, NULL), Use TimestampCounterFreq instead. > + TimeoutInMicroseconds > + ), > + 1000000 > + ); > + } > } > > /** > -- > 2.7.0.windows.1 _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
