* [PATCH 0/2] Fix Microcode failure issue
@ 2019-02-28 6:02 Chen A Chen
2019-02-28 6:02 ` [PATCH 1/2] UefiCpuPkg/Microcode: Fix InComplete CheckSum32 issue Chen A Chen
2019-02-28 6:02 ` [PATCH 2/2] UefiCpuPkg/Microcode: Add verification logic before parsing payload Chen A Chen
0 siblings, 2 replies; 5+ messages in thread
From: Chen A Chen @ 2019-02-28 6:02 UTC (permalink / raw)
To: edk2-devel
The first patch is used to fix InComplete CheckSum32 issue.
Add a verification logic to check Microcode header before parsing payload.
Chen A Chen (2):
UefiCpuPkg/Microcode: Fix InComplete CheckSum32 issue
UefiCpuPkg/Microcode: Add verification logic before parsing payload
UefiCpuPkg/Library/MpInitLib/Microcode.c | 227 ++++++++++++++++++++-----------
1 file changed, 146 insertions(+), 81 deletions(-)
--
2.16.2.windows.1
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 1/2] UefiCpuPkg/Microcode: Fix InComplete CheckSum32 issue
2019-02-28 6:02 [PATCH 0/2] Fix Microcode failure issue Chen A Chen
@ 2019-02-28 6:02 ` Chen A Chen
2019-02-28 6:14 ` Ni, Ray
2019-02-28 6:02 ` [PATCH 2/2] UefiCpuPkg/Microcode: Add verification logic before parsing payload Chen A Chen
1 sibling, 1 reply; 5+ messages in thread
From: Chen A Chen @ 2019-02-28 6:02 UTC (permalink / raw)
To: edk2-devel; +Cc: Chen A Chen, Ray Ni, Eric Dong
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1020
The Microcode region indicated by MicrocodePatchAddress PCD may contain
more than one Microcode entry. We should save InCompleteCheckSum32 value
for each payload. Move the logic for calculate InCompleteCheckSum32 from
the outsize of the do-while loop to the inside.
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 | 37 ++++++++++++++++----------------
1 file changed, 19 insertions(+), 18 deletions(-)
diff --git a/UefiCpuPkg/Library/MpInitLib/Microcode.c b/UefiCpuPkg/Library/MpInitLib/Microcode.c
index e1f661d6b1..5f9ae22794 100644
--- a/UefiCpuPkg/Library/MpInitLib/Microcode.c
+++ b/UefiCpuPkg/Library/MpInitLib/Microcode.c
@@ -159,30 +159,31 @@ MicrocodeDetect (
MicrocodeEnd = (UINTN) (CpuMpData->MicrocodePatchAddress + CpuMpData->MicrocodePatchRegionSize);
MicrocodeEntryPoint = (CPU_MICROCODE_HEADER *) (UINTN) CpuMpData->MicrocodePatchAddress;
- //
- // Save an in-complete CheckSum32 from CheckSum Part1 for common parts.
- //
- if (MicrocodeEntryPoint->DataSize == 0) {
- InCompleteCheckSum32 = CalculateSum32 (
- (UINT32 *) MicrocodeEntryPoint,
- sizeof (CPU_MICROCODE_HEADER) + 2000
- );
- } else {
- InCompleteCheckSum32 = CalculateSum32 (
- (UINT32 *) MicrocodeEntryPoint,
- sizeof (CPU_MICROCODE_HEADER) + MicrocodeEntryPoint->DataSize
- );
- }
- InCompleteCheckSum32 -= MicrocodeEntryPoint->ProcessorSignature.Uint32;
- InCompleteCheckSum32 -= MicrocodeEntryPoint->ProcessorFlags;
- InCompleteCheckSum32 -= MicrocodeEntryPoint->Checksum;
-
do {
//
// Check if the microcode is for the Cpu and the version is newer
// and the update can be processed on the platform
//
CorrectMicrocode = FALSE;
+
+ //
+ // Save an in-complete CheckSum32 from CheckSum Part1 for common parts.
+ //
+ if (MicrocodeEntryPoint->DataSize == 0) {
+ InCompleteCheckSum32 = CalculateSum32 (
+ (UINT32 *) MicrocodeEntryPoint,
+ sizeof (CPU_MICROCODE_HEADER) + 2000
+ );
+ } else {
+ InCompleteCheckSum32 = CalculateSum32 (
+ (UINT32 *) MicrocodeEntryPoint,
+ sizeof (CPU_MICROCODE_HEADER) + MicrocodeEntryPoint->DataSize
+ );
+ }
+ 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
--
2.16.2.windows.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/2] UefiCpuPkg/Microcode: Add verification logic before parsing payload
2019-02-28 6:02 [PATCH 0/2] Fix Microcode failure issue Chen A Chen
2019-02-28 6:02 ` [PATCH 1/2] UefiCpuPkg/Microcode: Fix InComplete CheckSum32 issue Chen A Chen
@ 2019-02-28 6:02 ` Chen A Chen
2019-02-28 6:54 ` Ni, Ray
1 sibling, 1 reply; 5+ messages in thread
From: Chen A Chen @ 2019-02-28 6:02 UTC (permalink / raw)
To: edk2-devel; +Cc: Chen A Chen, Ray Ni, Eric Dong
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 (
+ IN CPU_MICROCODE_HEADER *MicrocodeHeader
+ )
+{
+ UINTN TotalSize;
+ UINTN DataSize;
+
+ if (MicrocodeHeader == NULL) {
+ return FALSE;
+ }
+
+ ///
+ /// 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;
+ }
+ 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)) {
+ return FALSE;
+ }
+ 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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] UefiCpuPkg/Microcode: Fix InComplete CheckSum32 issue
2019-02-28 6:02 ` [PATCH 1/2] UefiCpuPkg/Microcode: Fix InComplete CheckSum32 issue Chen A Chen
@ 2019-02-28 6:14 ` Ni, Ray
0 siblings, 0 replies; 5+ messages in thread
From: Ni, Ray @ 2019-02-28 6:14 UTC (permalink / raw)
To: Chen, Chen A, edk2-devel@lists.01.org; +Cc: Dong, Eric
Reviewed-by: Ray Ni <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 1/2] UefiCpuPkg/Microcode: Fix InComplete
> CheckSum32 issue
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1020
>
> The Microcode region indicated by MicrocodePatchAddress PCD may contain
> more than one Microcode entry. We should save InCompleteCheckSum32
> value for each payload. Move the logic for calculate InCompleteCheckSum32
> from the outsize of the do-while loop to the inside.
>
> 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 | 37 ++++++++++++++++---------
> -------
> 1 file changed, 19 insertions(+), 18 deletions(-)
>
> diff --git a/UefiCpuPkg/Library/MpInitLib/Microcode.c
> b/UefiCpuPkg/Library/MpInitLib/Microcode.c
> index e1f661d6b1..5f9ae22794 100644
> --- a/UefiCpuPkg/Library/MpInitLib/Microcode.c
> +++ b/UefiCpuPkg/Library/MpInitLib/Microcode.c
> @@ -159,30 +159,31 @@ MicrocodeDetect (
> MicrocodeEnd = (UINTN) (CpuMpData->MicrocodePatchAddress +
> CpuMpData->MicrocodePatchRegionSize);
> MicrocodeEntryPoint = (CPU_MICROCODE_HEADER *) (UINTN)
> CpuMpData->MicrocodePatchAddress;
>
> - //
> - // Save an in-complete CheckSum32 from CheckSum Part1 for common
> parts.
> - //
> - if (MicrocodeEntryPoint->DataSize == 0) {
> - InCompleteCheckSum32 = CalculateSum32 (
> - (UINT32 *) MicrocodeEntryPoint,
> - sizeof (CPU_MICROCODE_HEADER) + 2000
> - );
> - } else {
> - InCompleteCheckSum32 = CalculateSum32 (
> - (UINT32 *) MicrocodeEntryPoint,
> - sizeof (CPU_MICROCODE_HEADER) + MicrocodeEntryPoint-
> >DataSize
> - );
> - }
> - InCompleteCheckSum32 -= MicrocodeEntryPoint-
> >ProcessorSignature.Uint32;
> - InCompleteCheckSum32 -= MicrocodeEntryPoint->ProcessorFlags;
> - InCompleteCheckSum32 -= MicrocodeEntryPoint->Checksum;
> -
> do {
> //
> // Check if the microcode is for the Cpu and the version is newer
> // and the update can be processed on the platform
> //
> CorrectMicrocode = FALSE;
> +
> + //
> + // Save an in-complete CheckSum32 from CheckSum Part1 for common
> parts.
> + //
> + if (MicrocodeEntryPoint->DataSize == 0) {
> + InCompleteCheckSum32 = CalculateSum32 (
> + (UINT32 *) MicrocodeEntryPoint,
> + sizeof (CPU_MICROCODE_HEADER) + 2000
> + );
> + } else {
> + InCompleteCheckSum32 = CalculateSum32 (
> + (UINT32 *) MicrocodeEntryPoint,
> + sizeof (CPU_MICROCODE_HEADER) + MicrocodeEntryPoint-
> >DataSize
> + );
> + }
> + 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
> --
> 2.16.2.windows.1
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] UefiCpuPkg/Microcode: Add verification logic before parsing payload
2019-02-28 6:02 ` [PATCH 2/2] UefiCpuPkg/Microcode: Add verification logic before parsing payload Chen A Chen
@ 2019-02-28 6:54 ` Ni, Ray
0 siblings, 0 replies; 5+ messages in thread
From: Ni, Ray @ 2019-02-28 6:54 UTC (permalink / raw)
To: Chen, Chen A, edk2-devel@lists.01.org; +Cc: Dong, Eric
> -----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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-02-28 6:54 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-02-28 6:02 [PATCH 0/2] Fix Microcode failure issue Chen A Chen
2019-02-28 6:02 ` [PATCH 1/2] UefiCpuPkg/Microcode: Fix InComplete CheckSum32 issue Chen A Chen
2019-02-28 6:14 ` Ni, Ray
2019-02-28 6:02 ` [PATCH 2/2] UefiCpuPkg/Microcode: Add verification logic before parsing payload Chen A Chen
2019-02-28 6:54 ` Ni, Ray
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox