Hi I have two feedback: 1) What we need measure is the input from VMM *before* any modification and *before* installation. Please don't use ACPI SDT protocol to get the table *after* modification.
2) Why not use TpmMeasureAndLogData() in https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Include/Library/TpmMeasurementLib.h ? Please don't use locate protocol. Thank you Yao, Jiewen > -----Original Message----- > From: Xu, Min M <min.m...@intel.com> > Sent: Monday, January 16, 2023 9:57 AM > To: devel@edk2.groups.io > Cc: Xu, Min M <min.m...@intel.com>; Aktas, Erdem > <erdemak...@google.com>; James Bottomley <j...@linux.ibm.com>; Yao, > Jiewen <jiewen....@intel.com>; Gerd Hoffmann <kra...@redhat.com>; > Tom Lendacky <thomas.lenda...@amd.com>; Michael Roth > <michael.r...@amd.com> > Subject: [PATCH V1 1/1] OvmfPkg/AcpiPlatformDxe: Measure ACPI table from > QEMU in TDVF > > From: Min M Xu <min.m...@intel.com> > > https://bugzilla.tianocore.org/show_bug.cgi?id=4245 > > The ACPI tables are downloaded from QEMU. QEMU/VMM is treated as > un-trusted in a td-guest. From the security perspective they should > be measured and extended if possible. So that they can be audited > later. The measurement protocol may be not installed. In this case > it still returns EFI_SUCCESS. > > Cc: Erdem Aktas <erdemak...@google.com> > Cc: James Bottomley <j...@linux.ibm.com> > Cc: Jiewen Yao <jiewen....@intel.com> > Cc: Gerd Hoffmann <kra...@redhat.com> > Cc: Tom Lendacky <thomas.lenda...@amd.com> > Cc: Michael Roth <michael.r...@amd.com> > Signed-off-by: Min Xu <min.m...@intel.com> > --- > OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf | 2 + > OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c | 168 > ++++++++++++++++++++ > 2 files changed, 170 insertions(+) > > diff --git a/OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf > b/OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf > index 8939dde42549..ae22bab38cf9 100644 > --- a/OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf > +++ b/OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf > @@ -51,6 +51,8 @@ > gEfiAcpiTableProtocolGuid # PROTOCOL ALWAYS_CONSUMED > gEfiPciIoProtocolGuid # PROTOCOL SOMETIMES_CONSUMED > gQemuAcpiTableNotifyProtocolGuid # PROTOCOL PRODUCES > + gEfiCcMeasurementProtocolGuid # PROTOCOL > SOMETIMES_CONSUMED > + gEfiAcpiSdtProtocolGuid # PROTOCOL > SOMETIMES_CONSUMED > > [Guids] > gRootBridgesConnectedEventGroupGuid > diff --git a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c > b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c > index f0d81d6fd73d..f442850c2e00 100644 > --- a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c > +++ b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c > @@ -18,6 +18,8 @@ > #include <Library/QemuFwCfgLib.h> // QemuFwCfgFindFile() > #include <Library/QemuFwCfgS3Lib.h> // QemuFwCfgS3Enabled() > #include <Library/UefiBootServicesTableLib.h> // gBS > +#include <Protocol/AcpiSystemDescriptionTable.h> > +#include <Protocol/CcMeasurement.h> > > #include "AcpiPlatform.h" > > @@ -812,12 +814,168 @@ UndoCmdWritePointer ( > )); > } > > +/** > + Mesure firmware ACPI table with CcMeasurement Protocol > + > + @param[in] CcProtocol Pointer to the CcMeasurment Protocol > + @param[in] EventData Pointer to the event data. > + @param[in] EventSize Size of event data. > + @param[in] CfgDataBase Configuration data base address. > + @param[in] EventSize Size of configuration data . > + @retval EFI_NOT_FOUND Cannot locate protocol. > + @retval EFI_OUT_OF_RESOURCES Allocate zero pool failure. > + @return Status codes returned by > + mTcg2Protocol->HashLogExtendEvent. > +**/ > +STATIC > +EFI_STATUS > +EFIAPI > +CcMeasureAcpiTable ( > + IN EFI_CC_MEASUREMENT_PROTOCOL *CcProtocol, > + IN CHAR8 *EventData, > + IN UINT32 EventSize, > + IN EFI_PHYSICAL_ADDRESS CfgDataBase, > + IN UINTN CfgDataLength > + ) > +{ > + EFI_STATUS Status; > + EFI_CC_EVENT *CcEvent; > + UINT32 MrIndex; > + > + if (CcProtocol == NULL) { > + return EFI_INVALID_PARAMETER; > + } > + > + Status = CcProtocol->MapPcrToMrIndex (CcProtocol, 1, &MrIndex); > + if (EFI_ERROR (Status)) { > + return EFI_INVALID_PARAMETER; > + } > + > + CcEvent = AllocateZeroPool (EventSize + sizeof (EFI_CC_EVENT) - sizeof > (CcEvent->Event)); > + if (CcEvent == NULL) { > + return EFI_OUT_OF_RESOURCES; > + } > + > + CcEvent->Size = EventSize + sizeof (EFI_CC_EVENT) - sizeof > (CcEvent->Event); > + CcEvent->Header.EventType = EV_PLATFORM_CONFIG_FLAGS; > + CcEvent->Header.MrIndex = MrIndex; > + CcEvent->Header.HeaderSize = sizeof (EFI_CC_EVENT_HEADER); > + CcEvent->Header.HeaderVersion = EFI_CC_EVENT_HEADER_VERSION; > + CopyMem (&CcEvent->Event[0], EventData, EventSize); > + > + Status = CcProtocol->HashLogExtendEvent ( > + CcProtocol, > + 0, > + CfgDataBase, > + CfgDataLength, > + CcEvent > + ); > + > + FreePool (CcEvent); > + > + return Status; > +} > + > // > // We'll be saving the keys of installed tables so that we can roll them back > // in case of failure. 128 tables should be enough for anyone (TM). > // > #define INSTALLED_TABLES_MAX 128 > > +/** > + * The ACPI tables are downloaded from QEMU. From the security > perspective these are > + * external inputs and should be measured and extended if possible. So that > they can > + * be audited later. The measurement protocol may be not installed. In this > case it > + * still returns EFI_SUCCESS. > + * > + * @param InstalledKey Pointer to an array which contains the keys of the > installed ACPI tables > + * @param Length Length of the array > + * @retval EFI_SUCCESS Successfully measure the ACPI tables > + * @retval Others Other errors as indicated > + */ > +STATIC > +EFI_STATUS > +MeasureInstalledTablesFromQemu ( > + IN UINTN *InstalledKey, > + IN INT32 Length > + ) > +{ > + EFI_STATUS Status; > + UINTN Index1; > + INT32 Index2; > + EFI_ACPI_SDT_HEADER *Table; > + EFI_ACPI_TABLE_VERSION Version; > + UINTN TableKey; > + EFI_ACPI_SDT_PROTOCOL *AcpiSdtProtocol; > + EFI_CC_MEASUREMENT_PROTOCOL *CcProtocol = NULL; > + > + Status = gBS->LocateProtocol (&gEfiCcMeasurementProtocolGuid, NULL, > (VOID **)&CcProtocol); > + if (EFI_ERROR (Status)) { > + // > + // CcMeasurement protocol is not installed. > + // > + return EFI_SUCCESS; > + } > + > + Status = gBS->LocateProtocol (&gEfiAcpiSdtProtocolGuid, NULL, (void > **)&AcpiSdtProtocol); > + if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_ERROR, "Unable to locate ACPI SDT protocol.\n")); > + return Status; > + } > + > + Index1 = 0; > + do { > + Status = AcpiSdtProtocol->GetAcpiTable (Index1, &Table, &Version, > &TableKey); > + if (EFI_ERROR (Status)) { > + if (Status == EFI_NOT_FOUND) { > + // > + // There is no more ACPI tables found. So we return with EFI_SUCCESS. > + // > + Status = EFI_SUCCESS; > + } > + > + break; > + } > + > + for (Index2 = 0; Index2 < Length; Index2++) { > + if (TableKey == InstalledKey[Index2]) { > + break; > + } > + } > + > + if (Index2 < Length) { > + Status = CcMeasureAcpiTable ( > + CcProtocol, > + (CHAR8 *)&Table->Signature, > + sizeof (Table->Signature), > + (EFI_PHYSICAL_ADDRESS)(UINTN)Table, > + Table->Length > + ); > + if (EFI_ERROR (Status)) { > + DEBUG (( > + DEBUG_ERROR, > + "Measure ACPI table [%-4.4a] with Size = 0x%x failed! Status = > %r\n", > + (CONST CHAR8 *)&Table->Signature, > + Table->Length, > + Status > + )); > + break; > + } else { > + DEBUG (( > + DEBUG_INFO, > + "Measure ACPI table [%-4.4a] with Size = 0x%x\n", > + (CONST CHAR8 *)&Table->Signature, > + Table->Length > + )); > + } > + } > + > + Index1++; > + } while (TRUE); > + > + return Status; > +} > + > /** > Process a QEMU_LOADER_ADD_POINTER command in order to see if its > target byte > array is an ACPI table, and if so, install it. > @@ -1247,6 +1405,16 @@ InstallQemuFwCfgTables ( > } > } > > + // > + // Measure the ACPI tables which are downloaded from QEMU > + // > + if (Installed > 0) { > + Status = MeasureInstalledTablesFromQemu (InstalledKey, Installed); > + if (EFI_ERROR (Status)) { > + goto UninstallAcpiTables; > + } > + } > + > // > // Install a protocol to notify that the ACPI table provided by Qemu is > // ready. > -- > 2.29.2.windows.2 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#98535): https://edk2.groups.io/g/devel/message/98535 Mute This Topic: https://groups.io/mt/96299277/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
