From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 134.134.136.126, mailfrom: chao.b.zhang@intel.com) Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) by groups.io with SMTP; Tue, 25 Jun 2019 18:59:02 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga005.jf.intel.com ([10.7.209.41]) by orsmga106.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 25 Jun 2019 18:59:01 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.63,418,1557212400"; d="scan'208,217";a="337052324" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by orsmga005.jf.intel.com with ESMTP; 25 Jun 2019 18:59:00 -0700 Received: from FMSMSX109.amr.corp.intel.com (10.18.116.9) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.439.0; Tue, 25 Jun 2019 18:59:01 -0700 Received: from shsmsx108.ccr.corp.intel.com (10.239.4.97) by fmsmsx109.amr.corp.intel.com (10.18.116.9) with Microsoft SMTP Server (TLS) id 14.3.439.0; Tue, 25 Jun 2019 18:59:00 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.33]) by SHSMSX108.ccr.corp.intel.com ([169.254.8.236]) with mapi id 14.03.0439.000; Wed, 26 Jun 2019 09:58:58 +0800 From: "Zhang, Chao B" To: "devel@edk2.groups.io" , "Gao, Liming" , "Dong, Eric" , "Gao, Zhichao" CC: "Ni, Ray" , Laszlo Ersek Subject: Re: [edk2-devel] [PATCH V2] UefiCpuPkg/MpInitLib: MicrocodeDetect: Ensure checked range is valid Thread-Topic: [edk2-devel] [PATCH V2] UefiCpuPkg/MpInitLib: MicrocodeDetect: Ensure checked range is valid Thread-Index: AQHVK2jwaM0XPpsGk0WuPAu4kT5wkqaslLKAgAANXQCAAItqUA== Date: Wed, 26 Jun 2019 01:58:58 +0000 Message-ID: References: <20190625151541.28632-1-zhichao.gao@intel.com> <4A89E2EF3DFEDB4C8BFDE51014F606A14E48D584@SHSMSX104.ccr.corp.intel.com> In-Reply-To: <4A89E2EF3DFEDB4C8BFDE51014F606A14E48D584@SHSMSX104.ccr.corp.intel.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ctpclassification: CTP_NT x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiYTZhZTZhODUtMjdlYy00MGRlLWEzY2MtMzkwNzhmMzU0ZjJkIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiaW5ybWtGYTdZOGE0NUFndlVZUk9yUHl4Z0ttbXBCa2FlSUFqU0FLSll3K1FwVWE2Zk9lRXRvNTJKQUNMWHBpRiJ9 dlp-product: dlpe-windows dlp-version: 11.0.600.7 dlp-reaction: no-action x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: chao.b.zhang@intel.com Content-Language: en-US Content-Type: multipart/alternative; boundary="_000_FF72C7E4248F3C4E9BDF19D4918E90F24DEDEF8Bshsmsx102ccrcor_" --_000_FF72C7E4248F3C4E9BDF19D4918E90F24DEDEF8Bshsmsx102ccrcor_ Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Hi All: Is that patch to fix potential overflow in MicroCodeEntryPoint + TotalSize= ? Is there a clearer way to check it? Like MAX_ADDRESS - TotalSize <=3D Mic= roCodeEntryPoint. And I suggest to add check before doing MicrroCodeEntryPoint + TotalSize. From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Limi= ng Gao Sent: Wednesday, June 26, 2019 9:36 AM To: Dong, Eric ; Gao, Zhichao = ; devel@edk2.groups.io Cc: Ni, Ray ; Laszlo Ersek 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 >; d= evel@edk2.groups.io >Cc: Ni, Ray >; Laszlo Ersek >; Gao, >Liming > >Subject: RE: [PATCH V2] UefiCpuPkg/MpInitLib: MicrocodeDetect: Ensure >checked range is valid > >Hi Zhichao, > >Reviewed-by: Eric Dong > > >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 >; Ni, R= ay >; Laszlo >> Ersek >; Gao, Liming > >> Subject: [PATCH V2] UefiCpuPkg/MpInitLib: MicrocodeDetect: Ensure >> checked range is valid >> >> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D1934 >> >> V1: >> Originally, the checksum part would done before verfiy the microcode da= ta. >> 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 (MicrocodeEntryPoi= nt- >> >HeaderVersion =3D=3D 0x1)' section for a valid microcode data. >> >> V2: >> 'if (MicrocodeEntryPoint->HeaderVersion =3D=3D 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 o= f >non >> microcde data may make (MicrocodeEntryPoint + TotalSize) larger than >> 0xffffffff. For PEI driver, UINTN is 32bit and the result is overflow a= nd it may >> be a very small value. That means the checksum check would be done out = of >> the microcode range. >> >> Cc: Eric Dong > >> Cc: Ray Ni > >> Cc: Laszlo Ersek > >> Cc: Liming Gao > >> Signed-off-by: Zhichao Gao > >> --- >> 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. >> + Copyright (c) 2015 - 2019, Intel Corporation. All rights >> + reserved.
>> 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) !=3D 0 >> ) { >> MicrocodeEntryPoint =3D (CPU_MICROCODE_HEADER *) (((UINTN) >> MicrocodeEntryPoint) + SIZE_1KB); >> -- >> 2.21.0.windows.1 --_000_FF72C7E4248F3C4E9BDF19D4918E90F24DEDEF8Bshsmsx102ccrcor_ Content-Type: text/html; charset="us-ascii" Content-Transfer-Encoding: quoted-printable

Hi All:

Is that p= atch to fix potential overflow in MicroCodeEntryPoint + TotalSize?=

Is there = a clearer way to check it?  Like MAX_ADDRESS – TotalSize <=3D= MicroCodeEntryPoint.

And I sug= gest 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 <zhicha= o.gao@intel.com>; devel@edk2.groups.io
Cc: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redha= t.com>
Subject: Re: [edk2-devel] [PATCH V2] UefiCpuPkg/MpInitLib: Microcod= eDetect: Ensure checked range is valid

 

Zhichao:
  One generic comment,&nbs= p;the commit message doesn't need to include&= nbsp;V1, V2. It is just the change descr= iption. 

Thanks
Liming
>-----Original Message----- >From: Dong, Eric
>Sent: Wednesday, June 26,&= nbsp;2019 8:48 AM
>To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io
>Cc: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek&= nbsp;<lersek@redhat.com>;&nb= sp;Gao,
>Liming <liming.gao@intel.com>
>Subject: RE: [PATCH V2]&nb= sp;UefiCpuPkg/MpInitLib: MicrocodeDetect: Ensure
>checked range is valid
>
>Hi Zhichao,
>
>Reviewed-by: Eric Dong <= ;eric.dong@intel.com><= br> >
>It's better to add so= me comments in the code to explain the&n= bsp;change which
>make the code easy to=  be understood.
>
>Thanks,
>Eric
>
>> -----Original Message-----
>> From: Gao, Zhichao
>> Sent: Tuesday, June&nbs= p;25, 2019 11:16 PM
>> To: devel@edk2.groups.io
>> Cc: Dong, Eric <= ;eric.dong@intel.com>; N= i, Ray <ray.ni@intel.com>; Laszlo
>> Ersek <lersek@redhat.com>; Gao, Liming &l= t;liming.gao@intel.com>
>> Subject: [PATCH V2]&nbs= p;UefiCpuPkg/MpInitLib: MicrocodeDetect: Ensure
>> checked range is v= alid
>>
>> REF: https://bugzilla.tianocore.org/s= how_bug.cgi?id=3D1934
>>
>> V1:
>> Originally, the checksu= m part would done before verfiy the micr= ocode data.
>> Which meas the che= cksum would be done for a meaningless da= ta.
>> It would cause a&n= bsp;incorrect TotalSize (the size of microcode&nbs= p;data), then
>> incorrect checksum and&= nbsp;incorrect pointer increasing would happen.<= br> >> To fix this, move&= nbsp;the checksum part 1 section in 'if = (MicrocodeEntryPoint-
>> >HeaderVersion =3D=3D&nb= sp;0x1)' section for a valid microcode data.<= /span>
>>
>> V2:
>> 'if (MicrocodeEntryPoint->= ;HeaderVersion =3D=3D 0x1)' condition doesn't make=
>> sure the entry dat= a is a valid microcode. So abandon it.&n= bsp;Instead, make sure
>> the checked data i= s in the microcode data range. Because t= he DataSize of
>non
>> microcde data may = make (MicrocodeEntryPoint + TotalSize) larger = than
>> 0xffffffff. For PEI&nbs= p;driver, UINTN is 32bit and the result = is overflow and it may
>> be a very small&nb= sp;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 &= lt;lersek@redhat.com> >> Cc: Liming Gao <= ;liming.gao@intel.com>
>> Signed-off-by: Zhichao = Gao <zhichao.gao@intel.com= >
>> ---
>>  UefiCpuPkg/Library/MpInitLi= b/Microcode.c | 3 ++-
>>  1 file changed,&n= bsp;2 insertions(+), 1 deletion(-)
>>
>> diff --git a/UefiCpuPkg= /Library/MpInitLib/Microcode.c
>> b/UefiCpuPkg/Library/MpInitLib/Mi= crocode.c
>> index 4763dcfebe..6c0995cb0d=  100644
>> --- a/UefiCpuPkg/Library/MpI= nitLib/Microcode.c
>> +++ b/UefiCpuPkg= /Library/MpInitLib/Microcode.c
>> @@ -1,7 +1,7 @= @
>>  /** @file
>>    Implementation&= nbsp;of loading microcode on processors.
>>
>> -  Copyright (c)&n= bsp;2015 - 2018, Intel Corporation. All right= s reserved.<BR>
>> +  Copyright (= c) 2015 - 2019, Intel Corporation. All r= ights
>> + reserved.<BR>
>>    SPDX-License-Id= entifier: BSD-2-Clause-Patent
>>
>>  **/
>> @@ -170,6 +170,7&nb= sp;@@ MicrocodeDetect (
>>      ///=  Check overflow and whether TotalSize is = ;aligned with 4 bytes.
>>      ///=
>>      if&= nbsp;( ((UINTN)MicrocodeEntryPoint + TotalSize) >= ; MicrocodeEnd ||
>> +    &nbs= p;    ((UINTN)MicrocodeEntryPoint + Total= Size) < (UINTN)
>> + CpuMpData->Microcod= ePatchAddress ||
>>      &nb= sp;    (TotalSize & 0x3) !=3D 0=
>>      &nb= sp;  ) {
>>      &nb= sp; MicrocodeEntryPoint =3D (CPU_MICROCODE_HEADER *)&nb= sp;(((UINTN)
>> MicrocodeEntryPoint) +&n= bsp;SIZE_1KB);
>> --
>> 2.21.0.windows.1


--_000_FF72C7E4248F3C4E9BDF19D4918E90F24DEDEF8Bshsmsx102ccrcor_--