Re: [PATCH 2/2] UefiCpuPkg/Microcode: Add verification logic before parsing payload

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

> -----Original Message-----
> From: edk2-devel <edk2-devel-bounces@lists.01.org> On Behalf Of Chen A
> Chen
> Sent: Thursday, February 28, 2019 2:03 PM
> To: edk2-devel@lists.01.org
> Cc: Dong, Eric <eric.dong@intel.com>
> Subject: [edk2] [PATCH 2/2] UefiCpuPkg/Microcode: Add verification logic
> before parsing payload
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1020
> 
> The function MicrocodeDetect may receive untrusted input. Add verification
> logic to check Microcode Payload Header.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Chen A Chen <chen.a.chen@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> ---
>  UefiCpuPkg/Library/MpInitLib/Microcode.c | 190
> +++++++++++++++++++++----------
>  1 file changed, 127 insertions(+), 63 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/Microcode.c
> b/UefiCpuPkg/Library/MpInitLib/Microcode.c
> index 5f9ae22794..1d5e964791 100644
> --- a/UefiCpuPkg/Library/MpInitLib/Microcode.c
> +++ b/UefiCpuPkg/Library/MpInitLib/Microcode.c
> @@ -32,6 +32,69 @@ GetCurrentMicrocodeSignature (
>    return BiosSignIdMsr.Bits.MicrocodeUpdateSignature;
>  }
> 
> +/**
> +  Verify Microcode.
> +
> +  @param[in]  MicrocodeHeader       Point to the Microcode payload buffer.
> +
> +  @retval     TRUE                  Valid payload
> +  @retval     FALSE                 Invalid payload
> +**/
> +BOOLEAN
> +VerifyMicrocodeHeader (


1. "Verify" cannot tell what the return value stands for. Use "IsMicrocodeHeaderValid"?

> +  IN CPU_MICROCODE_HEADER *MicrocodeHeader
> +  )
> +{
> +  UINTN                   TotalSize;
> +  UINTN                   DataSize;
> +
> +  if (MicrocodeHeader == NULL) {
> +    return FALSE;
> +  }
2. Can we use assert here?

> +
> +  ///
> +  /// Verify Version
> +  ///
> +  if (MicrocodeHeader->HeaderVersion != 0x1) {
> +    return FALSE;
> +  }
> +  if (MicrocodeHeader->LoaderRevision != 0x1) {
> +    return FALSE;
> +  }
> +
> +  ///
> +  /// Verify TotalSize
> +  ///
> +  if (MicrocodeHeader->DataSize == 0) {
> +    TotalSize = 2048;
> +  } else {
> +    TotalSize = MicrocodeHeader->TotalSize;
> +  }
> +  if (TotalSize <= sizeof (CPU_MICROCODE_HEADER)) {
> +    return FALSE;
> +  }

3. Can you please add more comments here to describe the alignment requirement?

> +  if ((TotalSize & (SIZE_1KB - 1)) != 0) {
> +    return FALSE;
> +  }
> +
> +  ///
> +  /// Verify DataSize
> +  ///
> +  if (MicrocodeHeader->DataSize == 0) {
> +    DataSize = 2048 - sizeof (CPU_MICROCODE_HEADER);
> +  } else {
> +    DataSize = MicrocodeHeader->DataSize;
> +  }
> +  if (DataSize > TotalSize - sizeof (CPU_MICROCODE_HEADER)) {

4. if MIcrocodeHeader->DataSize is 0, seems the if-check is redundant.

> +    return FALSE;
> +  }

5. Can you please add more comments here to describe the alignment requirement?
> +  if ((DataSize & 0x3) != 0) {
> +    return FALSE;
> +  }
> +
> +  return TRUE;
> +}
> +
>  /**
>    Detect whether specified processor can find matching microcode patch and
> load it.
> 
> @@ -166,6 +229,18 @@ MicrocodeDetect (
>      //
>      CorrectMicrocode = FALSE;
> 
> +    if (!VerifyMicrocodeHeader (MicrocodeEntryPoint)) {
> +      //
> +      // It is the padding data between the microcode patches for microcode
> patches alignment.
> +      // Because the microcode patch is the multiple of 1-KByte, the padding
> data should not
> +      // exist if the microcode patch alignment value is not larger than 1-KByte.
> So, the microcode
> +      // alignment value should be larger than 1-KByte. We could skip
> SIZE_1KB padding data to
> +      // find the next possible microcode patch header.
> +      //
> +      MicrocodeEntryPoint = (CPU_MICROCODE_HEADER *) (((UINTN)
> MicrocodeEntryPoint) + SIZE_1KB);
> +      continue;
> +    }
> +
>      //
>      // Save an in-complete CheckSum32 from CheckSum Part1 for common
> parts.
>      //
> @@ -184,90 +259,79 @@ MicrocodeDetect (
>      InCompleteCheckSum32 -= MicrocodeEntryPoint->ProcessorFlags;
>      InCompleteCheckSum32 -= MicrocodeEntryPoint->Checksum;
> 
> -    if (MicrocodeEntryPoint->HeaderVersion == 0x1) {
> +    //
> +    // It is the microcode header. It is not the padding data between
> microcode patches
> +    // because the padding data should not include 0x00000001 and it should
> be the repeated
> +    // byte format (like 0xXYXYXYXY....).
> +    //
> +    if (MicrocodeEntryPoint->ProcessorSignature.Uint32 == Eax.Uint32 &&
> +        MicrocodeEntryPoint->UpdateRevision > LatestRevision &&
> +        (MicrocodeEntryPoint->ProcessorFlags & (1 << PlatformId))
> +        ) {
>        //
> -      // It is the microcode header. It is not the padding data between
> microcode patches
> -      // because the padding data should not include 0x00000001 and it should
> be the repeated
> -      // byte format (like 0xXYXYXYXY....).
> +      // Calculate CheckSum Part1.
>        //
> -      if (MicrocodeEntryPoint->ProcessorSignature.Uint32 == Eax.Uint32 &&
> -          MicrocodeEntryPoint->UpdateRevision > LatestRevision &&
> -          (MicrocodeEntryPoint->ProcessorFlags & (1 << PlatformId))
> -          ) {
> +      CheckSum32 = InCompleteCheckSum32;
> +      CheckSum32 += MicrocodeEntryPoint->ProcessorSignature.Uint32;
> +      CheckSum32 += MicrocodeEntryPoint->ProcessorFlags;
> +      CheckSum32 += MicrocodeEntryPoint->Checksum;
> +      if (CheckSum32 == 0) {
> +        CorrectMicrocode = TRUE;
> +        ProcessorFlags = MicrocodeEntryPoint->ProcessorFlags;
> +      }
> +    } else if ((MicrocodeEntryPoint->DataSize != 0) &&
> +               (MicrocodeEntryPoint->UpdateRevision > LatestRevision)) {
> +      ExtendedTableLength = MicrocodeEntryPoint->TotalSize -
> (MicrocodeEntryPoint->DataSize +
> +                              sizeof (CPU_MICROCODE_HEADER));
> +      if (ExtendedTableLength != 0) {
>          //
> -        // Calculate CheckSum Part1.
> +        // Extended Table exist, check if the CPU in support list
>          //
> -        CheckSum32 = InCompleteCheckSum32;
> -        CheckSum32 += MicrocodeEntryPoint->ProcessorSignature.Uint32;
> -        CheckSum32 += MicrocodeEntryPoint->ProcessorFlags;
> -        CheckSum32 += MicrocodeEntryPoint->Checksum;
> -        if (CheckSum32 == 0) {
> -          CorrectMicrocode = TRUE;
> -          ProcessorFlags = MicrocodeEntryPoint->ProcessorFlags;
> -        }
> -      } else if ((MicrocodeEntryPoint->DataSize != 0) &&
> -                 (MicrocodeEntryPoint->UpdateRevision > LatestRevision)) {
> -        ExtendedTableLength = MicrocodeEntryPoint->TotalSize -
> (MicrocodeEntryPoint->DataSize +
> -                                sizeof (CPU_MICROCODE_HEADER));
> -        if (ExtendedTableLength != 0) {
> -          //
> -          // Extended Table exist, check if the CPU in support list
> -          //
> -          ExtendedTableHeader =
> (CPU_MICROCODE_EXTENDED_TABLE_HEADER *) ((UINT8 *)
> (MicrocodeEntryPoint)
> -                                  + MicrocodeEntryPoint->DataSize + sizeof
> (CPU_MICROCODE_HEADER));
> +        ExtendedTableHeader =
> (CPU_MICROCODE_EXTENDED_TABLE_HEADER *) ((UINT8 *)
> (MicrocodeEntryPoint)
> +                                + MicrocodeEntryPoint->DataSize + sizeof
> (CPU_MICROCODE_HEADER));
> +        //
> +        // Calculate Extended Checksum
> +        //
> +        if ((ExtendedTableLength % 4) == 0) {
>            //
> -          // Calculate Extended Checksum
> +          // Calculate CheckSum Part2.
>            //
> -          if ((ExtendedTableLength % 4) == 0) {
> +          CheckSum32 = CalculateSum32 ((UINT32 *) ExtendedTableHeader,
> ExtendedTableLength);
> +          if (CheckSum32 == 0) {
>              //
> -            // Calculate CheckSum Part2.
> +            // Checksum correct
>              //
> -            CheckSum32 = CalculateSum32 ((UINT32 *) ExtendedTableHeader,
> ExtendedTableLength);
> -            if (CheckSum32 == 0) {
> +            ExtendedTableCount = ExtendedTableHeader-
> >ExtendedSignatureCount;
> +            ExtendedTable      = (CPU_MICROCODE_EXTENDED_TABLE *)
> (ExtendedTableHeader + 1);
> +            for (Index = 0; Index < ExtendedTableCount; Index ++) {
>                //
> -              // Checksum correct
> +              // Calculate CheckSum Part3.
>                //
> -              ExtendedTableCount = ExtendedTableHeader-
> >ExtendedSignatureCount;
> -              ExtendedTable      = (CPU_MICROCODE_EXTENDED_TABLE *)
> (ExtendedTableHeader + 1);
> -              for (Index = 0; Index < ExtendedTableCount; Index ++) {
> +              CheckSum32 = InCompleteCheckSum32;
> +              CheckSum32 += ExtendedTable->ProcessorSignature.Uint32;
> +              CheckSum32 += ExtendedTable->ProcessorFlag;
> +              CheckSum32 += ExtendedTable->Checksum;
> +              if (CheckSum32 == 0) {
>                  //
> -                // Calculate CheckSum Part3.
> +                // Verify Header
>                  //
> -                CheckSum32 = InCompleteCheckSum32;
> -                CheckSum32 += ExtendedTable->ProcessorSignature.Uint32;
> -                CheckSum32 += ExtendedTable->ProcessorFlag;
> -                CheckSum32 += ExtendedTable->Checksum;
> -                if (CheckSum32 == 0) {
> +                if ((ExtendedTable->ProcessorSignature.Uint32 == Eax.Uint32) &&
> +                    (ExtendedTable->ProcessorFlag & (1 << PlatformId)) ) {
>                    //
> -                  // Verify Header
> +                  // Find one
>                    //
> -                  if ((ExtendedTable->ProcessorSignature.Uint32 == Eax.Uint32) &&
> -                      (ExtendedTable->ProcessorFlag & (1 << PlatformId)) ) {
> -                    //
> -                    // Find one
> -                    //
> -                    CorrectMicrocode = TRUE;
> -                    ProcessorFlags = ExtendedTable->ProcessorFlag;
> -                    break;
> -                  }
> +                  CorrectMicrocode = TRUE;
> +                  ProcessorFlags = ExtendedTable->ProcessorFlag;
> +                  break;
>                  }
> -                ExtendedTable ++;
>                }
> +              ExtendedTable ++;
>              }
>            }
>          }
>        }
> -    } else {
> -      //
> -      // It is the padding data between the microcode patches for microcode
> patches alignment.
> -      // Because the microcode patch is the multiple of 1-KByte, the padding
> data should not
> -      // exist if the microcode patch alignment value is not larger than 1-KByte.
> So, the microcode
> -      // alignment value should be larger than 1-KByte. We could skip SIZE_1KB
> padding data to
> -      // find the next possible microcode patch header.
> -      //
> -      MicrocodeEntryPoint = (CPU_MICROCODE_HEADER *) (((UINTN)
> MicrocodeEntryPoint) + SIZE_1KB);
> -      continue;
>      }
> +
>      //
>      // Get the next patch.
>      //
> --
> 2.16.2.windows.1
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel