Re: [edk2-devel] [PATCH 1/1] UefiCpuPkg: Extend measurement of microcode patches to TPM

["Yang, Longlong" <longlong.yang@intel.com>]

Thank you Ray for your kind and patient feedbacks and advices.
I checked all 10 comments one by one and you could see my responses inline in below code change.
I am testing new patch, will send to community soon if all tests pass.

BRs
Longlong

-----Original Message-----
From: Ni, Ray <ray.ni@intel.com> 
Sent: Tuesday, November 2, 2021 11:30 AM
To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; Yang, Longlong <longlong.yang@intel.com>
Cc: Dong, Eric <eric.dong@intel.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Xu, Min M <min.m.xu@intel.com>; Zhang, Qi1 <qi1.zhang@intel.com>
Subject: RE: [edk2-devel] [PATCH 1/1] UefiCpuPkg: Extend measurement of microcode patches to TPM

Just offline discussed with Longlong, measuring the entire microcode buffer might spend more time comparing to only measuring the applied microcode, when the platform firmware includes lots of microcode.

10 comments embedded in code change in below.

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ni, Ray
Sent: Tuesday, November 2, 2021 9:55 AM
To: Yang, Longlong <longlong.yang@intel.com>; devel@edk2.groups.io
Cc: Dong, Eric <eric.dong@intel.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Xu, Min M <min.m.xu@intel.com>; Zhang, Qi1 <qi1.zhang@intel.com>
Subject: Re: [edk2-devel] [PATCH 1/1] UefiCpuPkg: Extend measurement of microcode patches to TPM

Longlong,
Your code creates a big buffer that holds microcode data for all threads.
	MicrocodeCpu[i] = MicrocodePatchHob->MicrocodePatchAddress + MicrocodePatchHob->ProcessorSpecificPatchOffset[i]
	BigBuffer = GetMicrocodeBuffer (MicrocodeOfCpu[0]) + GetMicrocodeBuffer (MicrocodeOfCpu[1]) + ...
	HashValue = Hash (BigBuffer)

I am not sure if we can do like below:
	BigBuffer = <Entire microcode buffer pointed by MicrocodePatchHob->MicrocodePatchAddress> + <array content of MicrocodePatchHob->ProcessorSpecificPatchOffset[]>
	HashValue = Hash (BigBuffer)

The second approach doesn't require sorting, one-by-one-copying.

Thanks,
Ray

-----Original Message-----
From: Yang, Longlong <longlong.yang@intel.com>
Sent: Thursday, October 28, 2021 3:21 PM
To: devel@edk2.groups.io
Cc: Yang, Longlong <longlong.yang@intel.com>; Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Xu, Min M <min.m.xu@intel.com>; Zhang, Qi1 <qi1.zhang@intel.com>
Subject: [PATCH 1/1] UefiCpuPkg: Extend measurement of microcode patches to TPM

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3683

TCG specification says BIOS should extend measurement of microcode to TPM.
However, reference BIOS is not doing this. This patch consumes gEdkiiMicrocodePatchHobGuid to checkout all applied microcode patches, then all applied microcode patches are packed in order to form a single binary blob which is measured with event type EV_CPU_MICROCODE to PCR[1] in TPM.

Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Min M Xu <min.m.xu@intel.com>
Cc: Qi Zhang <qi1.zhang@intel.com>
Signed-off-by: Longlong Yang <longlong.yang@intel.com>
---
 .../MicrocodeMeasurementDxe.c                 | 254 ++++++++++++++++++
 .../MicrocodeMeasurementDxe.inf               |  58 ++++
 .../MicrocodeMeasurementDxe.uni               |  15 ++
 .../MicrocodeMeasurementDxeExtra.uni          |  12 +
 UefiCpuPkg/UefiCpuPkg.dsc                     |   2 +
 5 files changed, 341 insertions(+)
 create mode 100644 UefiCpuPkg/MicrocodeMeasurementDxe/MicrocodeMeasurementDxe.c
 create mode 100644 UefiCpuPkg/MicrocodeMeasurementDxe/MicrocodeMeasurementDxe.inf
 create mode 100644 UefiCpuPkg/MicrocodeMeasurementDxe/MicrocodeMeasurementDxe.uni
 create mode 100644 UefiCpuPkg/MicrocodeMeasurementDxe/MicrocodeMeasurementDxeExtra.uni

diff --git a/UefiCpuPkg/MicrocodeMeasurementDxe/MicrocodeMeasurementDxe.c b/UefiCpuPkg/MicrocodeMeasurementDxe/MicrocodeMeasurementDxe.c
new file mode 100644
index 000000000000..1898a2bff023
--- /dev/null
+++ b/UefiCpuPkg/MicrocodeMeasurementDxe/MicrocodeMeasurementDxe.c
@@ -0,0 +1,254 @@
+/** @file
+
+
+  if (TRUE == mMicrocodeMeasured) {

1. Remove "TRUE == " please
[longlong] The mMicrocodeMeasured flag and this check are removed in new implementation.


2. Can you please duplicate the MicrocodePatchHob->ProcessorSpecificPatchOffset in a new array and sort the "PatchOffset" before calculating the total microcode size?
     This avoids big memory consumption in many-core platforms.
[longlong] Fixed in new implementation.


+
+  //
+  // Extract all microcode patches to a list from MicrocodePatchHob  // 
+ MicrocodePatchesList = AllocatePool (MicrocodePatchHob->ProcessorCount
+ * sizeof (MICROCODE_PATCH_TYPE));  if (NULL == MicrocodePatchesList) {
+    DEBUG ((DEBUG_ERROR, "ERROR: AllocatePool to MicrocodePatchesList Failed!\n"));
+    return;
+  }
+  for (Index = 0; Index < MicrocodePatchHob->ProcessorCount; Index++) {
+    if (MAX_UINT64 == MicrocodePatchHob->ProcessorSpecificPatchOffset[Index]) {
+      //
+      // If no microcode patch was found in a slot, set the address of the microcode patch
+      // in that slot to MAX_UINTN, and the size to 0, thus indicates no patch in that slot.
+      //
+      MicrocodePatchesList[Index].Address = MAX_UINTN;
+      MicrocodePatchesList[Index].Size    = 0;
+
+      DEBUG ((DEBUG_INFO, "INFO: Processor#%d: detected no microcode patch\n", Index));
+    } else {
+      MicrocodePatchesList[Index].Address     = (UINTN)(MicrocodePatchHob->MicrocodePatchAddress + MicrocodePatchHob->ProcessorSpecificPatchOffset[Index]);
+      MicrocodePatchesList[Index].Size        = ((CPU_MICROCODE_HEADER*)((UINTN)(MicrocodePatchHob->MicrocodePatchAddress + MicrocodePatchHob->ProcessorSpecificPatchOffset[Index])))->TotalSize;

3. Can you please use GetMicrocodeLength() from MicrocodeLib?
[longlong] Fixed in new implementation.


+  PerformQuickSort (
+               MicrocodePatchesList,
+               MicrocodePatchHob->ProcessorCount,
+               sizeof (MICROCODE_PATCH_TYPE),
+               MicrocodePatchesListSortFunction
+               );


4. Can you please use QuickSort() in BaseLib? This avoids UefiCpuPkg depends on MdeModulePkg.
[longlong] Fixed in new implementation.


+  for (Index = 0; Index < MicrocodePatchHob->ProcessorCount; Index++) {
+    DEBUG ((DEBUG_INFO, "INFO: After sorting: Processor#%d: Microcode 
+ patch address: 0x%x, size: 0x%x\n", Index, 
+ MicrocodePatchesList[Index].Address,
+ MicrocodePatchesList[Index].Size));
+  }

5. There are lots of debug messages in this module. Please review them and think about what are necessary. Try to remove some unnecessary messages.
[longlong] Checked and removed some unnecessary messages in new implementation.


+  //
+  // LastPackedMicrocodeAddress is used to skip duplicate microcode patch.

6. You might need a "LastPatchOffset" to skip duplicate the PatchOffset after sorting.
[longlong] Advice accepted. "LastPatchOffset" is used to skip duplicate the PatchOffset after sorting in new implementation.

+
+  if (0 == MicrocodePatchesBlobSize) {
+    DEBUG ((DEBUG_INFO, "INFO: No microcode patch was ever applied!"));
+    FreePool (MicrocodePatchesList);
+    FreePool (MicrocodePatchesBlob);
+    return;
+  }

7. Please confirm with Jiewen or Qi whether no measurement is fine if there is no microcode.
[longlong] Confirmed with Qi, he prefers no measurement if there is no microcode.

+
+  Status = TpmMeasureAndLogData (
+               PCRIndex,                  // PCRIndex
+               EventType,                 // EventType
+               &EventLog,                 // EventLog
+               EventLogSize,              // LogLen
+               MicrocodePatchesBlob,      // HashData
+               MicrocodePatchesBlobSize   // HashDataLen
+               );
+  if (!EFI_ERROR (Status)) {
+    mMicrocodeMeasured = TRUE;
+    gBS->CloseEvent (Event);

8. I think if you CloseEvent() there is no need to use mMicrocodeMeasured flag because the event won't be signaled again.
[longlong] The mMicrocodeMeasured flag is removed in new implementation.

+
+#
+# The following information is for reference only and not required by the build tools.
+#
+#  VALID_ARCHITECTURES           = IA32 X64 EBC ARM AARCH64

9. Can you just list "IA32" and "X64"? The microcode HOB doesn't apply to ARM. EBC can be added to the supported list if we verified it works.
[longlong] After following a template to create the inf file, I ignored this comment, Sorry for that. Will check other comments as well and fix it in new implementation.

   VmgExitLib|UefiCpuPkg/Library/VmgExitLibNull/VmgExitLibNull.inf
   MicrocodeLib|UefiCpuPkg/Library/MicrocodeLib/MicrocodeLib.inf
+  SortLib|MdeModulePkg/Library/UefiSortLib/UefiSortLib.inf

10. No need the above SortLib.
[longlong] SortLib is deleted in new implementation.