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

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

Sorry, I would like to withdraw my comment on ACPI table event and event type.

PFP spec defines the ACPI table on flash be measured to PCR[0] as part of POST_CODE.

But here ACPI table is configuration from VMM. I feel it is more suitable to extend to PCR[1] as part of configuration data.

I apologize for the confusing.

Thank you
Yao, Jiewen

> -----Original Message-----
> From: Xu, Min M <min.m.xu@intel.com>
> Sent: Tuesday, January 17, 2023 8:36 AM
> To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io;
> ardb@kernel.org
> Cc: Aktas, Erdem <erdemaktas@google.com>; James Bottomley
> <jejb@linux.ibm.com>; Gerd Hoffmann <kraxel@redhat.com>; Tom
> Lendacky <thomas.lendacky@amd.com>; Michael Roth
> <michael.roth@amd.com>
> Subject: RE: [edk2-devel] [PATCH V1 1/1] OvmfPkg/AcpiPlatformDxe:
> Measure ACPI table from QEMU in TDVF
> 
> Thanks Jiewen. It will be updated in the next version.
> 
> > -----Original Message-----
> > From: Yao, Jiewen <jiewen.yao@intel.com>
> > Sent: Tuesday, January 17, 2023 8:30 AM
> > To: devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@intel.com>;
> > ardb@kernel.org
> > Cc: Xu, Min M <min.m.xu@intel.com>; Aktas, Erdem
> > <erdemaktas@google.com>; James Bottomley <jejb@linux.ibm.com>; Gerd
> > Hoffmann <kraxel@redhat.com>; Tom Lendacky
> > <thomas.lendacky@amd.com>; Michael Roth <michael.roth@amd.com>
> > Subject: RE: [edk2-devel] [PATCH V1 1/1] OvmfPkg/AcpiPlatformDxe:
> Measure
> > ACPI table from QEMU in TDVF
> >
> > Hi Min
> > One more thing: This is to measure ACPI table.
> >
> > According to https://trustedcomputinggroup.org/resource/pc-client-
> specific-
> > platform-firmware-profile-specification/, the event type should be
> > EV_POST_CODE, and event data should be "ACPI DATA" in all caps. You can
> > use below pattern:
> > =================
> >   TpmMeasureAndLogData (
> >     0,
> >     EV_POST_CODE,
> >     EV_POSTCODE_INFO_ACPI_DATA,
> >     ACPI_DATA_LEN,
> >     Table,
> >     TableSize
> >     );
> > =================
> >
> > > -----Original Message-----
> > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Yao,
> > > Jiewen
> > > Sent: Monday, January 16, 2023 10:28 PM
> > > To: devel@edk2.groups.io; ardb@kernel.org
> > > Cc: Xu, Min M <min.m.xu@intel.com>; Aktas, Erdem
> > > <erdemaktas@google.com>; James Bottomley <jejb@linux.ibm.com>;
> Gerd
> > > Hoffmann <kraxel@redhat.com>; Tom Lendacky
> > <thomas.lendacky@amd.com>;
> > > Michael Roth <michael.roth@amd.com>
> > > Subject: Re: [edk2-devel] [PATCH V1 1/1] OvmfPkg/AcpiPlatformDxe:
> > > Measure ACPI table from QEMU in TDVF
> > >
> > > Each module, which wants to load ACPI table.
> > >
> > > Most codes are from close source platform.
> > >
> > > One example in EDKII SecurityPkg is TPM2.
> > >
> https://github.com/tianocore/edk2/blob/master/SecurityPkg/Tcg/Tcg2Acpi
> > > /
> > > Tcg2Acpi.c#L690
> > >
> > >
> https://github.com/tianocore/edk2/blob/master/SecurityPkg/Tcg/Tcg2Acpi
> > > /
> > > Tcg2Acpi.c#L794
> > >
> > > Just search "TpmMeasureAndLogData" ...
> > >
> > >
> > > > -----Original Message-----
> > > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ard
> > > > Biesheuvel
> > > > Sent: Monday, January 16, 2023 10:15 PM
> > > > To: devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@intel.com>
> > > > Cc: Xu, Min M <min.m.xu@intel.com>; Aktas, Erdem
> > > > <erdemaktas@google.com>; James Bottomley <jejb@linux.ibm.com>;
> > Gerd
> > > > Hoffmann <kraxel@redhat.com>; Tom Lendacky
> > > > <thomas.lendacky@amd.com>; Michael Roth <michael.roth@amd.com>
> > > > Subject: Re: [edk2-devel] [PATCH V1 1/1] OvmfPkg/AcpiPlatformDxe:
> > > > Measure ACPI table from QEMU in TDVF
> > > >
> > > > On Mon, 16 Jan 2023 at 14:57, Yao, Jiewen <jiewen.yao@intel.com>
> > wrote:
> > > > >
> > > > > Do you mean real platform BIOS?
> > > > >
> > > >
> > > > Yes
> > > >
> > > > > The rules are:
> > > > > 1) The ACPI table on the flash loaded by the BIOS is measured
> > > > > *before*
> > > any
> > > > modification and *before* installation. For example, DSDT, SSDT.
> > > > > 2) The ACPI table constructed by the BIOS dynamically does not
> > > > > need to
> > > be
> > > > measured. For example MADT.
> > > > >
> > > >
> > > > OK so which EDK2 component is normally in charge of this?
> > > >
> > > >
> > > >
> > > > > > -----Original Message-----
> > > > > > From: Ard Biesheuvel <ardb@kernel.org>
> > > > > > Sent: Monday, January 16, 2023 6:38 PM
> > > > > > To: devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@intel.com>
> > > > > > Cc: Xu, Min M <min.m.xu@intel.com>; Aktas, Erdem
> > > > > > <erdemaktas@google.com>; James Bottomley
> <jejb@linux.ibm.com>;
> > > > Gerd
> > > > > > Hoffmann <kraxel@redhat.com>; Tom Lendacky
> > > > > > <thomas.lendacky@amd.com>; Michael Roth
> > <michael.roth@amd.com>
> > > > > > Subject: Re: [edk2-devel] [PATCH V1 1/1] OvmfPkg/AcpiPlatformDxe:
> > > > > > Measure ACPI table from QEMU in TDVF
> > > > > >
> > > > > > Another question: how does this deviate from ordinary measured
> boot?
> > > > > > How and when are ACPI tables measured on such systems?
> > > > > >
> > > > > >
> > > > > > On Mon, 16 Jan 2023 at 03:42, Yao, Jiewen <jiewen.yao@intel.com>
> > > > wrote:
> > > > > > >
> > > > > > > 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/Li
> > > > > > brary/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
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > >
> > > >
> > > >
> > > >
> > >
> > >
> > >
> > > 
> > >