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

["Laszlo Ersek" <lersek@redhat.com>]

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 <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
>  .../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] Ptr    Pointer to the location to write.
> +  @param[in] Value  The value to write.
> +
> +**/
> +VOID
> +QemuFlashPtrWrite (
> +  IN        volatile UINT8    *Ptr,
> +  IN        UINT8             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 <Library/BaseMemoryLib.h>
>  #include <Library/DebugLib.h>
> +#include <Library/MemEncryptSevLib.h>
>  #include <Library/PcdLib.h>
>  
>  #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 <Library/UefiRuntimeLib.h>
> +#include <Library/MemEncryptSevLib.h>
> +#include <Library/VmgExitLib.h>
> +#include <Register/Amd/Msr.h>

- new include directive, most likely due to flattening VmgMmioWrite; OK

>  
>  #include "QemuFlash.h"
>  
> @@ -32,3 +35,40 @@ QemuFlashBeforeProbe (
>    // Do nothing
>    //
>  }
> +
> +/**
> +  Write to QEMU Flash
> +
> +  @param[in] Ptr    Pointer to the location to write.
> +  @param[in] Value  The value to write.
> +
> +**/

- new comment, OK

> +VOID
> +QemuFlashPtrWrite (
> +  IN        volatile UINT8    *Ptr,
> +  IN        UINT8             Value
> +  )
> +{
> +  if (MemEncryptSevEsIsEnabled ()) {
> +    MSR_SEV_ES_GHCB_REGISTER  Msr;
> +    GHCB                      *Ghcb;
> +
> +    Msr.GhcbPhysicalAddress = AsmReadMsr64 (MSR_SEV_ES_GHCB);
> +    Ghcb = Msr.Ghcb;
> +
> +    //
> +    // Writing to flash is emulated by the hypervisor through the use of write
> +    // protection. This won't work for an SEV-ES guest because the write won't
> +    // be recognized as a true MMIO write, which would result in the required
> +    // #VC exception. Instead, use the the VMGEXIT MMIO write support directly
> +    // to perform the update.
> +    //
> +    VmgInit (Ghcb);
> +    Ghcb->SharedBuffer[0] = Value;
> +    Ghcb->SaveArea.SwScratch = (UINT64) (UINTN) Ghcb->SharedBuffer;
> +    VmgExit (Ghcb, SVM_EXIT_MMIO_WRITE, (UINT64) (UINTN) Ptr, 1);
> +    VmgDone (Ghcb);

- this is what replaces VmgMmioWrite(); seems reasonable

> +  } else {
> +    *Ptr = Value;
> +  }
> +}
> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashSmm.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashSmm.c
> index 7eb426e03855..7eb80bfeffae 100644
> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashSmm.c
> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashSmm.c
> @@ -46,3 +46,19 @@ QemuFlashBeforeProbe (
>               );
>    ASSERT_EFI_ERROR (Status);
>  }
> +
> +/**
> +  Write to QEMU Flash
> +
> +  @param[in] Ptr    Pointer to the location to write.
> +  @param[in] Value  The value to write.
> +
> +**/

- new comment.

> +VOID
> +QemuFlashPtrWrite (
> +  IN        volatile UINT8    *Ptr,
> +  IN        UINT8             Value
> +  )
> +{
> +  *Ptr = Value;
> +}
> 

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo