In the future, please include the reviewers on cc: that are identified in
the Maintainers.txt file (use ./BaseTools/Scripts/GetMaintainer.py), I
almost missed this patch.
On 12/8/22 17:43, Adam Dunlap via groups.io wrote:
When running under SEV-ES, a page of shared memory is allocated for the
GHCB during the SEC phase at address 0x809000. This page of memory is
eventually passed to the OS as EfiConventionalMemory. When running
SEV-SNP, this page is not PVALIDATE'd in the RMP table, meaning that if
the guest OS tries to access the page, it will think that the host has
voilated the security guarantees and will likely crash.
This patch validates this page immediately after EDK2 switches to using
the GHCB page allocated for the PEI phase.
This was tested by writing a UEFI application that reads to and writes
from one bytes of each page of memory and checks to see if a #VC
exception is generated indicating that the page was not validated.
Signed-off-by: Adam Dunlap <acdun...@google.com>
This should include a Fixes: tag.
---
OvmfPkg/PlatformPei/AmdSev.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c
index e1b9fd9b7f..c465732068 100644
--- a/OvmfPkg/PlatformPei/AmdSev.c
+++ b/OvmfPkg/PlatformPei/AmdSev.c
@@ -206,6 +206,7 @@ AmdSevEsInitialize (
{
UINT8 *GhcbBase;
PHYSICAL_ADDRESS GhcbBasePa;
+ PHYSICAL_ADDRESS PrevGhcbPa;
UINTN GhcbPageCount;
UINT8 *GhcbBackupBase;
UINT8 *GhcbBackupPages;
@@ -293,8 +294,24 @@ AmdSevEsInitialize (
GhcbRegister (GhcbBasePa);
}
+ PrevGhcbPa = AsmReadMsr64 (MSR_SEV_ES_GHCB);
In general, I don't think this is necessary. There is only one path into
this function and no matter what, the SEC GHCB address must be used.
+
AsmWriteMsr64 (MSR_SEV_ES_GHCB, GhcbBasePa);
+ //
+ // Now that the PEI GHCB is set up, the SEC GHCB page is no longer necessary
+ // to keep shared. Later, it is exposed to the OS as EfiConventionalMemory,
so
+ // it needs to be marked private. The size of the region is hardcoded in
+ // OvmfPkg/ResetVector/ResetVector.nasmb in the definition of
+ // SNP_SEC_MEM_BASE_DESC_2.
+ //
+ ASSERT (PrevGhcbPa == FixedPcdGet32(PcdOvmfSecGhcbBase));
Again, I don't think this is necessary.
+
+ ASSERT_RETURN_ERROR(MemEncryptSevSetPageEncMask(
I find this hard to read. Please be consistent with what is currently done
in the file/function and rename the DecryptStatus variable to just Status
and do:
Status = MemEncryptSevSetPageEncMask (
0,
...
ASSERT_RETURN_ERROR (Status);
(And notice the space between the function name and the opening "(")
+ 0 /*Cr3 -- use system Cr3*/,
Coding standards should have the comma after the 0 and the comment aligned
out to the longest parameter and using //. See other examples in the file.
+ PrevGhcbPa,
Please use FixedPcdGet32(PcdOvmfSecGhcbBase) here.
+ 1 /*Number of pages*/));
The closing ");" must be on a line of its own.
Thanks,
Tom
+
//
// The SEV support will clear the C-bit from non-RAM areas. The early GDT
// lives in a non-RAM area, so when an exception occurs (like a #VC) the GDT
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#97203): https://edk2.groups.io/g/devel/message/97203
Mute This Topic: https://groups.io/mt/95556055/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
