Re: [edk2-devel] [PATCH V2] UefiCpuPkg/MpInitLib: MicrocodeDetect: Ensure checked range is valid

["Gao, Zhichao" <zhichao.gao@intel.com>]

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