Re: [edk2-devel] [PATCH v4 34/40] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Bypass flash detection with SEV-ES is enabled

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

On 02/05/20 00:01, 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 VmgMmioWrite function from the VmgExitLib library 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>
> ---
>  OvmfPkg/OvmfPkgIa32.dsc                       |  1 +
>  OvmfPkg/OvmfPkgIa32X64.dsc                    |  1 +
>  OvmfPkg/OvmfPkgX64.dsc                        |  1 +

I think the lib class resolutions can be dropped if you centralize one
resolution per DSC file under [LibraryClasses] (see my comment under v4
08/40), in an earlier OvmfPkg patch.

Looks nice otherwise, so please do keep the R-b.

Thanks
Laszlo

>  .../FvbServicesRuntimeDxe.inf                 |  2 ++
>  .../QemuFlash.h                               |  6 +++++
>  .../QemuFlash.c                               | 23 ++++++++++++++++---
>  .../QemuFlashDxe.c                            | 15 ++++++++++++
>  .../QemuFlashSmm.c                            |  9 ++++++++
>  8 files changed, 55 insertions(+), 3 deletions(-)
> 
> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
> index c364da521579..ac67fc1bd8b8 100644
> --- a/OvmfPkg/OvmfPkgIa32.dsc
> +++ b/OvmfPkg/OvmfPkgIa32.dsc
> @@ -320,6 +320,7 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER]
>    BaseCryptLib|CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
>    PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
>    QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf
> +  VmgExitLib|UefiCpuPkg/Library/VmgExitLib/VmgExitLib.inf
>  
>  [LibraryClasses.common.UEFI_DRIVER]
>    PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> index 0adae1e0be30..ecf5c2ad54a3 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> @@ -325,6 +325,7 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER]
>    BaseCryptLib|CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
>    PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
>    QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf
> +  VmgExitLib|UefiCpuPkg/Library/VmgExitLib/VmgExitLib.inf
>  
>  [LibraryClasses.common.UEFI_DRIVER]
>    PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index 4b5e96a32b0b..33f7571d4df0 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -325,6 +325,7 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER]
>    BaseCryptLib|CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
>    PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
>    QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf
> +  VmgExitLib|UefiCpuPkg/Library/VmgExitLib/VmgExitLib.inf
>  
>  [LibraryClasses.common.UEFI_DRIVER]
>    PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
> index ca6326e833ed..0b7741ac07f8 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..19ac1f733279 100644
> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.h
> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.h
> @@ -89,5 +89,11 @@ QemuFlashBeforeProbe (
>    IN  UINTN                   FdBlockCount
>    );
>  
> +VOID
> +QemuFlashPtrWrite (
> +  IN        volatile UINT8    *Ptr,
> +  IN        UINT8             Value
> +  );
> +
>  #endif
>  
> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c
> index c81c58972bf2..ccf5ad7f7afb 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 ((EFI_D_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..939463a8e17c 100644
> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c
> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c
> @@ -10,6 +10,8 @@
>  **/
>  
>  #include <Library/UefiRuntimeLib.h>
> +#include <Library/MemEncryptSevLib.h>
> +#include <Library/VmgExitLib.h>
>  
>  #include "QemuFlash.h"
>  
> @@ -32,3 +34,16 @@ QemuFlashBeforeProbe (
>    // Do nothing
>    //
>  }
> +
> +VOID
> +QemuFlashPtrWrite (
> +  IN        volatile UINT8    *Ptr,
> +  IN        UINT8             Value
> +  )
> +{
> +  if (MemEncryptSevEsIsEnabled()) {
> +    VmgMmioWrite ((UINT8 *) Ptr, &Value, 1);
> +  } else {
> +    *Ptr = Value;
> +  }
> +}
> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashSmm.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashSmm.c
> index 7eb426e03855..eff40ae28032 100644
> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashSmm.c
> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashSmm.c
> @@ -46,3 +46,12 @@ QemuFlashBeforeProbe (
>               );
>    ASSERT_EFI_ERROR (Status);
>  }
> +
> +VOID
> +QemuFlashPtrWrite (
> +  IN        volatile UINT8    *Ptr,
> +  IN        UINT8             Value
> +  )
> +{
> +  *Ptr = Value;
> +}
>