Re: [PATCH v1 1/1] IntelSiliconPkg/ShadowMicrocodePei: Add microcode header verification.

["Ni, Ray" <ray.ni@intel.com>]

Reviewed-by: Ray Ni <ray.ni@intel.com>

> -----Original Message-----
> From: Fu, Siyuan <siyuan.fu@intel.com>
> Sent: Thursday, February 4, 2021 1:40 PM
> To: Li, Aaron <aaron.li@intel.com>; devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>; Chaganty, Rangasai V
> <rangasai.v.chaganty@intel.com>
> Subject: RE: [PATCH v1 1/1] IntelSiliconPkg/ShadowMicrocodePei: Add
> microcode header verification.
> 
> Reviewed-by: Siyuan Fu <siyuan.fu@intel.com>
> 
> > -----Original Message-----
> > From: Li, Aaron <aaron.li@intel.com>
> > Sent: 2021年2月3日 15:06
> > To: devel@edk2.groups.io
> > Cc: Ni, Ray <ray.ni@intel.com>; Chaganty, Rangasai V
> > <rangasai.v.chaganty@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>
> > Subject: [PATCH v1 1/1] IntelSiliconPkg/ShadowMicrocodePei: Add
> microcode
> > header verification.
> >
> > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3196
> >
> > Microcode header should be checked before calling
> > IsMicrocodePatchNeedLoad(). This is to make sure garbage value after
> > remove microcode from FV would not cause stack overflow in
> > IsMicrocodePatchNeedLoad().
> >
> > Signed-off-by: Aaron Li <aaron.li@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Rangasai V Chaganty <rangasai.v.chaganty@intel.com>
> > Cc: Siyuan Fu <siyuan.fu@intel.com>
> > ---
> >
> >
> Silicon/Intel/IntelSiliconPkg/Feature/ShadowMicrocode/ShadowMicrocodeP
> ei.c
> > | 30 +++++++++++++++++++-
> >  1 file changed, 29 insertions(+), 1 deletion(-)
> >
> > diff --git
> >
> a/Silicon/Intel/IntelSiliconPkg/Feature/ShadowMicrocode/ShadowMicrocod
> ePe
> > i.c
> >
> b/Silicon/Intel/IntelSiliconPkg/Feature/ShadowMicrocode/ShadowMicrocod
> ePe
> > i.c
> > index 1494397a8e36..98a7aed69757 100644
> > ---
> >
> a/Silicon/Intel/IntelSiliconPkg/Feature/ShadowMicrocode/ShadowMicrocod
> ePe
> > i.c
> > +++
> >
> b/Silicon/Intel/IntelSiliconPkg/Feature/ShadowMicrocode/ShadowMicrocod
> ePe
> > i.c
> > @@ -402,6 +402,7 @@ ShadowMicrocode (
> >    UINTN                             MaxPatchNumber;
> >
> >    CPU_MICROCODE_HEADER              *MicrocodeEntryPoint;
> >
> >    UINTN                             PatchCount;
> >
> > +  UINTN                             DataSize;
> >
> >    UINTN                             TotalSize;
> >
> >    UINTN                             TotalLoadSize;
> >
> >
> >
> > @@ -446,7 +447,34 @@ ShadowMicrocode (
> >    for (Index = 0; Index < EntryNum; Index++) {
> >
> >      if (FitEntry[Index].Type == FIT_TYPE_01_MICROCODE) {
> >
> >        MicrocodeEntryPoint = (CPU_MICROCODE_HEADER *) (UINTN)
> > FitEntry[Index].Address;
> >
> > -      TotalSize = (MicrocodeEntryPoint->DataSize == 0) ? 2048 :
> > MicrocodeEntryPoint->TotalSize;
> >
> > +
> >
> > +      if (*(UINT32 *) MicrocodeEntryPoint == 0xFFFFFFFF) {
> >
> > +        //
> >
> > +        // An empty slot for reserved microcode update, skip to check next
> entry.
> >
> > +        //
> >
> > +        continue;
> >
> > +      }
> >
> > +
> >
> > +      if (MicrocodeEntryPoint->HeaderVersion != 0x1) {
> >
> > +        //
> >
> > +        // Not a valid microcode header, skip to check next entry.
> >
> > +        //
> >
> > +        continue;
> >
> > +      }
> >
> > +
> >
> > +      DataSize  = MicrocodeEntryPoint->DataSize;
> >
> > +      TotalSize = (DataSize == 0) ? 2048 : MicrocodeEntryPoint->TotalSize;
> >
> > +      if ( (UINTN)MicrocodeEntryPoint > (MAX_ADDRESS - TotalSize) ||
> >
> > +           (DataSize & 0x3) != 0 ||
> >
> > +           (TotalSize & (SIZE_1KB - 1)) != 0 ||
> >
> > +           TotalSize < DataSize
> >
> > +        ) {
> >
> > +        //
> >
> > +        // Not a valid microcode header, skip to check next entry.
> >
> > +        //
> >
> > +        continue;
> >
> > +      }
> >
> > +
> >
> >        if (IsMicrocodePatchNeedLoad (CpuIdCount, MicrocodeCpuId,
> > MicrocodeEntryPoint)) {
> >
> >          PatchInfoBuffer[PatchCount].Address     = (UINTN)
> MicrocodeEntryPoint;
> >
> >          PatchInfoBuffer[PatchCount].Size        = TotalSize;
> >
> > --
> > 2.29.2.windows.2