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

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

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