From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by mx.groups.io with SMTP id smtpd.web12.7736.1619173608539559164 for ; Fri, 23 Apr 2021 03:26:48 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=Uznj6QuV; spf=pass (domain: redhat.com, ip: 216.205.24.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1619173607; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=smo7M+CduS3dsGhcebRFoW8jqcmif0H3bS4K6yNoMPE=; b=Uznj6QuVx53znHiYEoiLQ7cHki3x5e8eurzrHc0rd29Npqjegubk9E1zcX9ytMBaGOf273 Od0rg9wgZ90ukcut+HkPfOvSr2QO+R4Mym2Ah/utG+JIb+dx/zTYBS2sF7imqDn7PoGlZz VRI/92liKOkBdRZ8oOx1ifDnCu7ck2Y= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-83-aT1fFm9CORqXsXNCPeh_5w-1; Fri, 23 Apr 2021 06:26:43 -0400 X-MC-Unique: aT1fFm9CORqXsXNCPeh_5w-1 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id EEA0F343A2; Fri, 23 Apr 2021 10:26:41 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-115-2.ams2.redhat.com [10.36.115.2]) by smtp.corp.redhat.com (Postfix) with ESMTP id 7F4A65DDAD; Fri, 23 Apr 2021 10:26:39 +0000 (UTC) Subject: Re: [PATCH 3/3] OvmfPkg/PlatformPei: Mark TPM MMIO range as unencrypted for SEV To: Tom Lendacky Cc: devel@edk2.groups.io, Joerg Roedel , Borislav Petkov , Ard Biesheuvel , Jordan Justen , Brijesh Singh , James Bottomley , Jiewen Yao , Min Xu References: <1f64ca5689ec86c427e4db8c41da598896dca4ba.1618959281.git.thomas.lendacky@amd.com> From: "Laszlo Ersek" Message-ID: <8417023b-b3d6-5268-e92a-0c6f5ebfff6b@redhat.com> Date: Fri, 23 Apr 2021 12:26:38 +0200 MIME-Version: 1.0 In-Reply-To: <1f64ca5689ec86c427e4db8c41da598896dca4ba.1618959281.git.thomas.lendacky@amd.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit review#2 from scratch: On 04/21/21 00:54, Tom Lendacky wrote: > From: Tom Lendacky > > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3345 > > The TPM support in OVMF performs MMIO accesses during the PEI phase. At > this point, MMIO ranges have not been marked un-encyrpted, so an SEV-ES > guest will fail attempting to perform MMIO to an encrypted address. (1) As discussed, please update the commit message, for more clarify about SEV vs. SEV-ES. > > Read the PcdTpmBaseAddress and mark the specification defined range > (0x5000 in length) as un-encrypted, to allow an SEV-ES guest to process > the MMIO requests. > > Cc: Laszlo Ersek > Cc: Ard Biesheuvel > Cc: Jordan Justen > Cc: Brijesh Singh > Cc: James Bottomley > Cc: Jiewen Yao > Cc: Min Xu > Signed-off-by: Tom Lendacky > --- > OvmfPkg/PlatformPei/PlatformPei.inf | 1 + > OvmfPkg/PlatformPei/AmdSev.c | 19 +++++++++++++++++++ > 2 files changed, 20 insertions(+) > > diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/PlatformPei.inf > index 6ef77ba7bb21..de60332e9390 100644 > --- a/OvmfPkg/PlatformPei/PlatformPei.inf > +++ b/OvmfPkg/PlatformPei/PlatformPei.inf > @@ -113,6 +113,7 @@ [Pcd] > > [FixedPcd] > gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress > + gEfiSecurityPkgTokenSpaceGuid.PcdTpmBaseAddress > gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIMemoryNVS > gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIReclaimMemory > gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiReservedMemoryType > diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c > index dddffdebda4b..d524929f9e10 100644 > --- a/OvmfPkg/PlatformPei/AmdSev.c > +++ b/OvmfPkg/PlatformPei/AmdSev.c > @@ -141,6 +141,7 @@ AmdSevInitialize ( > ) > { > UINT64 EncryptionMask; > + UINT64 TpmBaseAddress; > RETURN_STATUS PcdStatus; > > // > @@ -206,6 +207,24 @@ AmdSevInitialize ( > } > } > > + // > + // PEI TPM support will perform MMIO accesses, be sure this range is not > + // marked encrypted. > + // > + TpmBaseAddress = PcdGet64 (PcdTpmBaseAddress); > + if (TpmBaseAddress != 0) { It's OK to keep this as a sanity check, yes. > + RETURN_STATUS DecryptStatus; > + > + DecryptStatus = MemEncryptSevClearPageEncMask ( > + 0, > + TpmBaseAddress, > + EFI_SIZE_TO_PAGES (0x5000), (2) Should be (UINTN)0x5000, as discussed earlier. > + FALSE > + ); > + > + ASSERT_RETURN_ERROR (DecryptStatus); (3) So this is where the mess begins. The idea is to delay the dispatch of Tcg2ConfigPei until after PlatformPei determines if SEV is active, and (in case SEV is active) PlatformPei decrypts the MMIO range of the TPM. For this, we need to change the IA32 / X64 DEPEX of Tcg2ConfigPei, from the current TRUE, to some PPI GUID. There are two choices for that PPI: (a) gEfiPeiMemoryDiscoveredPpiGuid Advantages: - no new PPI definition needed, - no new PPI installation needed, - OvmfPkg/Bhyve/PlatformPei needs no separate change Disadvantages: - total abuse of gEfiPeiMemoryDiscoveredPpiGuid (b) gOvmfTpmMmioAccessiblePpiGuid Disadvantages: - this new GUID must be defined in "OvmfPkg.dec", in the [Ppis] section, in a separate patch; its comment should say "this PPI signals that accessing the MMIO range of the TPM is possible in the PEI phase, regardless of memory encryption". The PPI definitions should be kept alphabetically ordered. - OvmfPkg/PlatformPei must install this new PPI, with a NULL interface. (See "mPpiBootMode" as a technical example.) OvmfPkg/PlatformPei must install this new PPI either when the SEV check at the top of AmdSevInitialize() fails, or when MemEncryptSevClearPageEncMask() succeeds. - OvmfPkg/Bhyve/PlatformPei must receive the same update, in a separate patch. That's because "OvmfPkg/Bhyve/BhyveX64.fdf" includes the same "Tcg2ConfigPei", but consumes "OvmfPkg/Bhyve/PlatformPei" rather than "OvmfPkg/PlatformPei". Tcg2ConfigPei will receive the same stricter-than-before depex, so something on the bhyve platform too must produce the new PPI. Advantages: - more or less palatable as a concept, with the new PPI precisely expressing the dependency we have. In approach (b), the "OvmfPkg/Bhyve/PlatformPei" patch needs to be CC'd to the Bhyve reviewers. If the Bhyve reviewers determine that such an update is actually unnecessary, because on Bhyve, there is no TPM support and/or no SEV support in fact, then *first* we have to create an independent Bhyve cleanup series, that rips out the TPM and/or SEV remnants from the OvmfPkg/Bhyve sub-tree. I prefer approach (b). I'm sorry that it means extra work wrt. Bhyve, but I strongly believe in keeping all platforms in the tree, and that means we need to spend time on such changes. I'm not CC'ing Rebecca and Peter on this message -- we're deep into this patch review thread, and they'd have no useful context. I suggest simply including the "OvmfPkg/Bhyve/PlatformPei" patch in the next version of this series, with a proper explanation in the blurb (patch#0) and on the "OvmfPkg/Bhyve/PlatformPei" patch. That should give them enough context to evaluate whether the change is necessary, or whether we should purge the TPM and/or the SEV bits from Bhyve. You could also ask them just this question in advance, in a separate email on the list (with distilled context). Personally I'm unsure if the TPM and SEV bits survived into Bhyve because those bits are actually put to use there, or because the initial platform creation / cloning wasn't as minimal as it could have been. Note that in case TPM makes sense on bhyve but SEV doesn't, then "OvmfPkg/Bhyve/PlatformPei" will have to install the new PPI unconditionally. Thanks Laszlo