Re: [edk2-devel] [RFC PATCH] OvmfPkg/SecurityPkg: Add build option for coexistance of vTPM and RTMR.

["Dionna Glaze via groups.io" <dionnaglaze=google.com@groups.io>]

On Thu, Mar 21, 2024 at 9:59 AM qinkun Bao <qinkun@google.com> wrote:
>
> From: Qinkun Bao <qinkun@google.com>
>
> The UEFI v2.10 spec defines the protocol EFI_CC_MEASUREMENT_PROTOCOL
> to enable (for example) RTMR-based boot measurement for TDX VMs.
> With the current UEFI spec’s “should not” wording and EDK2
> implementation, TPM measurement in TDVF is disabled when
> RTMR measurement is enabled.
>
> Mutual exclusion of the CC measurement protocol and TCG measurement
> protocol breaks backwards compatibility, which makes adoption of RTMRs
> challenging. A virtualized TPM device (vTPM) managed by the host VMM
> makes boot measurements visible to the VMM operator, but this is an
> oft-requested feature that users can choose to accept.
>
> The TPM has been a standard for over a decade and many existing
> applications rely on the TPM. Both inside and outside Google,
> we have many users that require vTPM, including features that are
> not easily available via RTMRs (e.g. sealing using keys that the
> guest OS cannot access).
>
> This patch adds a non-default build option to allow the coexistence
> of both the CC measurement and TCG protocols. Not included is a
> vendor-specific measured event in the CC event log that indicates
> whether a vTPM is attached or not.

Thank you very much for starting this conversation. I'll carry forward
more context from our more senior engineers.

'
Not measuring into all possible measurement banks led to
(CVE-2021-42299) with TPM PCR banks. Let's not repeat this problem.
Requiring a monumental shift of all attestation-based applications to
CC_MEASUREMENT_PROTOCOL and CEL in one step is going to make the
technology very difficult to adopt.
'

The combination of these requirements means careful rollout of
attestation verification policies to match the updated behavior of the
firmware.
The measured boot components have to be known to correctly measure
into all available measurement protocols and register banks.
The firmware has to be known specifically which protocols it makes available.

Now, how this is done is a vendor matter for now. If there is strong
demand for making vTPM attachment status known for folks who really
don't want a TPM and don't trust the VMM to not attach one anyway,
we'll need to agree that the CEL should have an entry for an RTMR0
update detailing the combination of measurement protocols in use.
Likewise PCR1 should have an event detailing the protocols in use if
we want to make CC_MEASUREMENT_PROTOCOL usage configurable.

Philosophizing aside that both protocols should be allowed to be
active and that the spec should be updated to say something along the
lines of "all measurement protocols that are active (i.e., any
combination of EFI_CC_MEASUREMENT_PROTOCOL, EFI_TCG_PROTOCOL,
EFI_TCG2_PROTOCOL) should have comparable events measured if any one
protocol receives measurements. All measurement protocols that are
active MUST measure comparable separator events if any protocol
receives a separator event." This is crossing a spec boundary between
USWG and TCG, so I don't know where specifically the language needs to
go, but we need some language that implementers can use as
justification for measuring into any found protocol and not just at
most one.

It's not lost on me that it is similarly difficult to say that all
protocols that are discoverable need to have comparable events
measured into them since that _also_ introduces an all-or-nothing
migration problem, but at least that's more controllable from the
attestation verification policy side than from the UEFI spec side.
We're not adding new measurement protocols frequently, so we can get
the entire boot chain ready for a new measurement protocol before it's
made discoverable.

>
> 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: Qinkun Bao <qinkun@google.com>
> ---
>  OvmfPkg/OvmfPkgX64.dsc                               |  9 ++++++++-
>  .../DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.c    | 12 +++++++++++-
>  .../DxeTpmMeasurementLib/DxeTpmMeasurementLib.c      |  6 ++++++
>  3 files changed, 25 insertions(+), 2 deletions(-)
>
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index 56c920168d..9bcee45047 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -32,7 +32,8 @@
>    DEFINE SECURE_BOOT_ENABLE      = FALSE
>    DEFINE SMM_REQUIRE             = FALSE
>    DEFINE SOURCE_DEBUG_ENABLE     = FALSE
> -  DEFINE CC_MEASUREMENT_ENABLE   = FALSE
> +  DEFINE CC_MEASUREMENT_ENABLE   = TRUE
> +  DEFINE CC_MEASUREMENT_AND_TCG2_COEXIST  = FASLE
>
>  !include OvmfPkg/Include/Dsc/OvmfTpmDefines.dsc.inc
>
> @@ -99,6 +100,11 @@
>    INTEL:*_*_X64_GENFW_FLAGS = --keepexceptiontable
>  !endif
>    RELEASE_*_*_GENFW_FLAGS = --zero
> +!if $(CC_MEASUREMENT_ENABLE) == TRUE && $(CC_MEASUREMENT_AND_TCG2_COEXIST) == TRUE
> +  MSFT:*_*_*_CC_FLAGS = /D CC_MEASUREMENT_AND_TCG2_COEXIST_FEATURE
> +  INTEL:*_*_*_CC_FLAGS = /D CC_MEASUREMENT_AND_TCG2_COEXIST_FEATURE
> +  GCC:*_*_*_CC_FLAGS = -D CC_MEASUREMENT_AND_TCG2_COEXIST_FEATURE
> +!endif
>
>    #
>    # Disable deprecated APIs.
> @@ -1045,6 +1051,7 @@
>    }
>  !endif
>
> +
>    #
>    # TPM support
>    #
> diff --git a/SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.c b/SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.c
> index 73719f3b96..4c9bc8ab4a 100644
> --- a/SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.c
> +++ b/SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.c
> @@ -325,7 +325,12 @@ Tcg2MeasureGptTable (
>      }
>
>      DEBUG ((DEBUG_INFO, "DxeTpm2MeasureBootHandler - Cc MeasureGptTable - %r\n", Status));
> +#ifdef CC_MEASUREMENT_AND_TCG2_COEXIST_FEATURE

On to the less philosophical. Thank you for starting this
conversation. I do think that this implementation is unideal. These
kinds of configuration decisions should be implemented in the form of
a Pcd. Whether that's a static or dynamic Pcd is a matter of whether
you want to do all the event design of the configuration measurement
to MR0~PCR1 at the same time.

> +  }
> +  if (Tcg2Protocol != NULL) {
> +#else
>    } else if (Tcg2Protocol != NULL) {
> +#endif
>      //
>      // If Tcg2Protocol is installed, then Measure GPT data with this protocol.
>      //
> @@ -493,7 +498,12 @@ Tcg2MeasurePeImage (
>                             CcEvent
>                             );
>      DEBUG ((DEBUG_INFO, "DxeTpm2MeasureBootHandler - Cc MeasurePeImage - %r\n", Status));
> -  } else if (Tcg2Protocol != NULL) {
> +#ifdef CC_MEASUREMENT_AND_TCG2_COEXIST_FEATURE
> +   }
> +   if (Tcg2Protocol != NULL) {
> +#else
> +   } else if (Tcg2Protocol != NULL) {
> +#endif
>      Status = Tcg2Protocol->HashLogExtendEvent (
>                               Tcg2Protocol,
>                               PE_COFF_IMAGE,
> diff --git a/SecurityPkg/Library/DxeTpmMeasurementLib/DxeTpmMeasurementLib.c b/SecurityPkg/Library/DxeTpmMeasurementLib/DxeTpmMeasurementLib.c
> index 6f287b31fc..b1c6198b4b 100644
> --- a/SecurityPkg/Library/DxeTpmMeasurementLib/DxeTpmMeasurementLib.c
> +++ b/SecurityPkg/Library/DxeTpmMeasurementLib/DxeTpmMeasurementLib.c
> @@ -261,7 +261,11 @@ TpmMeasureAndLogData (
>                 HashData,
>                 HashDataLen
>                 );
> +#ifdef CC_MEASUREMENT_AND_TCG2_COEXIST_FEATURE
> +  }
> +#else
>    } else {
> +#endif
>      //
>      // Try to measure using Tpm20 protocol
>      //
> @@ -287,7 +291,9 @@ TpmMeasureAndLogData (
>                   HashDataLen
>                   );
>      }
> +#ifndef CC_MEASUREMENT_AND_TCG2_COEXIST_FEATURE
>    }
> +#endif
>
>    return Status;
>  }
> --
> 2.44.0.291.gc1ea87d7ee-goog
>


--
-Dionna Glaze, PhD (she/her)


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117017): https://edk2.groups.io/g/devel/message/117017
Mute This Topic: https://groups.io/mt/105070442/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-