* [PATCH V1 1/1] OvmfPkg/AcpiPlatformDxe: Measure ACPI table from QEMU in TDVF
@ 2023-01-16 1:56 Min Xu
2023-01-16 2:42 ` Yao, Jiewen
0 siblings, 1 reply; 11+ messages in thread
From: Min Xu @ 2023-01-16 1:56 UTC (permalink / raw)
To: devel
Cc: Min M Xu, Erdem Aktas, James Bottomley, Jiewen Yao, Gerd Hoffmann,
Tom Lendacky, Michael Roth
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH V1 1/1] OvmfPkg/AcpiPlatformDxe: Measure ACPI table from QEMU in TDVF
2023-01-16 1:56 [PATCH V1 1/1] OvmfPkg/AcpiPlatformDxe: Measure ACPI table from QEMU in TDVF Min Xu
@ 2023-01-16 2:42 ` Yao, Jiewen
2023-01-16 10:38 ` [edk2-devel] " Ard Biesheuvel
0 siblings, 1 reply; 11+ messages in thread
From: Yao, Jiewen @ 2023-01-16 2:42 UTC (permalink / raw)
To: Xu, Min M, devel@edk2.groups.io
Cc: Aktas, Erdem, James Bottomley, Gerd Hoffmann, Tom Lendacky,
Michael Roth
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [PATCH V1 1/1] OvmfPkg/AcpiPlatformDxe: Measure ACPI table from QEMU in TDVF
2023-01-16 2:42 ` Yao, Jiewen
@ 2023-01-16 10:38 ` Ard Biesheuvel
2023-01-16 13:57 ` Yao, Jiewen
0 siblings, 1 reply; 11+ messages in thread
From: Ard Biesheuvel @ 2023-01-16 10:38 UTC (permalink / raw)
To: devel, jiewen.yao
Cc: Xu, Min M, Aktas, Erdem, James Bottomley, Gerd Hoffmann,
Tom Lendacky, Michael Roth
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/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
>
>
>
>
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [PATCH V1 1/1] OvmfPkg/AcpiPlatformDxe: Measure ACPI table from QEMU in TDVF
2023-01-16 10:38 ` [edk2-devel] " Ard Biesheuvel
@ 2023-01-16 13:57 ` Yao, Jiewen
2023-01-16 14:14 ` Ard Biesheuvel
0 siblings, 1 reply; 11+ messages in thread
From: Yao, Jiewen @ 2023-01-16 13:57 UTC (permalink / raw)
To: Ard Biesheuvel, devel@edk2.groups.io
Cc: Xu, Min M, Aktas, Erdem, James Bottomley, Gerd Hoffmann,
Tom Lendacky, Michael Roth
Do you mean real platform BIOS?
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.
Thank You
Yao, Jiewen
> -----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
> >
> >
> >
> >
> >
> >
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [PATCH V1 1/1] OvmfPkg/AcpiPlatformDxe: Measure ACPI table from QEMU in TDVF
2023-01-16 13:57 ` Yao, Jiewen
@ 2023-01-16 14:14 ` Ard Biesheuvel
2023-01-16 14:27 ` Yao, Jiewen
[not found] ` <173AD0557CFDE0A2.23170@groups.io>
0 siblings, 2 replies; 11+ messages in thread
From: Ard Biesheuvel @ 2023-01-16 14:14 UTC (permalink / raw)
To: devel, jiewen.yao
Cc: Xu, Min M, Aktas, Erdem, James Bottomley, Gerd Hoffmann,
Tom Lendacky, Michael Roth
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
> > >
> > >
> > >
> > >
> > >
> > >
>
>
>
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [PATCH V1 1/1] OvmfPkg/AcpiPlatformDxe: Measure ACPI table from QEMU in TDVF
2023-01-16 14:14 ` Ard Biesheuvel
@ 2023-01-16 14:27 ` Yao, Jiewen
[not found] ` <173AD0557CFDE0A2.23170@groups.io>
1 sibling, 0 replies; 11+ messages in thread
From: Yao, Jiewen @ 2023-01-16 14:27 UTC (permalink / raw)
To: devel@edk2.groups.io, ardb@kernel.org
Cc: Xu, Min M, Aktas, Erdem, James Bottomley, Gerd Hoffmann,
Tom Lendacky, Michael Roth
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
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> >
> >
> >
> >
> >
>
>
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [PATCH V1 1/1] OvmfPkg/AcpiPlatformDxe: Measure ACPI table from QEMU in TDVF
[not found] ` <173AD0557CFDE0A2.23170@groups.io>
@ 2023-01-17 0:30 ` Yao, Jiewen
2023-01-17 0:35 ` Min Xu
0 siblings, 1 reply; 11+ messages in thread
From: Yao, Jiewen @ 2023-01-17 0:30 UTC (permalink / raw)
To: devel@edk2.groups.io, Yao, Jiewen, ardb@kernel.org
Cc: Xu, Min M, Aktas, Erdem, James Bottomley, Gerd Hoffmann,
Tom Lendacky, Michael Roth
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
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > >
> > >
> > >
> > >
> > >
> >
> >
> >
> >
>
>
>
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [PATCH V1 1/1] OvmfPkg/AcpiPlatformDxe: Measure ACPI table from QEMU in TDVF
2023-01-17 0:30 ` Yao, Jiewen
@ 2023-01-17 0:35 ` Min Xu
2023-01-17 2:12 ` Yao, Jiewen
[not found] ` <173AF6CD0E3DD24A.23170@groups.io>
0 siblings, 2 replies; 11+ messages in thread
From: Min Xu @ 2023-01-17 0:35 UTC (permalink / raw)
To: Yao, Jiewen, devel@edk2.groups.io, ardb@kernel.org
Cc: Aktas, Erdem, James Bottomley, Gerd Hoffmann, Tom Lendacky,
Michael Roth
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
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > >
> > >
> > >
> > >
> >
> >
> >
> >
> >
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [PATCH V1 1/1] OvmfPkg/AcpiPlatformDxe: Measure ACPI table from QEMU in TDVF
2023-01-17 0:35 ` Min Xu
@ 2023-01-17 2:12 ` Yao, Jiewen
2023-01-17 3:46 ` Min Xu
[not found] ` <173AF6CD0E3DD24A.23170@groups.io>
1 sibling, 1 reply; 11+ messages in thread
From: Yao, Jiewen @ 2023-01-17 2:12 UTC (permalink / raw)
To: Xu, Min M, devel@edk2.groups.io, ardb@kernel.org
Cc: Aktas, Erdem, James Bottomley, Gerd Hoffmann, Tom Lendacky,
Michael Roth
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
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > >
> > > >
> > > >
> > > >
> > >
> > >
> > >
> > >
> > >
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [PATCH V1 1/1] OvmfPkg/AcpiPlatformDxe: Measure ACPI table from QEMU in TDVF
[not found] ` <173AF6CD0E3DD24A.23170@groups.io>
@ 2023-01-17 2:17 ` Yao, Jiewen
0 siblings, 0 replies; 11+ messages in thread
From: Yao, Jiewen @ 2023-01-17 2:17 UTC (permalink / raw)
To: devel@edk2.groups.io, Yao, Jiewen, Xu, Min M, ardb@kernel.org
Cc: Aktas, Erdem, James Bottomley, Gerd Hoffmann, Tom Lendacky,
Michael Roth
And we should still follow the rule to measure the *exact* input data from VMM. That is the table *before* modification and installation.
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Yao,
> Jiewen
> Sent: Tuesday, January 17, 2023 10:13 AM
> To: Xu, Min M <min.m.xu@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
>
> 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
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > >
> > > >
> > > >
> > > >
> > > >
>
>
>
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [PATCH V1 1/1] OvmfPkg/AcpiPlatformDxe: Measure ACPI table from QEMU in TDVF
2023-01-17 2:12 ` Yao, Jiewen
@ 2023-01-17 3:46 ` Min Xu
0 siblings, 0 replies; 11+ messages in thread
From: Min Xu @ 2023-01-17 3:46 UTC (permalink / raw)
To: Yao, Jiewen, devel@edk2.groups.io, ardb@kernel.org
Cc: Aktas, Erdem, James Bottomley, Gerd Hoffmann, Tom Lendacky,
Michael Roth
On January 17, 2023 10:13 AM, Yao Jiewen wrote:
> 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.
>
Thanks for clarification. So the measurement of the ACPI table from QEMU looks like below, right?
Status = TpmMeasureAndLogData (
1,
EV_PLATFORM_CONFIG_FLAGS, <-- event type
EV_POSTCODE_INFO_ACPI_DATA, <-- "ACPI DATA"
ACPI_DATA_LEN,
(VOID *)(UINTN)Table,
TableSize
);
Thanks
Min
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2023-01-17 3:46 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-01-16 1:56 [PATCH V1 1/1] OvmfPkg/AcpiPlatformDxe: Measure ACPI table from QEMU in TDVF Min Xu
2023-01-16 2:42 ` Yao, Jiewen
2023-01-16 10:38 ` [edk2-devel] " Ard Biesheuvel
2023-01-16 13:57 ` Yao, Jiewen
2023-01-16 14:14 ` Ard Biesheuvel
2023-01-16 14:27 ` Yao, Jiewen
[not found] ` <173AD0557CFDE0A2.23170@groups.io>
2023-01-17 0:30 ` Yao, Jiewen
2023-01-17 0:35 ` Min Xu
2023-01-17 2:12 ` Yao, Jiewen
2023-01-17 3:46 ` Min Xu
[not found] ` <173AF6CD0E3DD24A.23170@groups.io>
2023-01-17 2:17 ` Yao, Jiewen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox