Re: [edk2-devel] [PATCH v8 39/46] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Bypass flash detection with SEV-ES

2020-05-26 Thread Laszlo Ersek
On 05/19/20 23:51, Lendacky, Thomas wrote:
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2198
> 
> The flash detection routine will attempt to determine how the flash
> device behaves (e.g. ROM, RAM, Flash). But when SEV-ES is enabled and
> the flash device behaves as a ROM device (meaning it is marked read-only
> by the hypervisor), this check may result in an infinite nested page fault
> because of the attempted write. Since the instruction cannot be emulated
> when SEV-ES is enabled, the RIP is never advanced, resulting in repeated
> nested page faults.
> 
> When SEV-ES is enabled, exit the flash detection early and assume that
> the FD behaves as Flash. This will result in QemuFlashWrite() being called
> to store EFI variables, which will also result in an infinite nested page
> fault when the write is performed. In this case, update QemuFlashWrite()
> to use the VMGEXIT MMIO write support to have the hypervisor perform the
> write without having to emulate the instruction.
> 
> Cc: Jordan Justen 
> Cc: Laszlo Ersek 
> Cc: Ard Biesheuvel 
> Reviewed-by: Laszlo Ersek 
> Signed-off-by: Tom Lendacky 
> ---
>  .../FvbServicesRuntimeDxe.inf |  2 +
>  .../QemuFlash.h   | 13 ++
>  .../QemuFlash.c   | 23 +--
>  .../QemuFlashDxe.c| 40 +++
>  .../QemuFlashSmm.c| 16 
>  5 files changed, 91 insertions(+), 3 deletions(-)

- subject line has been cleaned up relative to v6, OK

- commit message updated in sync with open-coding VmgMmioWrite() in this
patch; and VmgMmioWrite() has been documented in the v8 blurb -- OK

> 
> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf 
> b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
> index 72cabba4357d..8bb2325157ea 100644
> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
> @@ -38,6 +38,7 @@ [Sources]
>  [Packages]
>MdePkg/MdePkg.dec
>MdeModulePkg/MdeModulePkg.dec
> +  UefiCpuPkg/UefiCpuPkg.dec
>OvmfPkg/OvmfPkg.dec
>  
>  [LibraryClasses]
> @@ -52,6 +53,7 @@ [LibraryClasses]
>UefiBootServicesTableLib
>UefiDriverEntryPoint
>UefiRuntimeLib
> +  VmgExitLib
>  
>  [Guids]
>gEfiEventVirtualAddressChangeGuid   # ALWAYS_CONSUMED
> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.h 
> b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.h
> index f1afabcbe6ae..219d0d6e83cf 100644
> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.h
> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.h
> @@ -89,5 +89,18 @@ QemuFlashBeforeProbe (
>IN  UINTN   FdBlockCount
>);
>  
> +/**
> +  Write to QEMU Flash
> +
> +  @param[in] PtrPointer to the location to write.
> +  @param[in] Value  The value to write.
> +
> +**/
> +VOID
> +QemuFlashPtrWrite (
> +  INvolatile UINT8*Ptr,
> +  INUINT8 Value
> +  );
> +
>  #endif
>  

- New comment, OK.

> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c 
> b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c
> index 1b0d6c053f1a..0d29bf701aca 100644
> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c
> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c
> @@ -9,6 +9,7 @@
>  
>  #include 
>  #include 
> +#include 
>  #include 
>  
>  #include "QemuFlash.h"
> @@ -80,6 +81,21 @@ QemuFlashDetected (
>  
>DEBUG ((DEBUG_INFO, "QEMU Flash: Attempting flash detection at %p\n", 
> Ptr));
>  
> +  if (MemEncryptSevEsIsEnabled ()) {
> +//
> +// When SEV-ES is enabled, the check below can result in an infinite
> +// loop with respect to a nested page fault. When the memslot is mapped
> +// read-only, the nested page table entry is read-only. The check below
> +// will cause a nested page fault that cannot be emulated, causing
> +// the instruction to retried over and over. For SEV-ES, acknowledge that
> +// the FD appears as ROM and not as FLASH, but report FLASH anyway 
> because
> +// FLASH behavior can be simulated using VMGEXIT.
> +//
> +DEBUG ((DEBUG_INFO,
> +  "QEMU Flash: SEV-ES enabled, assuming FD behaves as FLASH\n"));
> +return TRUE;
> +  }
> +
>OriginalUint8 = *Ptr;
>*Ptr = CLEAR_STATUS_CMD;
>ProbeUint8 = *Ptr;
> @@ -181,8 +197,9 @@ QemuFlashWrite (
>//
>Ptr = QemuFlashPtr (Lba, Offset);
>for (Loop = 0; Loop < *NumBytes; Loop++) {
> -*Ptr = WRITE_BYTE_CMD;
> -*Ptr = Buffer[Loop];
> +QemuFlashPtrWrite (Ptr, WRITE_BYTE_CMD);
> +QemuFlashPtrWrite (Ptr, Buffer[Loop]);
> +
>  Ptr++;
>}
>  
> @@ -190,7 +207,7 @@ QemuFlashWrite (
>// Restore flash to read mode
>//
>if (*NumBytes > 0) {
> -*(Ptr - 1) = READ_ARRAY_CMD;
> +QemuFlashPtrWrite (Ptr - 1, READ_ARRAY_CMD);
>}
>  
>return EFI_SUCCESS;
> diff --git 

[edk2-devel] [PATCH v8 39/46] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Bypass flash detection with SEV-ES

2020-05-19 Thread Lendacky, Thomas
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2198

The flash detection routine will attempt to determine how the flash
device behaves (e.g. ROM, RAM, Flash). But when SEV-ES is enabled and
the flash device behaves as a ROM device (meaning it is marked read-only
by the hypervisor), this check may result in an infinite nested page fault
because of the attempted write. Since the instruction cannot be emulated
when SEV-ES is enabled, the RIP is never advanced, resulting in repeated
nested page faults.

When SEV-ES is enabled, exit the flash detection early and assume that
the FD behaves as Flash. This will result in QemuFlashWrite() being called
to store EFI variables, which will also result in an infinite nested page
fault when the write is performed. In this case, update QemuFlashWrite()
to use the VMGEXIT MMIO write support to have the hypervisor perform the
write without having to emulate the instruction.

Cc: Jordan Justen 
Cc: Laszlo Ersek 
Cc: Ard Biesheuvel 
Reviewed-by: Laszlo Ersek 
Signed-off-by: Tom Lendacky 
---
 .../FvbServicesRuntimeDxe.inf |  2 +
 .../QemuFlash.h   | 13 ++
 .../QemuFlash.c   | 23 +--
 .../QemuFlashDxe.c| 40 +++
 .../QemuFlashSmm.c| 16 
 5 files changed, 91 insertions(+), 3 deletions(-)

diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf 
b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
index 72cabba4357d..8bb2325157ea 100644
--- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
+++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
@@ -38,6 +38,7 @@ [Sources]
 [Packages]
   MdePkg/MdePkg.dec
   MdeModulePkg/MdeModulePkg.dec
+  UefiCpuPkg/UefiCpuPkg.dec
   OvmfPkg/OvmfPkg.dec
 
 [LibraryClasses]
@@ -52,6 +53,7 @@ [LibraryClasses]
   UefiBootServicesTableLib
   UefiDriverEntryPoint
   UefiRuntimeLib
+  VmgExitLib
 
 [Guids]
   gEfiEventVirtualAddressChangeGuid   # ALWAYS_CONSUMED
diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.h 
b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.h
index f1afabcbe6ae..219d0d6e83cf 100644
--- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.h
+++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.h
@@ -89,5 +89,18 @@ QemuFlashBeforeProbe (
   IN  UINTN   FdBlockCount
   );
 
+/**
+  Write to QEMU Flash
+
+  @param[in] PtrPointer to the location to write.
+  @param[in] Value  The value to write.
+
+**/
+VOID
+QemuFlashPtrWrite (
+  INvolatile UINT8*Ptr,
+  INUINT8 Value
+  );
+
 #endif
 
diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c 
b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c
index 1b0d6c053f1a..0d29bf701aca 100644
--- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c
+++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c
@@ -9,6 +9,7 @@
 
 #include 
 #include 
+#include 
 #include 
 
 #include "QemuFlash.h"
@@ -80,6 +81,21 @@ QemuFlashDetected (
 
   DEBUG ((DEBUG_INFO, "QEMU Flash: Attempting flash detection at %p\n", Ptr));
 
+  if (MemEncryptSevEsIsEnabled ()) {
+//
+// When SEV-ES is enabled, the check below can result in an infinite
+// loop with respect to a nested page fault. When the memslot is mapped
+// read-only, the nested page table entry is read-only. The check below
+// will cause a nested page fault that cannot be emulated, causing
+// the instruction to retried over and over. For SEV-ES, acknowledge that
+// the FD appears as ROM and not as FLASH, but report FLASH anyway because
+// FLASH behavior can be simulated using VMGEXIT.
+//
+DEBUG ((DEBUG_INFO,
+  "QEMU Flash: SEV-ES enabled, assuming FD behaves as FLASH\n"));
+return TRUE;
+  }
+
   OriginalUint8 = *Ptr;
   *Ptr = CLEAR_STATUS_CMD;
   ProbeUint8 = *Ptr;
@@ -181,8 +197,9 @@ QemuFlashWrite (
   //
   Ptr = QemuFlashPtr (Lba, Offset);
   for (Loop = 0; Loop < *NumBytes; Loop++) {
-*Ptr = WRITE_BYTE_CMD;
-*Ptr = Buffer[Loop];
+QemuFlashPtrWrite (Ptr, WRITE_BYTE_CMD);
+QemuFlashPtrWrite (Ptr, Buffer[Loop]);
+
 Ptr++;
   }
 
@@ -190,7 +207,7 @@ QemuFlashWrite (
   // Restore flash to read mode
   //
   if (*NumBytes > 0) {
-*(Ptr - 1) = READ_ARRAY_CMD;
+QemuFlashPtrWrite (Ptr - 1, READ_ARRAY_CMD);
   }
 
   return EFI_SUCCESS;
diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c 
b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c
index 5aabe9d7b59c..565383ee26d2 100644
--- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c
+++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c
@@ -10,6 +10,9 @@
 **/
 
 #include 
+#include 
+#include 
+#include 
 
 #include "QemuFlash.h"
 
@@ -32,3 +35,40 @@ QemuFlashBeforeProbe (
   // Do nothing
   //
 }
+
+/**
+  Write to QEMU Flash
+
+  @param[in] PtrPointer to the location to write.
+  @par