* [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: [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: [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: [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
* 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
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