[edk2-devel] [PATCH 1/1] UefiCpuPkg: Extend measurement of microcode patches to TPM
Yang, Longlong
longlong.yang at intel.com
Fri Nov 5 03:26:29 UTC 2021
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 at intel.com>
Sent: Tuesday, November 2, 2021 11:30 AM
To: devel at edk2.groups.io; Ni, Ray <ray.ni at intel.com>; Yang, Longlong <longlong.yang at intel.com>
Cc: Dong, Eric <eric.dong at intel.com>; Kumar, Rahul1 <rahul1.kumar at intel.com>; Yao, Jiewen <jiewen.yao at intel.com>; Xu, Min M <min.m.xu at intel.com>; Zhang, Qi1 <qi1.zhang at 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 at edk2.groups.io <devel at edk2.groups.io> On Behalf Of Ni, Ray
Sent: Tuesday, November 2, 2021 9:55 AM
To: Yang, Longlong <longlong.yang at intel.com>; devel at edk2.groups.io
Cc: Dong, Eric <eric.dong at intel.com>; Kumar, Rahul1 <rahul1.kumar at intel.com>; Yao, Jiewen <jiewen.yao at intel.com>; Xu, Min M <min.m.xu at intel.com>; Zhang, Qi1 <qi1.zhang at 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 at intel.com>
Sent: Thursday, October 28, 2021 3:21 PM
To: devel at edk2.groups.io
Cc: Yang, Longlong <longlong.yang at intel.com>; Dong, Eric <eric.dong at intel.com>; Ni, Ray <ray.ni at intel.com>; Kumar, Rahul1 <rahul1.kumar at intel.com>; Yao, Jiewen <jiewen.yao at intel.com>; Xu, Min M <min.m.xu at intel.com>; Zhang, Qi1 <qi1.zhang at 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 at intel.com>
Cc: Ray Ni <ray.ni at intel.com>
Cc: Rahul Kumar <rahul1.kumar at intel.com>
Cc: Jiewen Yao <jiewen.yao at intel.com>
Cc: Min M Xu <min.m.xu at intel.com>
Cc: Qi Zhang <qi1.zhang at intel.com>
Signed-off-by: Longlong Yang <longlong.yang at 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.
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#83403): https://edk2.groups.io/g/devel/message/83403
Mute This Topic: https://groups.io/mt/86757841/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list