Zhichao: One generic comment, the commit message doesn't need to include V1, V2. It is just the change description.
Thanks Liming >-----Original Message----- >From: Dong, Eric >Sent: Wednesday, June 26, 2019 8:48 AM >To: Gao, Zhichao <zhichao....@intel.com>; devel@edk2.groups.io >Cc: Ni, Ray <ray...@intel.com>; Laszlo Ersek <ler...@redhat.com>; Gao, >Liming <liming....@intel.com> >Subject: RE: [PATCH V2] UefiCpuPkg/MpInitLib: MicrocodeDetect: Ensure >checked range is valid > >Hi Zhichao, > >Reviewed-by: Eric Dong <eric.d...@intel.com> > >It's better to add some comments in the code to explain the change which >make the code easy to be understood. > >Thanks, >Eric > >> -----Original Message----- >> From: Gao, Zhichao >> Sent: Tuesday, June 25, 2019 11:16 PM >> To: devel@edk2.groups.io >> Cc: Dong, Eric <eric.d...@intel.com>; Ni, Ray <ray...@intel.com>; Laszlo >> Ersek <ler...@redhat.com>; Gao, Liming <liming....@intel.com> >> Subject: [PATCH V2] UefiCpuPkg/MpInitLib: MicrocodeDetect: Ensure >> checked range is valid >> >> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1934 >> >> V1: >> Originally, the checksum part would done before verfiy the microcode data. >> Which meas the checksum would be done for a meaningless data. >> It would cause a incorrect TotalSize (the size of microcode data), then >> incorrect checksum and incorrect pointer increasing would happen. >> To fix this, move the checksum part 1 section in 'if (MicrocodeEntryPoint- >> >HeaderVersion == 0x1)' section for a valid microcode data. >> >> V2: >> 'if (MicrocodeEntryPoint->HeaderVersion == 0x1)' condition doesn't make >> sure the entry data is a valid microcode. So abandon it. Instead, make sure >> the checked data is in the microcode data range. Because the DataSize of >non >> microcde data may make (MicrocodeEntryPoint + TotalSize) larger than >> 0xffffffff. For PEI driver, UINTN is 32bit and the result is overflow and it >> may >> be a very small value. That means the checksum check would be done out of >> the microcode range. >> >> Cc: Eric Dong <eric.d...@intel.com> >> Cc: Ray Ni <ray...@intel.com> >> Cc: Laszlo Ersek <ler...@redhat.com> >> Cc: Liming Gao <liming....@intel.com> >> Signed-off-by: Zhichao Gao <zhichao....@intel.com> >> --- >> UefiCpuPkg/Library/MpInitLib/Microcode.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/UefiCpuPkg/Library/MpInitLib/Microcode.c >> b/UefiCpuPkg/Library/MpInitLib/Microcode.c >> index 4763dcfebe..6c0995cb0d 100644 >> --- a/UefiCpuPkg/Library/MpInitLib/Microcode.c >> +++ b/UefiCpuPkg/Library/MpInitLib/Microcode.c >> @@ -1,7 +1,7 @@ >> /** @file >> Implementation of loading microcode on processors. >> >> - Copyright (c) 2015 - 2018, Intel Corporation. All rights reserved.<BR> >> + Copyright (c) 2015 - 2019, Intel Corporation. All rights >> + reserved.<BR> >> SPDX-License-Identifier: BSD-2-Clause-Patent >> >> **/ >> @@ -170,6 +170,7 @@ MicrocodeDetect ( >> /// Check overflow and whether TotalSize is aligned with 4 bytes. >> /// >> if ( ((UINTN)MicrocodeEntryPoint + TotalSize) > MicrocodeEnd || >> + ((UINTN)MicrocodeEntryPoint + TotalSize) < (UINTN) >> + CpuMpData->MicrocodePatchAddress || >> (TotalSize & 0x3) != 0 >> ) { >> MicrocodeEntryPoint = (CPU_MICROCODE_HEADER *) (((UINTN) >> MicrocodeEntryPoint) + SIZE_1KB); >> -- >> 2.21.0.windows.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#42847): https://edk2.groups.io/g/devel/message/42847 Mute This Topic: https://groups.io/mt/32204622/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
