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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#42833): https://edk2.groups.io/g/devel/message/42833
Mute This Topic: https://groups.io/mt/32204622/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#42842): https://edk2.groups.io/g/devel/message/42842
Mute This Topic: https://groups.io/mt/32204622/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#42845): https://edk2.groups.io/g/devel/message/42845
Mute This Topic: https://groups.io/mt/32204622/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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


-=-=-=-=-=-=-=-=-=-=-=-
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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#42853): https://edk2.groups.io/g/devel/message/42853
Mute This Topic: https://groups.io/mt/32204622/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#42862): https://edk2.groups.io/g/devel/message/42862
Mute This Topic: https://groups.io/mt/32204622/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

> > @@ -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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#42852): https://edk2.groups.io/g/devel/message/42852
Mute This Topic: https://groups.io/mt/32204622/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

> -----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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#42861): https://edk2.groups.io/g/devel/message/42861
Mute This Topic: https://groups.io/mt/32204622/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-