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.lenda...@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 <ler...@redhat.com>
>>
>> Thanks
>> Laszlo
>>
>>
>>
>>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#51187): https://edk2.groups.io/g/devel/message/51187
Mute This Topic: https://groups.io/mt/60973139/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
