Re: [edk2-devel] [RFC PATCH v3 32/43] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Bypass flash detection with SEV-ES is enabled

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

On 11/21/19 22:11, Tom Lendacky wrote:
> On 11/21/19 6:31 AM, Laszlo Ersek via Groups.Io wrote:
>> On 11/20/19 21:06, Lendacky, Thomas wrote:
>>> 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.
>>
>> (1) Please include the Bugzilla link in the commit message:
>>
>> https://bugzilla.tianocore.org/show_bug.cgi?id=2198
> 
> Yup, will do. I'm also missing the Cc:'s, so I'll add those as well.
> 
>>
>>>
>>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>>> ---
>>>  OvmfPkg/OvmfPkgIa32.dsc                       |  1 +
>>>  OvmfPkg/OvmfPkgIa32X64.dsc                    |  1 +
>>>  OvmfPkg/OvmfPkgX64.dsc                        |  1 +
>>>  .../FvbServicesRuntimeDxe.inf                 |  2 +
>>>  .../QemuFlash.c                               | 38 +++++++++++++++++--
>>>  5 files changed, 40 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
>>> index 56670eefde6b..ff2814c6246e 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 9897e6889573..212952cfaacd 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 59c4f9207fc3..8331fc0b663e 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
>>
>> These hunks look good.
>>
>>> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c
>>> index c81c58972bf2..4f3bf690fcad 100644
>>> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c
>>> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c
>>> @@ -9,7 +9,9 @@
>>>
>>>  #include <Library/BaseMemoryLib.h>
>>>  #include <Library/DebugLib.h>
>>> +#include <Library/MemEncryptSevLib.h>
>>>  #include <Library/PcdLib.h>
>>> +#include <Library/VmgExitLib.h>
>>>
>>>  #include "QemuFlash.h"
>>>
>>> @@ -80,6 +82,20 @@ 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 FD behaves as
>>> +    // a ROM, the nested page table entry is read-only. The check below
>>
>> (2) I suggest a slight change to the wording above. Please replace
>>
>>   when the FD behaves as a ROM
>>
>> with
>>
>>   when the memslot is mapped read-only
>>
>> (It's OK to make qemu / kvm references in this code: the name of the
>> file includes "Qemu".)
> 
> Ok.
> 
>>
>>> +    // will cause a nested page fault that cannot be emulated, causing
>>> +    // the instruction to retried over and over. For SEV-ES, assume that
>>> +    // the FD does not behave as FLASH.
>>
>> (3) I would recommend
>>
>>   For SEV-ES, acknowledge that the FD appears as ROM and not as FLASH,
>>   but report FLASH anyway.
> 
> How about if I expand on this to add "because FLASH behavior can be
> simulated using VMGEXIT.", too.

Sure! Thanks!
Laszlo

> 
>>
>>> +    //
>>> +    DEBUG ((DEBUG_INFO,
>>> +      "QEMU Flash: SEV-ES enabled, assuming FD behaves as FLASH\n"));
>>> +    return TRUE;
>>> +  }
>>> +
>>>    OriginalUint8 = *Ptr;
>>>    *Ptr = CLEAR_STATUS_CMD;
>>>    ProbeUint8 = *Ptr;
>>> @@ -147,6 +163,21 @@ QemuFlashRead (
>>>  }
>>>
>>>
>>> +STATIC
>>> +VOID
>>> +QemuFlashPtrWrite (
>>> +  IN        volatile UINT8                      *Ptr,
>>> +  IN        UINT8                               Value
>>> +  )
>>> +{
>>> +  if (MemEncryptSevEsIsEnabled()) {
>>> +    VmgMmioWrite ((UINT8 *) Ptr, &Value, 1);
>>> +  } else {
>>> +    *Ptr = Value;
>>> +  }
>>> +}
>>> +
>>> +
>>>  /**
>>>    Write to QEMU Flash
>>>
>>
>> (4) This change will break the "FvbServicesSmm.inf" build of this module.
>>
>> (Because, you have -- correctly -- added the VmgExitLib class to
>> "FvbServicesRuntimeDxe.inf" only.)
>>
>> Please:
>>
>> - create two implementations of the QemuFlashPtrWrite() function, one in
>> "QemuFlashDxe.c", and another in "QemuFlashSmm.c".
>>
>> - the former should be the function shown above, the latter should only
>> perform the direct assignment.
>>
>> - the '#include <Library/VmgExitLib.h>' directive should also be moved
>> to "QemuFlashDxe.c".
>>
>> - the prototype for QemuFlashPtrWrite() should be added to "QemuFlash.h"
>> please.
> 
> Ah, nice catch.  I'll do that.
> 
> Thanks,
> Tom
> 
>>
>>> @@ -181,8 +212,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 +222,7 @@ QemuFlashWrite (
>>>    // Restore flash to read mode
>>>    //
>>>    if (*NumBytes > 0) {
>>> -    *(Ptr - 1) = READ_ARRAY_CMD;
>>> +    QemuFlashPtrWrite (Ptr - 1, READ_ARRAY_CMD);
>>>    }
>>>
>>>    return EFI_SUCCESS;
>>>
>>
>> With (1) and (4) fixed, and with (2) and (3) optionally fixed (i.e., if
>> you agree):
>>
>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>
>> Thanks
>> Laszlo
>>
>>
>> 
>>
>