Hi Dov, On 05/06/21 12:57, Dov Murik wrote: > > > On 05/05/2021 22:33, Laszlo Ersek wrote: >> On 05/05/21 15:11, Brijesh Singh wrote: >>> >>> On 5/5/21 1:42 AM, Dov Murik wrote: >>>> [+cc: Tobin] >>>> >>>> Hi Brijesh, >>>> >>>> On 30/04/2021 14:51, 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%7C93168c94eb6d44ed08e608d90f910426%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637557937779907471%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=nLpmk3G%2BmXcZrzXxCmO3M9EDPiLRnP1IUmPqRQNbBuU%3D&reserved=0 >>>>> >>>>> When AMD SEV is enabled in the guest VM, a hypervisor need to insert a >>>>> secrets page. >>>>> >>>>> 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 <j...@linux.ibm.com> >>>>> Cc: Min Xu <min.m...@intel.com> >>>>> Cc: Jiewen Yao <jiewen....@intel.com> >>>>> Cc: Tom Lendacky <thomas.lenda...@amd.com> >>>>> Cc: Jordan Justen <jordan.l.jus...@intel.com> >>>>> Cc: Ard Biesheuvel <ardb+tianoc...@kernel.org> >>>>> Cc: Laszlo Ersek <ler...@redhat.com> >>>>> Cc: Erdem Aktas <erdemak...@google.com> >>>>> Signed-off-by: Brijesh Singh <brijesh.si...@amd.com> >>>>> --- >>>>> OvmfPkg/AmdSev/SecretPei/SecretPei.c | 16 +++++++++++++++- >>>>> OvmfPkg/AmdSev/SecretPei/SecretPei.inf | 1 + >>>>> OvmfPkg/OvmfPkgX64.dsc | 2 ++ >>>>> OvmfPkg/OvmfPkgX64.fdf | 5 +++++ >>>>> 4 files changed, 23 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/OvmfPkg/AmdSev/SecretPei/SecretPei.c >>>>> b/OvmfPkg/AmdSev/SecretPei/SecretPei.c >>>>> index ad491515dd..92836c562c 100644 >>>>> --- a/OvmfPkg/AmdSev/SecretPei/SecretPei.c >>>>> +++ b/OvmfPkg/AmdSev/SecretPei/SecretPei.c >>>>> @@ -7,6 +7,7 @@ >>>>> #include <PiPei.h> >>>>> #include <Library/HobLib.h> >>>>> #include <Library/PcdLib.h> >>>>> +#include <Library/MemEncryptSevLib.h> >>>>> >>>>> EFI_STATUS >>>>> EFIAPI >>>>> @@ -15,10 +16,23 @@ InitializeSecretPei ( >>>>> IN CONST EFI_PEI_SERVICES **PeiServices >>>>> ) >>>>> { >>>>> + UINTN Type; >>>>> + >>>>> + // >>>>> + // The secret page should be mapped encrypted by the guest OS and must >>>>> not >>>>> + // be treated as a system RAM. Mark it as ACPI NVS so that guest OS >>>>> maps it >>>>> + // encrypted. >>>>> + // >>>>> + if (MemEncryptSevSnpIsEnabled ()) { >>>>> + Type = EfiACPIMemoryNVS; >>>>> + } else { >>>>> + Type = EfiBootServicesData; >>>>> + } >>>>> + >>>> Would it make sense to always use EfiACPIMemoryNVS for the injected secret >>>> area, even for regular SEV (non-SNP)? >>> >>> Ideally yes. Maybe James had some reasons for choosing the >>> EfiBootServicesData. If I had to guess, it was mainly because there no >>> guest kernel support which consumes the SEV secrets page. >> >> git-blame fingers commit bff2811c6d99 ("OvmfPkg/AmdSev: assign and >> reserve the Sev Secret area", 2020-12-14). >> >> Commit bff2811c6d99 makes it clear that the area in question lives in MEMFD. >> >> We're populating the area in the PEI phase. We don't want anything in >> DXE to overwrite it. >> >> Once the bootloader (and/or perhaps the kernel's EFI stub) fetched the >> secret from that particular location, there is no need to prevent later >> parts of the OS (the actual kernel) from repurposing that area. That's >> why EfiBootServicesData was used. >> > > The first use of the secret area was to hold the guest luks disk passphrase; > this is used in the grub-inside-OVMF (AmdSev package), and there was no need > to keep that page around for the guest kernel. > > The reason I'm raising this whole point is that we're working now on > guest-kernel support for reading secrets from that injected page (for plain > SEV). We considered either (a) modifying the secrets page memory type to > reserved here, or (b) add code to the kernel EFI stub that would copy this > page somewhere else for kernel's later use (which seems more work and not > sure what's the advantage). > > Option (b) seems harder and more fragile, and I'm not sure if there are any > advantages (though I'm definitely not an expert in that area). > > > > >>> Since the >>> memory is not marked ACPI NVS, so it can be used as a system RAM after >>> the ExitBootServices is called in the kernel. >> >> Yes. >> >> I don't think AcpiNVS would be a good fit. Linux saves and restores >> AcpiNVS areas upon S3 suspend/resume. Regardless of whether S3 works, or >> will work, in SEV* guests, if we don't want the guest kernel to touch >> that area *at all*, Reserved is a better type. > > Thanks for this clarification. > >> >> Please refer to "Table 7-6 Memory Type Usage after ExitBootServices()" >> in the UEFI spec (v2.9). >> >>> >>> I am fine with using ACPI NVS for both SEV and SEV-SNP. I was not able >>> to build and run AmdSev package in my setup, can you submit a prepatch >>> to change the memory type and verify that it works ? >> >> NB: I've not yet reached this patch in my own review of the series, so >> I'm likely missing some context. I do have a thought -- under SEV-SNP, >> the secrets page apparently needs different (stronger) protection from >> the host as under plain SEV. I don't think that hiding the different >> protection requirements behind a single common memory type is helpful. >> Not to mention the wasted memory in the plain SEV case -- it's not a lot >> of memory, mind you, but the principle matters. >> > > Like I said above, we have plans to have this small amount of memory > available also to the guest OS; so maybe that shouldn't be the driving force > in the decision here.
What you describe (= runtime guest OS access) seems to justify changing the memory type of this allocation. However, that update doesn't look tied to SEV-SNP -- it's basically a "change of use case" (change of purpose) under plain SEV too. I think that deserves a separate (stand-alone) patch; maybe even a separate TianoCore BZ ticket. Thanks Laszlo > > -Dov > > > >> So ATM I would like to keep this patch in the SEV-SNP series, and to >> preserve the different memory types between SEV and SEV-SNP. >> >> Thanks >> Laszlo >> >> >> >> >>> >>>> >>>> -Dov >>>> >>>> >>>> >>>>> BuildMemoryAllocationHob ( >>>>> PcdGet32 (PcdSevLaunchSecretBase), >>>>> PcdGet32 (PcdSevLaunchSecretSize), >>>>> - EfiBootServicesData >>>>> + Type >>>>> ); >>>>> >>>>> return EFI_SUCCESS; >>>>> diff --git a/OvmfPkg/AmdSev/SecretPei/SecretPei.inf >>>>> b/OvmfPkg/AmdSev/SecretPei/SecretPei.inf >>>>> index 08be156c4b..9265f8adee 100644 >>>>> --- a/OvmfPkg/AmdSev/SecretPei/SecretPei.inf >>>>> +++ b/OvmfPkg/AmdSev/SecretPei/SecretPei.inf >>>>> @@ -26,6 +26,7 @@ >>>>> HobLib >>>>> PeimEntryPoint >>>>> PcdLib >>>>> + MemEncryptSevLib >>>>> >>>>> [FixedPcd] >>>>> gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretBase >>>>> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc >>>>> index a7d747f6b4..593c0e69f6 100644 >>>>> --- a/OvmfPkg/OvmfPkgX64.dsc >>>>> +++ b/OvmfPkg/OvmfPkgX64.dsc >>>>> @@ -716,6 +716,7 @@ >>>>> OvmfPkg/SmmAccess/SmmAccessPei.inf >>>>> !endif >>>>> UefiCpuPkg/CpuMpPei/CpuMpPei.inf >>>>> + OvmfPkg/AmdSev/SecretPei/SecretPei.inf >>>>> >>>>> !if $(TPM_ENABLE) == TRUE >>>>> OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf >>>>> @@ -965,6 +966,7 @@ >>>>> 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 d519f85328..b04175f77c 100644 >>>>> --- a/OvmfPkg/OvmfPkgX64.fdf >>>>> +++ b/OvmfPkg/OvmfPkgX64.fdf >>>>> @@ -88,6 +88,9 @@ >>>>> gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase|gUefiCpuPkgTokenSpaceGuid.PcdSevE >>>>> 0x00C000|0x001000 >>>>> >>>>> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupSize >>>>> >>>>> +0x00D000|0x001000 >>>>> +gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretBase|gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretSize >>>>> + >>>>> 0x010000|0x010000 >>>>> >>>>> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize >>>>> >>>>> @@ -178,6 +181,7 @@ INF OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf >>>>> INF SecurityPkg/Tcg/TcgPei/TcgPei.inf >>>>> INF SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf >>>>> !endif >>>>> +INF OvmfPkg/AmdSev/SecretPei/SecretPei.inf >>>>> >>>>> >>>>> ################################################################################ >>>>> >>>>> @@ -313,6 +317,7 @@ INF >>>>> OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf >>>>> INF ShellPkg/Application/Shell/Shell.inf >>>>> >>>>> INF MdeModulePkg/Logo/LogoDxe.inf >>>>> +INF OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf >>>>> >>>>> # >>>>> # Network modules >>>>> >>> >>> >>> >>> >>> >> > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#74797): https://edk2.groups.io/g/devel/message/74797 Mute This Topic: https://groups.io/mt/82479058/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
