Re: [PATCH V1 1/1] OvmfPkg/AcpiPlatformDxe: Measure ACPI table from QEMU in TDVF

["Yao, Jiewen" <jiewen.yao@intel.com>]

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