* [PATCH V2] UefiCpuPkg/MpInitLib: MicrocodeDetect: Ensure checked range is valid @ 2019-06-25 15:15 Gao, Zhichao 2019-06-26 0:48 ` Dong, Eric 0 siblings, 1 reply; 8+ messages in thread From: Gao, Zhichao @ 2019-06-25 15:15 UTC (permalink / raw) To: devel; +Cc: Eric Dong, Ray Ni, Laszlo Ersek, Liming Gao 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.dong@intel.com> Cc: Ray Ni <ray.ni@intel.com> Cc: Laszlo Ersek <lersek@redhat.com> Cc: Liming Gao <liming.gao@intel.com> Signed-off-by: Zhichao Gao <zhichao.gao@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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH V2] UefiCpuPkg/MpInitLib: MicrocodeDetect: Ensure checked range is valid 2019-06-25 15:15 [PATCH V2] UefiCpuPkg/MpInitLib: MicrocodeDetect: Ensure checked range is valid Gao, Zhichao @ 2019-06-26 0:48 ` Dong, Eric 2019-06-26 1:17 ` Gao, Zhichao ` (2 more replies) 0 siblings, 3 replies; 8+ messages in thread From: Dong, Eric @ 2019-06-26 0:48 UTC (permalink / raw) To: Gao, Zhichao, devel@edk2.groups.io; +Cc: Ni, Ray, Laszlo Ersek, Gao, Liming Hi Zhichao, Reviewed-by: Eric Dong <eric.dong@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.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Laszlo > Ersek <lersek@redhat.com>; Gao, Liming <liming.gao@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.dong@intel.com> > Cc: Ray Ni <ray.ni@intel.com> > Cc: Laszlo Ersek <lersek@redhat.com> > Cc: Liming Gao <liming.gao@intel.com> > Signed-off-by: Zhichao Gao <zhichao.gao@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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH V2] UefiCpuPkg/MpInitLib: MicrocodeDetect: Ensure checked range is valid 2019-06-26 0:48 ` Dong, Eric @ 2019-06-26 1:17 ` Gao, Zhichao 2019-06-26 1:35 ` Liming Gao 2019-06-26 1:57 ` Ni, Ray 2 siblings, 0 replies; 8+ messages in thread From: Gao, Zhichao @ 2019-06-26 1:17 UTC (permalink / raw) To: Dong, Eric, devel@edk2.groups.io; +Cc: Ni, Ray, Laszlo Ersek, Gao, Liming HI Eric, I think of the comments as blow: Check overflow and whether TotalSize is aligned with 4 bytes. ==> Check whether (MicrocodeEntryPoint + TotalSize) is in the microcode data range and whether TotalSize is aligned with 4 bytes. This is the first check of the microcode data and TotalSize may be an invalid value. So ensure the check range is within the microcode data range. And next check would judge whether the data is microcode or not. Thanks, Zhichao > -----Original Message----- > From: Dong, Eric > Sent: Wednesday, June 26, 2019 8:48 AM > To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io > Cc: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>; Gao, > Liming <liming.gao@intel.com> > Subject: RE: [PATCH V2] UefiCpuPkg/MpInitLib: MicrocodeDetect: Ensure > checked range is valid > > Hi Zhichao, > > Reviewed-by: Eric Dong <eric.dong@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.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; > > Laszlo Ersek <lersek@redhat.com>; Gao, Liming <liming.gao@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.dong@intel.com> > > Cc: Ray Ni <ray.ni@intel.com> > > Cc: Laszlo Ersek <lersek@redhat.com> > > Cc: Liming Gao <liming.gao@intel.com> > > Signed-off-by: Zhichao Gao <zhichao.gao@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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH V2] UefiCpuPkg/MpInitLib: MicrocodeDetect: Ensure checked range is valid 2019-06-26 0:48 ` Dong, Eric 2019-06-26 1:17 ` Gao, Zhichao @ 2019-06-26 1:35 ` Liming Gao 2019-06-26 1:58 ` [edk2-devel] " Zhang, Chao B 2019-06-26 1:57 ` Ni, Ray 2 siblings, 1 reply; 8+ messages in thread From: Liming Gao @ 2019-06-26 1:35 UTC (permalink / raw) To: Dong, Eric, Gao, Zhichao, devel@edk2.groups.io; +Cc: Ni, Ray, Laszlo Ersek 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.gao@intel.com>; devel@edk2.groups.io >Cc: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>; Gao, >Liming <liming.gao@intel.com> >Subject: RE: [PATCH V2] UefiCpuPkg/MpInitLib: MicrocodeDetect: Ensure >checked range is valid > >Hi Zhichao, > >Reviewed-by: Eric Dong <eric.dong@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.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Laszlo >> Ersek <lersek@redhat.com>; Gao, Liming <liming.gao@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.dong@intel.com> >> Cc: Ray Ni <ray.ni@intel.com> >> Cc: Laszlo Ersek <lersek@redhat.com> >> Cc: Liming Gao <liming.gao@intel.com> >> Signed-off-by: Zhichao Gao <zhichao.gao@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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [edk2-devel] [PATCH V2] UefiCpuPkg/MpInitLib: MicrocodeDetect: Ensure checked range is valid 2019-06-26 1:35 ` Liming Gao @ 2019-06-26 1:58 ` Zhang, Chao B 2019-06-26 4:38 ` Gao, Zhichao 0 siblings, 1 reply; 8+ messages in thread From: Zhang, Chao B @ 2019-06-26 1:58 UTC (permalink / raw) To: devel@edk2.groups.io, Gao, Liming, Dong, Eric, Gao, Zhichao Cc: Ni, Ray, Laszlo Ersek [-- Attachment #1: Type: text/plain, Size: 4588 bytes --] Hi All: Is that patch to fix potential overflow in MicroCodeEntryPoint + TotalSize? Is there a clearer way to check it? Like MAX_ADDRESS - TotalSize <= MicroCodeEntryPoint. And I suggest to add check before doing MicrroCodeEntryPoint + TotalSize. From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Liming Gao Sent: Wednesday, June 26, 2019 9:36 AM To: Dong, Eric <eric.dong@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io Cc: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com> Subject: Re: [edk2-devel] [PATCH V2] UefiCpuPkg/MpInitLib: MicrocodeDetect: Ensure checked range is valid 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.gao@intel.com<mailto:zhichao.gao@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io> >Cc: Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>; Gao, >Liming <liming.gao@intel.com<mailto:liming.gao@intel.com>> >Subject: RE: [PATCH V2] UefiCpuPkg/MpInitLib: MicrocodeDetect: Ensure >checked range is valid > >Hi Zhichao, > >Reviewed-by: Eric Dong <eric.dong@intel.com<mailto:eric.dong@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<mailto:devel@edk2.groups.io> >> Cc: Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Laszlo >> Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>; Gao, Liming <liming.gao@intel.com<mailto:liming.gao@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.dong@intel.com<mailto:eric.dong@intel.com>> >> Cc: Ray Ni <ray.ni@intel.com<mailto:ray.ni@intel.com>> >> Cc: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>> >> Cc: Liming Gao <liming.gao@intel.com<mailto:liming.gao@intel.com>> >> Signed-off-by: Zhichao Gao <zhichao.gao@intel.com<mailto:zhichao.gao@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 [-- Attachment #2: Type: text/html, Size: 14142 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [edk2-devel] [PATCH V2] UefiCpuPkg/MpInitLib: MicrocodeDetect: Ensure checked range is valid 2019-06-26 1:58 ` [edk2-devel] " Zhang, Chao B @ 2019-06-26 4:38 ` Gao, Zhichao 0 siblings, 0 replies; 8+ messages in thread From: Gao, Zhichao @ 2019-06-26 4:38 UTC (permalink / raw) To: Zhang, Chao B, devel@edk2.groups.io, Gao, Liming, Dong, Eric Cc: Ni, Ray, Laszlo Ersek [-- Attachment #1: Type: text/plain, Size: 5266 bytes --] From: Zhang, Chao B Sent: Wednesday, June 26, 2019 9:59 AM To: devel@edk2.groups.io; Gao, Liming <liming.gao@intel.com>; Dong, Eric <eric.dong@intel.com>; Gao, Zhichao <zhichao.gao@intel.com> Cc: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com> Subject: RE: [edk2-devel] [PATCH V2] UefiCpuPkg/MpInitLib: MicrocodeDetect: Ensure checked range is valid Hi All: Is that patch to fix potential overflow in MicroCodeEntryPoint + TotalSize? Yes. Is there a clearer way to check it? Like MAX_ADDRESS - TotalSize <= MicroCodeEntryPoint. And I suggest to add check before doing MicrroCodeEntryPoint + TotalSize. The above two is mentioned in Ray's comments. I would update the patch with your advices and fix the commit message. From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> [mailto:devel@edk2.groups.io] On Behalf Of Liming Gao Sent: Wednesday, June 26, 2019 9:36 AM To: Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; Gao, Zhichao <zhichao.gao@intel.com<mailto:zhichao.gao@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io> Cc: Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>> Subject: Re: [edk2-devel] [PATCH V2] UefiCpuPkg/MpInitLib: MicrocodeDetect: Ensure checked range is valid 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.gao@intel.com<mailto:zhichao.gao@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io> >Cc: Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>; Gao, >Liming <liming.gao@intel.com<mailto:liming.gao@intel.com>> >Subject: RE: [PATCH V2] UefiCpuPkg/MpInitLib: MicrocodeDetect: Ensure >checked range is valid > >Hi Zhichao, > >Reviewed-by: Eric Dong <eric.dong@intel.com<mailto:eric.dong@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<mailto:devel@edk2.groups.io> >> Cc: Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Laszlo >> Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>; Gao, Liming <liming.gao@intel.com<mailto:liming.gao@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.dong@intel.com<mailto:eric.dong@intel.com>> >> Cc: Ray Ni <ray.ni@intel.com<mailto:ray.ni@intel.com>> >> Cc: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>> >> Cc: Liming Gao <liming.gao@intel.com<mailto:liming.gao@intel.com>> >> Signed-off-by: Zhichao Gao <zhichao.gao@intel.com<mailto:zhichao.gao@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 [-- Attachment #2: Type: text/html, Size: 16218 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH V2] UefiCpuPkg/MpInitLib: MicrocodeDetect: Ensure checked range is valid 2019-06-26 0:48 ` Dong, Eric 2019-06-26 1:17 ` Gao, Zhichao 2019-06-26 1:35 ` Liming Gao @ 2019-06-26 1:57 ` Ni, Ray 2019-06-26 4:36 ` Gao, Zhichao 2 siblings, 1 reply; 8+ messages in thread From: Ni, Ray @ 2019-06-26 1:57 UTC (permalink / raw) To: Dong, Eric, Gao, Zhichao, devel@edk2.groups.io; +Cc: Laszlo Ersek, Gao, Liming > > @@ -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 || How about below check? First comparison hits when the sum of MicrocodeEntryPoint and TotalSize overflows. Second comparison hits when the sum crosses the boundary of the whole microcode buffer boundary If (((UINTN) MicrocodeEntryPoint > MAX_UINTN - TotalSize) || ((UINTN)MicrocodeEntryPoint + TotalSize) > MicrocodeEnd) > > (TotalSize & 0x3) != 0 > > ) { > > MicrocodeEntryPoint = (CPU_MICROCODE_HEADER *) (((UINTN) > > MicrocodeEntryPoint) + SIZE_1KB); > > -- > > 2.21.0.windows.1 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH V2] UefiCpuPkg/MpInitLib: MicrocodeDetect: Ensure checked range is valid 2019-06-26 1:57 ` Ni, Ray @ 2019-06-26 4:36 ` Gao, Zhichao 0 siblings, 0 replies; 8+ messages in thread From: Gao, Zhichao @ 2019-06-26 4:36 UTC (permalink / raw) To: Ni, Ray, Dong, Eric, devel@edk2.groups.io; +Cc: Laszlo Ersek, Gao, Liming > -----Original Message----- > From: Ni, Ray > Sent: Wednesday, June 26, 2019 9:58 AM > To: Dong, Eric <eric.dong@intel.com>; Gao, Zhichao > <zhichao.gao@intel.com>; devel@edk2.groups.io > Cc: Laszlo Ersek <lersek@redhat.com>; Gao, Liming <liming.gao@intel.com> > Subject: RE: [PATCH V2] UefiCpuPkg/MpInitLib: MicrocodeDetect: Ensure > checked range is valid > > > > @@ -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 || > > How about below check? > First comparison hits when the sum of MicrocodeEntryPoint and TotalSize > overflows. > Second comparison hits when the sum crosses the boundary of the whole > microcode buffer boundary If (((UINTN) MicrocodeEntryPoint > MAX_UINTN > - TotalSize) || ((UINTN)MicrocodeEntryPoint + TotalSize) > MicrocodeEnd) Your advice is better. It avoid the sum of MicrocodeEntryPoint and TotalSize bigger than CpuMpData->MicrocodePatchAddress and less than MicrocodeEntryPoint. I would update it with your comments. Thanks, Zhichao > > > > > (TotalSize & 0x3) != 0 > > > ) { > > > MicrocodeEntryPoint = (CPU_MICROCODE_HEADER *) (((UINTN) > > > MicrocodeEntryPoint) + SIZE_1KB); > > > -- > > > 2.21.0.windows.1 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-06-26 4:38 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-06-25 15:15 [PATCH V2] UefiCpuPkg/MpInitLib: MicrocodeDetect: Ensure checked range is valid Gao, Zhichao 2019-06-26 0:48 ` Dong, Eric 2019-06-26 1:17 ` Gao, Zhichao 2019-06-26 1:35 ` Liming Gao 2019-06-26 1:58 ` [edk2-devel] " Zhang, Chao B 2019-06-26 4:38 ` Gao, Zhichao 2019-06-26 1:57 ` Ni, Ray 2019-06-26 4:36 ` Gao, Zhichao
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox