Re: [PATCH] UefiCpuPkg/MpInitLib: Move checksum part for valid microcode data

["Laszlo Ersek" <lersek@redhat.com>]

On 06/24/19 08:14, Zhichao Gao wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1934
> 
> 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.
> 
> 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 | 52 ++++++++++++------------
>  1 file changed, 26 insertions(+), 26 deletions(-)

I'll defer on this patch to the (other) UefiCpuPkg reviewers /
maintainers. OVMF inherits the default zero value for
"PcdCpuMicrocodePatchRegionSize", from "UefiCpuPkg.dec". Therefore the
first check in MicrocodeDetect() evaluates to TRUE, and the function
exits immediately, in OVMF.

Thanks
Laszlo

> diff --git a/UefiCpuPkg/Library/MpInitLib/Microcode.c b/UefiCpuPkg/Library/MpInitLib/Microcode.c
> index 4763dcfebe..f1a42f2d4e 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
>  
>  **/
> @@ -160,34 +160,34 @@ MicrocodeDetect (
>      //
>      CorrectMicrocode = FALSE;
>  
> -    if (MicrocodeEntryPoint->DataSize == 0) {
> -      TotalSize = sizeof (CPU_MICROCODE_HEADER) + 2000;
> -    } else {
> -      TotalSize = sizeof (CPU_MICROCODE_HEADER) + MicrocodeEntryPoint->DataSize;
> -    }
> +    if (MicrocodeEntryPoint->HeaderVersion == 0x1) {
> +      if (MicrocodeEntryPoint->DataSize == 0) {
> +        TotalSize = sizeof (CPU_MICROCODE_HEADER) + 2000;
> +      } else {
> +        TotalSize = sizeof (CPU_MICROCODE_HEADER) + MicrocodeEntryPoint->DataSize;
> +      }
>  
> -    ///
> -    /// Check overflow and whether TotalSize is aligned with 4 bytes.
> -    ///
> -    if ( ((UINTN)MicrocodeEntryPoint + TotalSize) > MicrocodeEnd ||
> -         (TotalSize & 0x3) != 0
> -       ) {
> -      MicrocodeEntryPoint = (CPU_MICROCODE_HEADER *) (((UINTN) MicrocodeEntryPoint) + SIZE_1KB);
> -      continue;
> -    }
> +      ///
> +      /// Check overflow and whether TotalSize is aligned with 4 bytes.
> +      ///
> +      if ( ((UINTN)MicrocodeEntryPoint + TotalSize) > MicrocodeEnd ||
> +           (TotalSize & 0x3) != 0
> +         ) {
> +        MicrocodeEntryPoint = (CPU_MICROCODE_HEADER *) (((UINTN) MicrocodeEntryPoint) + SIZE_1KB);
> +        continue;
> +      }
>  
> -    //
> -    // Save an in-complete CheckSum32 from CheckSum Part1 for common parts.
> -    //
> -    InCompleteCheckSum32 = CalculateSum32 (
> -                             (UINT32 *) MicrocodeEntryPoint,
> -                             TotalSize
> -                             );
> -    InCompleteCheckSum32 -= MicrocodeEntryPoint->ProcessorSignature.Uint32;
> -    InCompleteCheckSum32 -= MicrocodeEntryPoint->ProcessorFlags;
> -    InCompleteCheckSum32 -= MicrocodeEntryPoint->Checksum;
> +      //
> +      // Save an in-complete CheckSum32 from CheckSum Part1 for common parts.
> +      //
> +      InCompleteCheckSum32 = CalculateSum32 (
> +                               (UINT32 *) MicrocodeEntryPoint,
> +                               TotalSize
> +                               );
> +      InCompleteCheckSum32 -= MicrocodeEntryPoint->ProcessorSignature.Uint32;
> +      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
>