* [PATCH v3] OvmfPkg/PlatformPei: Validate SEC's GHCB page
@ 2022-12-09 21:04 Adam Dunlap
2022-12-12 19:29 ` Lendacky, Thomas
0 siblings, 1 reply; 3+ messages in thread
From: Adam Dunlap @ 2022-12-09 21:04 UTC (permalink / raw)
To: devel
Cc: Ard Biesheuvel, Jiewen Yao, Jordan Justen, Gerd Hoffmann,
Brijesh Singh, Erdem Aktas, James Bottomley, Min Xu, Tom Lendacky,
Dionna Glaze, Adam Dunlap
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 byte of each page of memory and checks to see if a #VC
exception is generated indicating that the page was not validated.
Fixes: 6995a1b79bab ("OvmfPkg: Create a GHCB page for use during Sec phase")
Signed-off-by: Adam Dunlap <acdunlap@google.com>
---
Removed the PcdStatus variable and just use Status for all statuses in
this function. Use uncrustify to fix some formatting errors.
OvmfPkg/PlatformPei/AmdSev.c | 40 ++++++++++++++++++++++++------------
1 file changed, 27 insertions(+), 13 deletions(-)
diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c
index e1b9fd9b7f..b2f2f3ac26 100644
--- a/OvmfPkg/PlatformPei/AmdSev.c
+++ b/OvmfPkg/PlatformPei/AmdSev.c
@@ -212,7 +212,7 @@ AmdSevEsInitialize (
UINTN GhcbBackupPageCount;
SEV_ES_PER_CPU_DATA *SevEsData;
UINTN PageCount;
- RETURN_STATUS PcdStatus, DecryptStatus;
+ RETURN_STATUS Status;
IA32_DESCRIPTOR Gdtr;
VOID *Gdt;
@@ -220,8 +220,8 @@ AmdSevEsInitialize (
return;
}
- PcdStatus = PcdSetBoolS (PcdSevEsIsEnabled, TRUE);
- ASSERT_RETURN_ERROR (PcdStatus);
+ Status = PcdSetBoolS (PcdSevEsIsEnabled, TRUE);
+ ASSERT_RETURN_ERROR (Status);
//
// Allocate GHCB and per-CPU variable pages.
@@ -240,20 +240,20 @@ AmdSevEsInitialize (
// only clear the encryption mask for the GHCB pages.
//
for (PageCount = 0; PageCount < GhcbPageCount; PageCount += 2) {
- DecryptStatus = MemEncryptSevClearPageEncMask (
- 0,
- GhcbBasePa + EFI_PAGES_TO_SIZE (PageCount),
- 1
- );
- ASSERT_RETURN_ERROR (DecryptStatus);
+ Status = MemEncryptSevClearPageEncMask (
+ 0,
+ GhcbBasePa + EFI_PAGES_TO_SIZE (PageCount),
+ 1
+ );
+ ASSERT_RETURN_ERROR (Status);
}
ZeroMem (GhcbBase, EFI_PAGES_TO_SIZE (GhcbPageCount));
- PcdStatus = PcdSet64S (PcdGhcbBase, GhcbBasePa);
- ASSERT_RETURN_ERROR (PcdStatus);
- PcdStatus = PcdSet64S (PcdGhcbSize, EFI_PAGES_TO_SIZE (GhcbPageCount));
- ASSERT_RETURN_ERROR (PcdStatus);
+ Status = PcdSet64S (PcdGhcbBase, GhcbBasePa);
+ ASSERT_RETURN_ERROR (Status);
+ Status = PcdSet64S (PcdGhcbSize, EFI_PAGES_TO_SIZE (GhcbPageCount));
+ ASSERT_RETURN_ERROR (Status);
DEBUG ((
DEBUG_INFO,
@@ -295,6 +295,20 @@ AmdSevEsInitialize (
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.
+ //
+ Status = MemEncryptSevSetPageEncMask (
+ 0, // Cr3 -- use system Cr3
+ FixedPcdGet32 (PcdOvmfSecGhcbBase), // BaseAddress
+ 1 // NumPages
+ );
+ ASSERT_RETURN_ERROR (Status);
+
//
// 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
--
2.39.0.rc1.256.g54fd8350bd-goog
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH v3] OvmfPkg/PlatformPei: Validate SEC's GHCB page
2022-12-09 21:04 [PATCH v3] OvmfPkg/PlatformPei: Validate SEC's GHCB page Adam Dunlap
@ 2022-12-12 19:29 ` Lendacky, Thomas
2022-12-15 8:10 ` Yao, Jiewen
0 siblings, 1 reply; 3+ messages in thread
From: Lendacky, Thomas @ 2022-12-12 19:29 UTC (permalink / raw)
To: Adam Dunlap, devel
Cc: Ard Biesheuvel, Jiewen Yao, Jordan Justen, Gerd Hoffmann,
Brijesh Singh, Erdem Aktas, James Bottomley, Min Xu, Dionna Glaze
On 12/9/22 15:04, Adam Dunlap 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 byte of each page of memory and checks to see if a #VC
> exception is generated indicating that the page was not validated.
>
> Fixes: 6995a1b79bab ("OvmfPkg: Create a GHCB page for use during Sec phase")
>
> Signed-off-by: Adam Dunlap <acdunlap@google.com>
Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
>
> Removed the PcdStatus variable and just use Status for all statuses in
> this function. Use uncrustify to fix some formatting errors.
>
> OvmfPkg/PlatformPei/AmdSev.c | 40 ++++++++++++++++++++++++------------
> 1 file changed, 27 insertions(+), 13 deletions(-)
>
> diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c
> index e1b9fd9b7f..b2f2f3ac26 100644
> --- a/OvmfPkg/PlatformPei/AmdSev.c
> +++ b/OvmfPkg/PlatformPei/AmdSev.c
> @@ -212,7 +212,7 @@ AmdSevEsInitialize (
> UINTN GhcbBackupPageCount;
> SEV_ES_PER_CPU_DATA *SevEsData;
> UINTN PageCount;
> - RETURN_STATUS PcdStatus, DecryptStatus;
> + RETURN_STATUS Status;
> IA32_DESCRIPTOR Gdtr;
> VOID *Gdt;
>
> @@ -220,8 +220,8 @@ AmdSevEsInitialize (
> return;
> }
>
> - PcdStatus = PcdSetBoolS (PcdSevEsIsEnabled, TRUE);
> - ASSERT_RETURN_ERROR (PcdStatus);
> + Status = PcdSetBoolS (PcdSevEsIsEnabled, TRUE);
> + ASSERT_RETURN_ERROR (Status);
>
> //
> // Allocate GHCB and per-CPU variable pages.
> @@ -240,20 +240,20 @@ AmdSevEsInitialize (
> // only clear the encryption mask for the GHCB pages.
> //
> for (PageCount = 0; PageCount < GhcbPageCount; PageCount += 2) {
> - DecryptStatus = MemEncryptSevClearPageEncMask (
> - 0,
> - GhcbBasePa + EFI_PAGES_TO_SIZE (PageCount),
> - 1
> - );
> - ASSERT_RETURN_ERROR (DecryptStatus);
> + Status = MemEncryptSevClearPageEncMask (
> + 0,
> + GhcbBasePa + EFI_PAGES_TO_SIZE (PageCount),
> + 1
> + );
> + ASSERT_RETURN_ERROR (Status);
> }
>
> ZeroMem (GhcbBase, EFI_PAGES_TO_SIZE (GhcbPageCount));
>
> - PcdStatus = PcdSet64S (PcdGhcbBase, GhcbBasePa);
> - ASSERT_RETURN_ERROR (PcdStatus);
> - PcdStatus = PcdSet64S (PcdGhcbSize, EFI_PAGES_TO_SIZE (GhcbPageCount));
> - ASSERT_RETURN_ERROR (PcdStatus);
> + Status = PcdSet64S (PcdGhcbBase, GhcbBasePa);
> + ASSERT_RETURN_ERROR (Status);
> + Status = PcdSet64S (PcdGhcbSize, EFI_PAGES_TO_SIZE (GhcbPageCount));
> + ASSERT_RETURN_ERROR (Status);
>
> DEBUG ((
> DEBUG_INFO,
> @@ -295,6 +295,20 @@ AmdSevEsInitialize (
>
> 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.
> + //
> + Status = MemEncryptSevSetPageEncMask (
> + 0, // Cr3 -- use system Cr3
> + FixedPcdGet32 (PcdOvmfSecGhcbBase), // BaseAddress
> + 1 // NumPages
> + );
> + ASSERT_RETURN_ERROR (Status);
> +
> //
> // 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
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH v3] OvmfPkg/PlatformPei: Validate SEC's GHCB page
2022-12-12 19:29 ` Lendacky, Thomas
@ 2022-12-15 8:10 ` Yao, Jiewen
0 siblings, 0 replies; 3+ messages in thread
From: Yao, Jiewen @ 2022-12-15 8:10 UTC (permalink / raw)
To: Tom Lendacky, Adam Dunlap, devel@edk2.groups.io
Cc: Ard Biesheuvel, Justen, Jordan L, Gerd Hoffmann, Brijesh Singh,
Aktas, Erdem, James Bottomley, Xu, Min M, Dionna Glaze
Merged https://github.com/tianocore/edk2/pull/3774
> -----Original Message-----
> From: Tom Lendacky <thomas.lendacky@amd.com>
> Sent: Tuesday, December 13, 2022 3:30 AM
> To: Adam Dunlap <acdunlap@google.com>; devel@edk2.groups.io
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>; Yao, Jiewen
> <jiewen.yao@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; Gerd
> Hoffmann <kraxel@redhat.com>; Brijesh Singh <brijesh.singh@amd.com>;
> Aktas, Erdem <erdemaktas@google.com>; James Bottomley
> <jejb@linux.ibm.com>; Xu, Min M <min.m.xu@intel.com>; Dionna Glaze
> <dionnaglaze@google.com>
> Subject: Re: [PATCH v3] OvmfPkg/PlatformPei: Validate SEC's GHCB page
>
> On 12/9/22 15:04, Adam Dunlap 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 byte of each page of memory and checks to see if a #VC
> > exception is generated indicating that the page was not validated.
> >
> > Fixes: 6995a1b79bab ("OvmfPkg: Create a GHCB page for use during Sec
> phase")
> >
> > Signed-off-by: Adam Dunlap <acdunlap@google.com>
>
> Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
>
> > ---
> >
> > Removed the PcdStatus variable and just use Status for all statuses in
> > this function. Use uncrustify to fix some formatting errors.
> >
> > OvmfPkg/PlatformPei/AmdSev.c | 40 ++++++++++++++++++++++++----------
> --
> > 1 file changed, 27 insertions(+), 13 deletions(-)
> >
> > diff --git a/OvmfPkg/PlatformPei/AmdSev.c
> b/OvmfPkg/PlatformPei/AmdSev.c
> > index e1b9fd9b7f..b2f2f3ac26 100644
> > --- a/OvmfPkg/PlatformPei/AmdSev.c
> > +++ b/OvmfPkg/PlatformPei/AmdSev.c
> > @@ -212,7 +212,7 @@ AmdSevEsInitialize (
> > UINTN GhcbBackupPageCount;
> > SEV_ES_PER_CPU_DATA *SevEsData;
> > UINTN PageCount;
> > - RETURN_STATUS PcdStatus, DecryptStatus;
> > + RETURN_STATUS Status;
> > IA32_DESCRIPTOR Gdtr;
> > VOID *Gdt;
> >
> > @@ -220,8 +220,8 @@ AmdSevEsInitialize (
> > return;
> > }
> >
> > - PcdStatus = PcdSetBoolS (PcdSevEsIsEnabled, TRUE);
> > - ASSERT_RETURN_ERROR (PcdStatus);
> > + Status = PcdSetBoolS (PcdSevEsIsEnabled, TRUE);
> > + ASSERT_RETURN_ERROR (Status);
> >
> > //
> > // Allocate GHCB and per-CPU variable pages.
> > @@ -240,20 +240,20 @@ AmdSevEsInitialize (
> > // only clear the encryption mask for the GHCB pages.
> > //
> > for (PageCount = 0; PageCount < GhcbPageCount; PageCount += 2) {
> > - DecryptStatus = MemEncryptSevClearPageEncMask (
> > - 0,
> > - GhcbBasePa + EFI_PAGES_TO_SIZE (PageCount),
> > - 1
> > - );
> > - ASSERT_RETURN_ERROR (DecryptStatus);
> > + Status = MemEncryptSevClearPageEncMask (
> > + 0,
> > + GhcbBasePa + EFI_PAGES_TO_SIZE (PageCount),
> > + 1
> > + );
> > + ASSERT_RETURN_ERROR (Status);
> > }
> >
> > ZeroMem (GhcbBase, EFI_PAGES_TO_SIZE (GhcbPageCount));
> >
> > - PcdStatus = PcdSet64S (PcdGhcbBase, GhcbBasePa);
> > - ASSERT_RETURN_ERROR (PcdStatus);
> > - PcdStatus = PcdSet64S (PcdGhcbSize, EFI_PAGES_TO_SIZE
> (GhcbPageCount));
> > - ASSERT_RETURN_ERROR (PcdStatus);
> > + Status = PcdSet64S (PcdGhcbBase, GhcbBasePa);
> > + ASSERT_RETURN_ERROR (Status);
> > + Status = PcdSet64S (PcdGhcbSize, EFI_PAGES_TO_SIZE (GhcbPageCount));
> > + ASSERT_RETURN_ERROR (Status);
> >
> > DEBUG ((
> > DEBUG_INFO,
> > @@ -295,6 +295,20 @@ AmdSevEsInitialize (
> >
> > 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.
> > + //
> > + Status = MemEncryptSevSetPageEncMask (
> > + 0, // Cr3 -- use system Cr3
> > + FixedPcdGet32 (PcdOvmfSecGhcbBase), // BaseAddress
> > + 1 // NumPages
> > + );
> > + ASSERT_RETURN_ERROR (Status);
> > +
> > //
> > // 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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-12-15 8:10 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-12-09 21:04 [PATCH v3] OvmfPkg/PlatformPei: Validate SEC's GHCB page Adam Dunlap
2022-12-12 19:29 ` Lendacky, Thomas
2022-12-15 8:10 ` Yao, Jiewen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox