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.web10.9982.1623144041902256345 for ; Tue, 08 Jun 2021 02:20:42 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=Ox3+Wp0N; 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=1623144041; 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=IQwUF5RpGWSATghI05DooodFA6Bhq9g6CngmRm9vc+M=; b=Ox3+Wp0N1ir8AGqY679M7gVOvlyaVNU3gFOcLG3zKEprbGQApx/e8/4LWbNt3CYY7l0AAw DSi07KMpINMjg5La4j7CQ4XGiIx0bRdLInVN4klv00f8/bsCsInh8t8kglcFk/DoJRLb4g 9iPRFYMKXm15uWp7r+QoDhHlLjrIMLg= 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-476-_GPPyMRNPK6rCNywQXXCaw-1; Tue, 08 Jun 2021 05:20:37 -0400 X-MC-Unique: _GPPyMRNPK6rCNywQXXCaw-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 0D449501E1; Tue, 8 Jun 2021 09:20:36 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-113-27.ams2.redhat.com [10.36.113.27]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 8516660FC2; Tue, 8 Jun 2021 09:20:33 +0000 (UTC) Subject: Re: [PATCH RFC v3 05/22] OvmfPkg: reserve Secrets page in MEMFD To: Brijesh Singh , James Bottomley , Min Xu , Jiewen Yao , Tom Lendacky , Jordan Justen , Erdem Aktas , Eric Dong , Ray Ni , Rahul Kumar , devel@edk2.groups.io Cc: Ard Biesheuvel References: <20210526231118.12946-1-brijesh.singh@amd.com> <20210526231118.12946-6-brijesh.singh@amd.com> <6c1d0c68-0537-9b58-ada4-ec9deb1a7c9d@redhat.com> From: "Laszlo Ersek" Message-ID: <699aba35-c2af-f9c0-4904-e9be1032b13d@redhat.com> Date: Tue, 8 Jun 2021 11:20:32 +0200 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 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: 8bit On 06/07/21 17:58, Brijesh Singh wrote: > > On 6/7/21 7:26 AM, Laszlo Ersek wrote: >> On 05/27/21 01:11, Brijesh Singh wrote: >>> BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3275&data=04%7C01%7Cbrijesh.singh%40amd.com%7C32a95d87f0984b88080708d929af878f%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637586656154129803%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=JyrMLVE%2BMNq%2B1sUTI7WnbxkjloKi81PcISiLvz2geLg%3D&reserved=0 >>> >>> When AMD SEV is enabled in the guest VM, a hypervisor need to insert a >>> secrets page. >> For pure SEV? > > The secrets page is applicable to all the SEV's (SEV, SEV-ES and > SEV-SNP) but there is some difference see below. > > >> >>> When SEV-SNP is enabled, the secrets page contains the VM platform >>> communication keys. The guest BIOS and OS can use this key to communicate >>> with the SEV firmware to get attesation report. See the SEV-SNP firmware >>> spec for more details for the content of the secrets page. >>> >>> When SEV and SEV-ES is enabled, the secrets page contains the information >>> provided by the guest owner after the attestation. See the SEV >>> LAUNCH_SECRET command for more details. >>> >>> Cc: James Bottomley >>> Cc: Min Xu >>> Cc: Jiewen Yao >>> Cc: Tom Lendacky >>> Cc: Jordan Justen >>> Cc: Ard Biesheuvel >>> Cc: Laszlo Ersek >>> Cc: Erdem Aktas >>> Signed-off-by: Brijesh Singh >>> --- >>> OvmfPkg/OvmfPkgX64.dsc | 2 ++ >>> OvmfPkg/OvmfPkgX64.fdf | 5 +++++ >>> OvmfPkg/AmdSev/SecretPei/SecretPei.inf | 1 + >>> OvmfPkg/AmdSev/SecretPei/SecretPei.c | 15 ++++++++++++++- >>> 4 files changed, 22 insertions(+), 1 deletion(-) >> How is all of the above related to the "OvmfPkg/OvmfPkgX64.dsc" >> platform, where remote attestation is not a goal? >> >> What you describe makes sense to me, but only for the remote-attested >> "OvmfPkg/AmdSev/AmdSevX64.dsc" platform. (Which already includes >> SecretPei and SecretDxe, and sets the necessary PCDs.) >> >> Then, even if we limit this patch only to the "OvmfPkg/AmdSev/SecretPei" >> module, the commit message does not explain sufficiently why the secrets >> page must be reserved for good. The "SEV-SNP firmware spec" reference is >> vague at best; I'm permanently lost between the dozen PDF files I have >> downloaded locally from the AMD website. Please include a specific >> document number, revision number, and chapter/section identifier. > > > There is a fundamental difference between SEV and SEV-SNP attestation > flow. In the case of SEV and SEV-ES, the attestation happens before the > VM is booted, and the secrets page contains the data provided by the > guest owner after the attestation is complete. The hypervisor injects > that data into the guest memory before booting it.  However, with > SEV-SNP, the guest uses the data from the secrets page to build a > message for the PSP. The guest can send the following message to the PSP: > > 1. Expand the filtered CPUID list > 2. Query attestation report > 2. Derive a key > 3. VM export, import, and absorb -- migration specific command > > See chapter 7 [1] for all possible commands that a guest can send to PSP > through the guest message request. I understand that it is confusing, > but the secrets page is *not* same as SEV/SEV-ES. But since SEV-SNP spec > calls it secrets, so I used the same name.  I thought the secrets page was entirely opaque to the guest firmware; i.e., all the guest firmware would do with it is (a) cover it with an allocation in SecretPei, (b) forward it to the guest OS via a UEFI system config table in SecretDxe. This patch uses the same PCD names ("launch secret", where I understand the SEV-SNP case *not* to be a *launch* secret; is that right?), plus it uses the same drivers. That's way too confusing. So what is this "SNP secrets" page supposed to contain: - both the previously defined SEV/SEV-ES level launch secret, and the SNP-specific VMPCK (?) - how are these secret bits separated from each other in the page? - does the guest (firmware and/or OS) *write* to the new locations in the page, possibly for secure message construction? Either way, I think the proposed repurposing of the page, for the sake of SNP secrets (VMPCK and maybe even secure message construction?), breaks the current declarations of the PCDs, in "OvmfPkg.dec": ## The base address and size of the SEV Launch Secret Area provisioned # after remote attestation. If this is set in the .fdf, the platform # is responsible for protecting the area from DXE phase overwrites. gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretBase|0x0|UINT32|0x42 gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretSize|0x0|UINT32|0x43 > > In SEV-SNP, the secrets page is not tight up with just the remote > attestation. This is the most important statement. We need the SNP secrets page even without remote attestation. OvmfPkgX64.dsc does not deal with remote attestation. But then (putting all the PCD naming confusion aside), if a driver is promoted to "common use", from the AmdSevX64 platform to multiple OvmfPkg platforms, then it should be lifted to the top-level OvmfPkg directory. > Later, the AmdSev.dsc can include a library to perform the > SEV-SNP-specific attestation. The library can use the SNP secrets page > to get the key and message counter use for constructing the guest > message to query the attestation report. > > I hope it clarifies it. > > [1] https://www.amd.com/system/files/TechDocs/56860.pdf > > >> Honestly I'm getting a *rushed* vibe on this whole series. Why is that? > > I am not sure why you are getting this feel, please let me know where I > can help to clarify but the series is *rushed* at all. Its building on > existing support. It's possible that we are getting mixed with the > fundamental difference between the SEV and SEV-SNP attestation flow and > recent patches from Dov to expand the attestation to cover other aspects > of the boot flow. > > In case of SEV-SNP, some folks may prefer to do all the attestation in > the OVMF and others may prefer to do the attestation in the guest OS. We > should try to not restrict one approach over the other. > > >> >> Assume that I'm dumb. You won't be far from the truth. Then hold my hand >> through all this? > > > Please let me know if the above explanation helps or I should expand more. You should please (a) expand your *commit messages*, (b) add a *wall* of text in the "OvmfPkg.dec" file, where the PCDs in questions are declared. When I grep the OvmfPkg subdirectory in two years for "PcdSevLaunchSecretBase", I'd like to find the DEC file's comments to be consistent with the actual uses of the PCD, and I'd like git-blame to tell me something useful about those lines, too. One problem is that I'm supposed to internalize about 50 pages from yet from another technical specification, in order to get the basics of a single patch. I can't even follow the *set* of AMD documents I should have a local copy of. How am I supposed to interleave all that with, for example, reviewing a 57 slide TDX design presentation? Honestly, this has gone off the rails. The pressure that Confidential Computing has generated for me as an OvmfPkg co-maintainer over the course of a few months exceeds what I've been under for nearly a *decade*, including all prior work with SEV and SEV-ES. This makes me incredibly unhappy. Laszlo > > >> Laszlo >> >> >>> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc >>> index 999738dc39cd..ea08e1fabc65 100644 >>> --- a/OvmfPkg/OvmfPkgX64.dsc >>> +++ b/OvmfPkg/OvmfPkgX64.dsc >>> @@ -716,6 +716,7 @@ [Components] >>> OvmfPkg/SmmAccess/SmmAccessPei.inf >>> !endif >>> UefiCpuPkg/CpuMpPei/CpuMpPei.inf >>> + OvmfPkg/AmdSev/SecretPei/SecretPei.inf >>> >>> !if $(TPM_ENABLE) == TRUE >>> OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf >>> @@ -966,6 +967,7 @@ [Components] >>> OvmfPkg/PlatformDxe/Platform.inf >>> OvmfPkg/AmdSevDxe/AmdSevDxe.inf >>> OvmfPkg/IoMmuDxe/IoMmuDxe.inf >>> + OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf >>> >>> !if $(SMM_REQUIRE) == TRUE >>> OvmfPkg/SmmAccess/SmmAccess2Dxe.inf >>> diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf >>> index d6be798fcadd..9126b8eb5014 100644 >>> --- a/OvmfPkg/OvmfPkgX64.fdf >>> +++ b/OvmfPkg/OvmfPkgX64.fdf >>> @@ -88,6 +88,9 @@ [FD.MEMFD] >>> 0x00C000|0x001000 >>> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupSize >>> >>> +0x00D000|0x001000 >>> +gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretBase|gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretSize >>> + >>> 0x010000|0x010000 >>> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize >>> >>> @@ -179,6 +182,7 @@ [FV.PEIFV] >>> INF SecurityPkg/Tcg/TcgPei/TcgPei.inf >>> INF SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf >>> !endif >>> +INF OvmfPkg/AmdSev/SecretPei/SecretPei.inf >>> >>> ################################################################################ >>> >>> @@ -314,6 +318,7 @@ [FV.DXEFV] >>> INF ShellPkg/Application/Shell/Shell.inf >>> >>> INF MdeModulePkg/Logo/LogoDxe.inf >>> +INF OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf >>> >>> # >>> # Network modules >>> diff --git a/OvmfPkg/AmdSev/SecretPei/SecretPei.inf b/OvmfPkg/AmdSev/SecretPei/SecretPei.inf >>> index 08be156c4bc0..9265f8adee12 100644 >>> --- a/OvmfPkg/AmdSev/SecretPei/SecretPei.inf >>> +++ b/OvmfPkg/AmdSev/SecretPei/SecretPei.inf >>> @@ -26,6 +26,7 @@ [LibraryClasses] >>> HobLib >>> PeimEntryPoint >>> PcdLib >>> + MemEncryptSevLib >>> >>> [FixedPcd] >>> gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretBase >>> diff --git a/OvmfPkg/AmdSev/SecretPei/SecretPei.c b/OvmfPkg/AmdSev/SecretPei/SecretPei.c >>> index ad491515dd5d..51eb094555aa 100644 >>> --- a/OvmfPkg/AmdSev/SecretPei/SecretPei.c >>> +++ b/OvmfPkg/AmdSev/SecretPei/SecretPei.c >>> @@ -7,6 +7,7 @@ >>> #include >>> #include >>> #include >>> +#include >>> >>> EFI_STATUS >>> EFIAPI >>> @@ -15,10 +16,22 @@ InitializeSecretPei ( >>> IN CONST EFI_PEI_SERVICES **PeiServices >>> ) >>> { >>> + UINTN Type; >>> + >>> + // >>> + // The location of the secret page should be marked reserved so that guest OS >>> + // does not treated as a system RAM. >>> + // >>> + if (MemEncryptSevSnpIsEnabled ()) { >>> + Type = EfiReservedMemoryType; >>> + } else { >>> + Type = EfiBootServicesData; >>> + } >>> + >>> BuildMemoryAllocationHob ( >>> PcdGet32 (PcdSevLaunchSecretBase), >>> PcdGet32 (PcdSevLaunchSecretSize), >>> - EfiBootServicesData >>> + Type >>> ); >>> >>> return EFI_SUCCESS; >>> >