Re: [PATCH RFC v3 05/22] OvmfPkg: reserve Secrets page in MEMFD


Laszlo Ersek
 

On 06/07/21 14:26, Laszlo Ersek wrote:
On 05/27/21 01:11, Brijesh Singh wrote:
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275

When AMD SEV is enabled in the guest VM, a hypervisor need to insert a
secrets page.
For pure SEV?


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@...>
Cc: Min Xu <min.m.xu@...>
Cc: Jiewen Yao <jiewen.yao@...>
Cc: Tom Lendacky <thomas.lendacky@...>
Cc: Jordan Justen <jordan.l.justen@...>
Cc: Ard Biesheuvel <ardb+tianocore@...>
Cc: Laszlo Ersek <lersek@...>
Cc: Erdem Aktas <erdemaktas@...>
Signed-off-by: Brijesh Singh <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.

Honestly I'm getting a *rushed* vibe on this whole series. Why is that?

Assume that I'm dumb. You won't be far from the truth. Then hold my hand
through all this?
Here's the v2 discussion:

- http://mid.mail-archive.com/9804ecb5-8afd-c56e-4982-d1a6ebad3de8@redhat.com
- https://edk2.groups.io/g/devel/message/74797
- https://listman.redhat.com/archives/edk2-devel-archive/2021-May/msg00112.html

That discussion refers to a different use case, raised by Dov. That use
case might justify reserving the area even for plain SEV. It's out of
scope for now, AIUI.

(

And even for that separate use case, James showed down-thread that *not*
reserving the page forever in the firmware is more flexible.

- http://mid.mail-archive.com/aed7d3490fe6edee74440ed8e4cd5364fb2ba4af.camel@linux.ibm.com
- https://edk2.groups.io/g/devel/message/74801
- https://listman.redhat.com/archives/edk2-devel-archive/2021-May/msg00116.html

)

AFAICT, the only effect of the v2 sub-thread on the patch has been that
we now use the Reserved memory type rather than AcpiNVS (when SEV-SNP is
in use). I have two comments on that:

- It's good that we're not mixing in the other use case raised by Dov
(i.e., enabling the guest-kernel to read secrets from the injected
page even under plain SEV).

- It's still unclear to me why the reservation needs to be permanent
under SEV-SNP.

Thanks
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 <PiPei.h>
#include <Library/HobLib.h>
#include <Library/PcdLib.h>
+#include <Library/MemEncryptSevLib.h>

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;

Join devel@edk2.groups.io to automatically receive all group messages.