public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH] OvmfPkg/PlatformPei: Validate SEC's GHCB page
@ 2022-12-08 23:43 acdunlap
  2022-12-09 16:41 ` [edk2-devel] " Lendacky, Thomas
  2022-12-09 20:08 ` [edk2-devel] [PATCH] " Ard Biesheuvel
  0 siblings, 2 replies; 6+ messages in thread
From: acdunlap @ 2022-12-08 23:43 UTC (permalink / raw)
  To: devel; +Cc: 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 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 <acdunlap@google.com>
---
 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);
+
   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));
+
+  ASSERT_RETURN_ERROR(MemEncryptSevSetPageEncMask(
+    0 /*Cr3 -- use system Cr3*/,
+    PrevGhcbPa,
+    1 /*Number of pages*/));
+
   //
   // 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] 6+ messages in thread

* Re: [edk2-devel] [PATCH] OvmfPkg/PlatformPei: Validate SEC's GHCB page
  2022-12-08 23:43 [PATCH] OvmfPkg/PlatformPei: Validate SEC's GHCB page acdunlap
@ 2022-12-09 16:41 ` Lendacky, Thomas
  2022-12-09 18:58   ` [PATCH v2] " Adam Dunlap
  2022-12-09 20:08 ` [edk2-devel] [PATCH] " Ard Biesheuvel
  1 sibling, 1 reply; 6+ messages in thread
From: Lendacky, Thomas @ 2022-12-09 16:41 UTC (permalink / raw)
  To: devel, acdunlap

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 <acdunlap@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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v2] OvmfPkg/PlatformPei: Validate SEC's GHCB page
  2022-12-09 16:41 ` [edk2-devel] " Lendacky, Thomas
@ 2022-12-09 18:58   ` Adam Dunlap
  2022-12-09 20:06     ` Lendacky, Thomas
  0 siblings, 1 reply; 6+ messages in thread
From: Adam Dunlap @ 2022-12-09 18:58 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>
---
 OvmfPkg/PlatformPei/AmdSev.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c
index e1b9fd9b7f..df560a8679 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        PcdStatus, Status;
   IA32_DESCRIPTOR      Gdtr;
   VOID                 *Gdt;
 
@@ -240,12 +240,12 @@ AmdSevEsInitialize (
   // only clear the encryption mask for the GHCB pages.
   //
   for (PageCount = 0; PageCount < GhcbPageCount; PageCount += 2) {
-    DecryptStatus = MemEncryptSevClearPageEncMask (
+    Status = MemEncryptSevClearPageEncMask (
                       0,
                       GhcbBasePa + EFI_PAGES_TO_SIZE (PageCount),
                       1
                       );
-    ASSERT_RETURN_ERROR (DecryptStatus);
+    ASSERT_RETURN_ERROR (Status);
   }
 
   ZeroMem (GhcbBase, EFI_PAGES_TO_SIZE (GhcbPageCount));
@@ -295,6 +295,21 @@ 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),
+    1                                   // Number of pages
+    );
+  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] 6+ messages in thread

* Re: [PATCH v2] OvmfPkg/PlatformPei: Validate SEC's GHCB page
  2022-12-09 18:58   ` [PATCH v2] " Adam Dunlap
@ 2022-12-09 20:06     ` Lendacky, Thomas
  2022-12-12  6:51       ` [edk2-devel] " Gerd Hoffmann
  0 siblings, 1 reply; 6+ messages in thread
From: Lendacky, Thomas @ 2022-12-09 20:06 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 12:58, 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>
> ---
>   OvmfPkg/PlatformPei/AmdSev.c | 21 ++++++++++++++++++---
>   1 file changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c
> index e1b9fd9b7f..df560a8679 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        PcdStatus, Status;

I'm not sure if the CI will complain in this case, but it doesn't allow 
multiple variable definitions in a line. And with the change I suggested, 
this will now likely trigger a CI failure (you can check by submitting a 
pull request to the EDK2 project, which will run the CI but not actually 
commit anything, and seeing if you encounter any issues).

>     IA32_DESCRIPTOR      Gdtr;
>     VOID                 *Gdt;
>   
> @@ -240,12 +240,12 @@ AmdSevEsInitialize (
>     // only clear the encryption mask for the GHCB pages.
>     //
>     for (PageCount = 0; PageCount < GhcbPageCount; PageCount += 2) {
> -    DecryptStatus = MemEncryptSevClearPageEncMask (
> +    Status = MemEncryptSevClearPageEncMask (
>                         0,
>                         GhcbBasePa + EFI_PAGES_TO_SIZE (PageCount),
>                         1
>                         );

You'll have to re-align the arguments under the lower-case "m" now.

> -    ASSERT_RETURN_ERROR (DecryptStatus);
> +    ASSERT_RETURN_ERROR (Status);
>     }
>   
>     ZeroMem (GhcbBase, EFI_PAGES_TO_SIZE (GhcbPageCount));
> @@ -295,6 +295,21 @@ 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.
> +  //
> +

Remove the blank line here.

> +  Status = MemEncryptSevSetPageEncMask (
> +    0,                                  // Cr3 -- use system Cr3
> +    FixedPcdGet32 (PcdOvmfSecGhcbBase),
> +    1                                   // Number of pages
> +    );

These arguments need to be aligned under the lower case "m", too.

Thanks,
Tom

> +  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] 6+ messages in thread

* Re: [edk2-devel] [PATCH] OvmfPkg/PlatformPei: Validate SEC's GHCB page
  2022-12-08 23:43 [PATCH] OvmfPkg/PlatformPei: Validate SEC's GHCB page acdunlap
  2022-12-09 16:41 ` [edk2-devel] " Lendacky, Thomas
@ 2022-12-09 20:08 ` Ard Biesheuvel
  1 sibling, 0 replies; 6+ messages in thread
From: Ard Biesheuvel @ 2022-12-09 20:08 UTC (permalink / raw)
  To: devel, acdunlap

On Fri, 9 Dec 2022 at 07:41, Adam Dunlap via groups.io
<acdunlap=google.com@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 <acdunlap@google.com>
> ---
>  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);
> +
>    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));
> +
> +  ASSERT_RETURN_ERROR(MemEncryptSevSetPageEncMask(
> +    0 /*Cr3 -- use system Cr3*/,
> +    PrevGhcbPa,
> +    1 /*Number of pages*/));
> +

What happens to this call when ASSERT()s are compiled out?

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [edk2-devel] [PATCH v2] OvmfPkg/PlatformPei: Validate SEC's GHCB page
  2022-12-09 20:06     ` Lendacky, Thomas
@ 2022-12-12  6:51       ` Gerd Hoffmann
  0 siblings, 0 replies; 6+ messages in thread
From: Gerd Hoffmann @ 2022-12-12  6:51 UTC (permalink / raw)
  To: devel, thomas.lendacky
  Cc: Adam Dunlap, Ard Biesheuvel, Jiewen Yao, Jordan Justen,
	Brijesh Singh, Erdem Aktas, James Bottomley, Min Xu, Dionna Glaze

  Hi,

> > +  Status = MemEncryptSevSetPageEncMask (
> > +    0,                                  // Cr3 -- use system Cr3
> > +    FixedPcdGet32 (PcdOvmfSecGhcbBase),
> > +    1                                   // Number of pages
> > +    );
> 
> These arguments need to be aligned under the lower case "m", too.

Best way to deal with this and the other code style issues is to install
uncrustify locally and let it format the source code correctly.

take care,
  Gerd


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2022-12-12  6:51 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-12-08 23:43 [PATCH] OvmfPkg/PlatformPei: Validate SEC's GHCB page acdunlap
2022-12-09 16:41 ` [edk2-devel] " Lendacky, Thomas
2022-12-09 18:58   ` [PATCH v2] " Adam Dunlap
2022-12-09 20:06     ` Lendacky, Thomas
2022-12-12  6:51       ` [edk2-devel] " Gerd Hoffmann
2022-12-09 20:08 ` [edk2-devel] [PATCH] " Ard Biesheuvel

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox