[edk2-devel] [PATCH RFC v2 11/28] OvmfPkg: Reserve Secrets page in MEMFD
Laszlo Ersek
lersek at redhat.com
Wed May 5 19:33:38 UTC 2021
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 <jejb at linux.ibm.com>
>>> Cc: Min Xu <min.m.xu at intel.com>
>>> Cc: Jiewen Yao <jiewen.yao at intel.com>
>>> Cc: Tom Lendacky <thomas.lendacky at amd.com>
>>> Cc: Jordan Justen <jordan.l.justen at intel.com>
>>> Cc: Ard Biesheuvel <ardb+tianocore at kernel.org>
>>> Cc: Laszlo Ersek <lersek at redhat.com>
>>> Cc: Erdem Aktas <erdemaktas at google.com>
>>> Signed-off-by: Brijesh Singh <brijesh.singh at 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.
> 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.
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.
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 (#74773): https://edk2.groups.io/g/devel/message/74773
Mute This Topic: https://groups.io/mt/82479058/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list