From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by mx.groups.io with SMTP id smtpd.web11.163597.1673865503970300914 for ; Mon, 16 Jan 2023 02:38:24 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=JnNEKuOW; spf=pass (domain: kernel.org, ip: 139.178.84.217, mailfrom: ardb@kernel.org) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 4000760F5D for ; Mon, 16 Jan 2023 10:38:23 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 2713FC43396 for ; Mon, 16 Jan 2023 10:38:22 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1673865502; bh=voljtepE5w23+LP7kmgtxyze5rs4Qh7eH1BgfQ12XgM=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=JnNEKuOWuEMEvANEPwXjSLOh1yG6c3k/11bMnEKegxjZbpjERPu0GUjKmovQVzDbG uoRUrnIdG4AMgZrU2ZsbVfQjlkLsDFx+TY1HmU/wqnfTs6DN7CwqfVhxtCCryI8zJP 0UhS5ao/MqUPVvH2h+K3E4KJaHLijNeivyHVfwWoFcfxAur5EY1/uh/HYzFI+DU0ef FemdDcQ5cnPfTonthAN4EkW+tbq7RpF8KH8nmiA/D8HYV0XAoTMQNBTDRNyvF/YPND iOAT30WpXBAzpxwnq1snU/wXNR7yZX8lNUzVbCjg40AFqP9mYIuoctbi3J8aoOyCpa +kzSX7NU+TMqQ== Received: by mail-lf1-f53.google.com with SMTP id j17so42137701lfr.3 for ; Mon, 16 Jan 2023 02:38:22 -0800 (PST) X-Gm-Message-State: AFqh2kqugWM9No3oNmu6yXFMk5gn3FmSg2F0IY3k5Rfcrm0awjHjwh2q 8u1t2b2/sNSKVYgZYCiXckRVRLOB9HofrrI3KyA= X-Google-Smtp-Source: AMrXdXukD3o8LiKktFrHnZgqUc9xBZ2qImtFLy1e6DpjSB9m7yOaREzzciwF/vD54wlT4uy02HyBSraJR6lzZjsFWhE= X-Received: by 2002:a05:6512:3d93:b0:4b8:9001:a694 with SMTP id k19-20020a0565123d9300b004b89001a694mr4301595lfv.426.1673865499995; Mon, 16 Jan 2023 02:38:19 -0800 (PST) MIME-Version: 1.0 References: <20230116015658.194-1-min.m.xu@intel.com> In-Reply-To: From: "Ard Biesheuvel" Date: Mon, 16 Jan 2023 11:38:08 +0100 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [edk2-devel] [PATCH V1 1/1] OvmfPkg/AcpiPlatformDxe: Measure ACPI table from QEMU in TDVF To: devel@edk2.groups.io, jiewen.yao@intel.com Cc: "Xu, Min M" , "Aktas, Erdem" , James Bottomley , Gerd Hoffmann , Tom Lendacky , Michael Roth Content-Type: text/plain; charset="UTF-8" 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 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 > > Sent: Monday, January 16, 2023 9:57 AM > > To: devel@edk2.groups.io > > Cc: Xu, Min M ; Aktas, Erdem > > ; James Bottomley ; Yao, > > Jiewen ; Gerd Hoffmann ; > > Tom Lendacky ; Michael Roth > > > > Subject: [PATCH V1 1/1] OvmfPkg/AcpiPlatformDxe: Measure ACPI table from > > QEMU in TDVF > > > > From: Min M Xu > > > > 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 > > Cc: James Bottomley > > Cc: Jiewen Yao > > Cc: Gerd Hoffmann > > Cc: Tom Lendacky > > Cc: Michael Roth > > Signed-off-by: Min Xu > > --- > > 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 // QemuFwCfgFindFile() > > #include // QemuFwCfgS3Enabled() > > #include // gBS > > +#include > > +#include > > > > #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 > > > > > >