1. Patchew
  2. EDK2
  3. View series

[edk2-devel] [PATCH] UefiCpuPkg/MpInitLib: Move checksum part for valid microcode data

Gao, Zhichao posted 1 patch 4 years, 10 months ago
Failed in applying to current master (apply log)
UefiCpuPkg/Library/MpInitLib/Microcode.c | 52 ++++++++++++------------
1 file changed, 26 insertions(+), 26 deletions(-)
[edk2-devel] [PATCH] UefiCpuPkg/MpInitLib: Move checksum part for valid microcode data
Posted by Gao, Zhichao 4 years, 10 months ago 
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(-)

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
-- 
2.21.0.windows.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#42733): https://edk2.groups.io/g/devel/message/42733
Mute This Topic: https://groups.io/mt/32189383/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH] UefiCpuPkg/MpInitLib: Move checksum part for valid microcode data
Posted by Laszlo Ersek 4 years, 10 months ago 
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
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#42757): https://edk2.groups.io/g/devel/message/42757
Mute This Topic: https://groups.io/mt/32189383/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.