From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mx.groups.io with SMTP id smtpd.web08.13505.1619797161924608382 for ; Fri, 30 Apr 2021 08:39:22 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=TOpRYXp4; spf=pass (domain: redhat.com, ip: 170.10.133.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1619797161; 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=MmAQugzIcPaKY3n9hdozy0LRttFYxh2GYAWDa0nGaus=; b=TOpRYXp4Z09MgJqYNIOI6065Xy2reC18oyfdjakwJOL04EkKP752YpfPEylQisXXxDCd8x 3ow9ZRB6ORs9wm6ALPviMmQPuACHRnbtTn0atvCLB4k4CBft9xLGUyFOGIyKYtzgztOCFC D0I0TpEz8UZcgnIVcZnWPXSSh1ZHEn8= 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-586-N10Cc37SPdOnUo_GEkzhDQ-1; Fri, 30 Apr 2021 11:39:18 -0400 X-MC-Unique: N10Cc37SPdOnUo_GEkzhDQ-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 5CC2BDF8DB; Fri, 30 Apr 2021 15:39:15 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-248.ams2.redhat.com [10.36.112.248]) by smtp.corp.redhat.com (Postfix) with ESMTP id 97BAD5D9E2; Fri, 30 Apr 2021 15:39:12 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 3/3] OvmfPkg/PlatformPei: Mark TPM MMIO range as unencrypted for SEV To: Tom Lendacky , devel@edk2.groups.io Cc: Joerg Roedel , Borislav Petkov , Ard Biesheuvel , Jordan Justen , Brijesh Singh , James Bottomley , Jiewen Yao , Min Xu References: <1f64ca5689ec86c427e4db8c41da598896dca4ba.1618959281.git.thomas.lendacky@amd.com> <8417023b-b3d6-5268-e92a-0c6f5ebfff6b@redhat.com> <4261e15a-84a0-11a7-2981-9eeb538f6da9@redhat.com> <311cdc78-d818-bea4-16a1-01077bfc1140@amd.com> <21f40236-88f6-5886-1345-5a8b9ac1f732@amd.com> <0ad9369d-d863-5bfc-1326-ff4b631bd04e@amd.com> <3f2e589d-7258-c372-c1ab-60992351dbde@redhat.com> From: "Laszlo Ersek" Message-ID: <5051e966-f47a-3a19-6523-2a536d236505@redhat.com> Date: Fri, 30 Apr 2021 17:39:11 +0200 MIME-Version: 1.0 In-Reply-To: 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 On 04/28/21 21:09, Tom Lendacky wrote: > On 4/28/21 11:12 AM, Laszlo Ersek wrote: >> On 04/27/21 16:58, Tom Lendacky wrote: >>> On 4/26/21 9:21 AM, Tom Lendacky wrote: >>>> On 4/26/21 7:07 AM, Laszlo Ersek wrote: >>>>> On 04/23/21 22:02, Tom Lendacky wrote: >> >>>>>> 1. SEV works with the current encrypted mapping, it is only the SEV-ES >>>>>> support that fails because of the ValidateMmioMemory() check. I can do >>>>>> the mapping change just for SEV-ES since it is X64 only. This works, >>>>>> because MemEncryptSevClearPageEncMask() will not return UNSUPPORTED >>>>>> when running in 64-bit. >>>>> >>>>> Can we really say "SEV works" though? Because, even using an X64 PEI >>>>> phase, and enabling only SEV (not SEV-ES), TPM access will be broken in >>>>> the PEI phase. Is my understanding correct? >>>> >>>> Because the memory range is marked as MMIO, we'll take a nested page fault >>>> (NPF). The GPA passed as part of the NPF does not include the c-bit. So we >>>> do in fact work properly with a TPM in SEV. >> >> Thanks for the explanation. >> >> Here's what bothers me about it: >> >> In AmdSevDxe, we clear the C-bit from MMIO and NonExistent areas in the >> GCD memory space map. This occurs early in the DXE phase (see "APRIORI >> DXE" in the FDF files). The justification is that we want the flash and >> (for example) the PCI MMIO apertures decrypted. >> >> Now, I realize there is a difference between flash and TPM. TPM is >> purely MMIO (no KVM memslot), but flash (when it is not in programming >> mode) is backed by a read-only KVM memslot. IOW, flash is "actual >> memory", and so it is affected by SEV. TPM is never "actual memory", so >> (according to your explanation, AIUI) it always traps to QEMU, per >> access, and the C-bit doesn't interfere with that. >> >> This is consistent with two facts about OVMF's PEI phase: >> >> - We use IO Port-based fw_cfg (never DMA), if SEV is enabled (see >> "QemuFwCfgPei.c"). >> >> - We access PCI config space via IO Ports (0xCF8, 0xCFC), never ECAM. >> (This was not motivated by SEV, see commit 7523788faa51, but it does >> play nice with SEV, in the PEI phase -- I think?) >> >> What I'm confused about, now, in retrospect, is the reference to the PCI >> MMIO aperture, in AmdSevDxe. If that area isn't backed by a KVM memslot >> *either* -- similarly to the TPM area --, then decrypting *that* in >> AmdSevDxe (via "nonexistent") is not strictly necessary. Is that correct? > > It is necessary for (and was added for) SEV-ES. The explicit mapping of > the PCI MMCONFIG range as unencrypted was added for SEV-ES so that the > SEV-ES mitigation would not terminate the guest because MMIO was being > performed against an encrypted address. > > There's a nice comment in OvmfPkg/PlatformPei/Platform.c that talks about > how the MMCONFIG range is not added as MMIO, but instead as reserved > memory. Because of that, the loop through the memory space map in > AmdSevDxe.c does not mark the MMCONFIG range as unencrypted. I didn't mean commit 84cddd70820f ("OvmfPkg/AmdSevDxe: Clear encryption bit on PCIe MMCONFIG range", 2021-01-07) -- I didn't mean PCI config space. I meant commit 24e4ad75546b ("OvmfPkg: Add AmdSevDxe driver", 2017-07-10) -- I wrote "PCI MMIO aperture". So, to repeat my question: when 24e4ad75546b3 was implemented (which happened just for SEV), then (apparently) clearing the C-bit on the PCI MMIO aperture (where PCI MMIO BARs were going to be allocated from) wasn't strictly necessary *at the time*; correct? Because that area isn't backed by RAM, accesses trap to QEMU directly, and the C bit does not make a difference there. (IIUC). Does this make sense or am I wrong? > >> >> I'm not asking for any code changes, just trying to form a consistent view. >> >> Another question (still for "base SEV"): when OVMF is built with >> SMM_REQUIRE, PlatformPei performs a (read-only) variable access. See the >> ReadOnlyVariable2->GetVariable() call in the RefreshMemTypeInfo() >> function. When SEV is active, does control reach RefreshMemTypeInfo() on >> your end? And does ReadOnlyVariable2->GetVariable() succeed for you? >> >> (There is a DEBUG_VERBOSE message in OnReadOnlyVariable2Available(), and >> a DEBUG_ERROR in RefreshMemTypeInfo(), so the above questions can be >> answered just from the log; no need to modify the code for that.) > > Yes, control reaches that point. But, notably, with a legacy VM I see the > following messages: > RefreshMemTypeInfo: GetVariable(): Not Found > > But with an SEV VM I see: > Firmware Volume for Variable Store is corrupted > RefreshMemTypeInfo: GetVariable(): Not Found > > So I get the "Not Found" in both cases. But with SEV, I see the > "corrupted" message from InitRealNonVolatileVariableStore() in > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableNonVolatile.c. I > imagine that since the range is marked encrypted, it reads the header > incorrectly and fails. Thank you for confirming! I don't think we need to sweat this symptom. I'm just glad my hunch wasn't wrong. Thanks, Laszlo > > Thanks, > Tom > >> >> Basically, with SEV enabled, I expect ReadOnlyVariable2->GetVariable() >> to fail -- or even to remain unreached, as FaultTolerantWritePei and >> VariablePei could bail out earlier (before installing the Variable PPI), >> due to failing flash accesses. In case I'm *not* wrong -- it's not the >> end of the world, I'm only asking this question too for the sake of >> clarifying "C-bit vs. MMIO". >> >> More or less it seems to boil down to whether there is a KVM memslot or >> not -- which is *not* equivalent to OVMF considering the area MMIO or not. >> >> >>>> SEV-ES would also work >>>> properly if the mitigation for accessing an encrypted address was removed >>>> from the #VC handler. It is only this added mitigation to protect MMIO >>>> that results in an issue with the TPM in PEI. >>> >>> So I'm thinking that I can have TpmMmioSevDecryptPeim.c do this: >>> >>> // >>> // If SEV or SEV-ES is active, MMIO succeeds against an encrypted physical >>> // address because the nested page fault (NPF) that occurs on access does not >>> // include the encryption bit in the guest physical address provided to the >>> // hypervisor. >>> // >>> // However, if SEV-ES is active, before performing the actual MMIO, an >>> // additional MMIO mitigation check is performed in the #VC handler to ensure >>> // that MMIO is being done to an unencrypted address. To prevent guest >>> // termination in this scenario, mark the range unencrypted ahead of access. >>> // >>> if (MemEncryptSevEsIsEnabled ()) { >>> // Do MemEncryptSevClearPageEncMask() ... >>> } >>> >>> Let me submit the next version with this and see what you think. >> >> Yep, I'll review that now. >> >> Thanks >> Laszlo >> >>> >>> Thanks, >>> Tom >>> >>>> >>>>> >>>>> I think the behavior you currently see is actually what we want, we >>>>> should double down on it -- if MemEncryptSevClearPageEncMask() fails, >>>>> report an explicit DEBUG_ERROR, and call CpuDeadLoop(). If the firmware >>>>> is built with TPM_ENABLE, and SEV is active, then an IA32 PEI phase is >>>>> simply unusable. Silently pretending that the TPM is not there, even >>>>> though it may have been configured on the QEMU command line, we just >>>>> failed to communicate with it, is not a good idea, IMO. >>>> >>>> However, because the c-bit is not part of the NPF, we do communicate >>>> successfully with the TPM. >>>> >>>> So we could actually do following: >>>> - For IA32: >>>> - Remove the Depex on gOvmfTpmMmioAccessiblePpiGuid >>>> - Do not add OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf >>>> >>>> - For X64: >>>> - Add the Depex on gOvmfTpmMmioAccessiblePpiGuid >>>> - Add OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf >>>> >>>> That might be confusing, though. So we could just do option #3 below. >>>> >>>> Thanks, >>>> Tom >>>> >>>>> >>>>> This is somewhat similar IMO to the S3Verification() function in >>>>> "OvmfPkg/PlatformPei/Platform.c". >>>>> >>>>> TPM_ENABLE, SEV, IA32 PEI phase: pick any two. >>>>> >>>>> Thanks, >>>>> Laszlo >>>>> >>>>>> >>>>>> 2. Call MemEncryptSevClearPageEncMask() for SEV or SEV-ES, but don't check >>>>>> the return status. >>>>>> >>>>>> 3. Create Ia32 and X64 versions of internal functions, where the Ia32 >>>>>> version simply returns SUCCESS because it can't do anything and the X64 >>>>>> version calls MemEncryptSevClearPageEncMask(), allowing the main code >>>>>> to ASSERT on any errors. >>>>>> >>>>>> I'm leaning towards #1, because this is an SEV-ES only issue. Thoughts? >>>>>> >>>>>> Thanks, >>>>>> Tom >>>>>> >>>>>>> >>>>>>> One thing I found is that the Bhyve package makes reference to the >>>>>>> OvmfPkg/Bhyve/Tcg directory, but that directory does not exist. So I don't >>>>>>> think that TPM enablement has been tested. I didn't update the Bhyve >>>>>>> support for that reason. >>>>>>> >>>>>>> Thanks, >>>>>>> Tom >>>>>>> >>>>>>>> Thanks! >>>>>>>> Laszlo >>>>>>>> >>>>>> >>>>> >>> >> >