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

[Gao, Zhichao]

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 at intel.com>; devel at edk2.groups.io
> Cc: Ni, Ray <ray.ni at intel.com>; Laszlo Ersek <lersek at redhat.com>; Gao,
> Liming <liming.gao at intel.com>
> Subject: RE: [PATCH V2] UefiCpuPkg/MpInitLib: MicrocodeDetect: Ensure
> checked range is valid
> 
> Hi Zhichao,
> 
> Reviewed-by: Eric Dong <eric.dong at 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 at edk2.groups.io
> > Cc: Dong, Eric <eric.dong at intel.com>; Ni, Ray <ray.ni at intel.com>;
> > Laszlo Ersek <lersek at redhat.com>; Gao, Liming <liming.gao at 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 at intel.com>
> > Cc: Ray Ni <ray.ni at intel.com>
> > Cc: Laszlo Ersek <lersek at redhat.com>
> > Cc: Liming Gao <liming.gao at intel.com>
> > Signed-off-by: Zhichao Gao <zhichao.gao at 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/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-