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

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

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