[edk2-devel] [PATCH V1 1/1] OvmfPkg/AcpiPlatformDxe: Measure ACPI table from QEMU in TDVF
Yao, Jiewen
jiewen.yao at intel.com
Mon Jan 16 02:42:03 UTC 2023
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.xu at intel.com>
> Sent: Monday, January 16, 2023 9:57 AM
> To: devel at edk2.groups.io
> Cc: Xu, Min M <min.m.xu at intel.com>; Aktas, Erdem
> <erdemaktas at google.com>; James Bottomley <jejb at linux.ibm.com>; Yao,
> Jiewen <jiewen.yao at intel.com>; Gerd Hoffmann <kraxel at redhat.com>;
> Tom Lendacky <thomas.lendacky at amd.com>; Michael Roth
> <michael.roth at amd.com>
> Subject: [PATCH V1 1/1] OvmfPkg/AcpiPlatformDxe: Measure ACPI table from
> QEMU in TDVF
>
> From: Min M Xu <min.m.xu at 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 <erdemaktas at google.com>
> Cc: James Bottomley <jejb at linux.ibm.com>
> Cc: Jiewen Yao <jiewen.yao at intel.com>
> Cc: Gerd Hoffmann <kraxel at redhat.com>
> Cc: Tom Lendacky <thomas.lendacky at amd.com>
> Cc: Michael Roth <michael.roth at amd.com>
> Signed-off-by: Min Xu <min.m.xu at 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/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