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 <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.
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 <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;
