Re: [RFC PATCH 1/1] OvmfPkg/QemuFlash: Fix Runtime variable access when SEV is enabled

[Laszlo Ersek <lersek@redhat.com>]

On 06/26/18 21:46, Brijesh Singh wrote:
> Problem statement:
> ------------------
> Fedora-28 contains 4.16 kernel -- which has all the required support to
> run as an SEV guest.  When the installer is launched from SEV guest then
> it fails to install the bootloader. The installer was failing to update
> the 'BootOrder' UEFI runtime variable.
> 
> Root Cause Analysis
> --------------------
> Since QemuFlash storage memory is accessed by both guest and hypervisor
> hence we need to map this memory range as unencrypted. AmdSevDxe maps the
> range as "unencrypted" but later FtwNotificationEvent() in
> MdeModule/Universal/Variable/RuntimeDxe/VariableDxe.c resets the mapping
> and the memory region gets remapped as "encrypted".

Is this a new issue, or has it always been there, and we just failed to
notice it?

BTW, I don't understand why FtwNotificationEvent() in
"MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c" has to mark
the flash range as EFI_MEMORY_RUNTIME. I thought that this action
belonged in the flash driver itself, and we do that already in
MarkMemoryRangeForRuntimeAccess(), in file
"OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c".

The variable driver does not own the pflash chip (the pflash driver owns
it), so I believe the variable driver shouldn't mess with the mapping
attributes.

Here's a suggestion -- Star, Eric, can you please comment? In the
FtwNotificationEvent() function, after we get the memory descriptor for
the pflash range, first check whether EFI_MEMORY_RUNTIME is already set.
If it is, don't do anything; if it isn't, add the attribute.

This should cause no observable change on any non-SEV platform, and it
should remove the gDS->SetMemorySpaceAttributes() call for OVMF (where
it breaks things for SEV).

> After that, any access
> to the flash will end up going through the encryption engine. I did try
> hacking EDK2 to restore the C-bit

(I continue to be annoyed that the memory encryption bit is not exposed
in the GCD memory space attributes explicitly.)

> but that was not sufficient because UEFI
> runtime services are mapped as "encrypted" in OS page table

What do you mean here? Runtime services *code* or runtime services
*data*? Code must obviously be remain encrypted (otherwise we cannot
execute it in SEV). Runtime Services Data should also be mapped as
encrypted (it is normal RAM that is not used for guest<->hypervisor
exchange).

> hence we end up accessing the flash as encrypted when OS requests to update the variables.

I don't understand the "hence" here; I don't see how the implication
follows. runtime services code and data should be encrypted. Runtime
MMIO should be un-encrypted.

Ohh, wait, in MarkMemoryRangeForRuntimeAccess(), we use
"EfiGcdMemoryTypeSystemMemory". I don't have a clue why that is a good
idea. That should have been EfiGcdMemoryTypeMemoryMappedIo.

... Anyway, I think first we should go with the "check
EFI_MEMORY_RUNTIME attribute before setting it", in FtwNotificationEvent().

> 
> A possible solution
> ---------------------
> To solve the issue, after QemuFlash is probed, I allocate an encrypted
> buffer and initialize this buffer with the contents from the flash memory.
> When SEV is enabled, we use newly allocated encrypted buffer in
> FwInstance->FvBase instead of the original flash region. The idea is if
> caller grabs the FwInstance->FvBase pointer and tries to access the
> FvVolumeHeader then it should get the data from the encrypted buffer.
> But if caller wants read/writes to/from the flash device then we internally
> use the original "unencrypted" flash region to access the data.

No, this is neither safe, nor a desirable design.

Safety: all accesses (via both pointers and FVB protocol members) that
higher-level drives *think* go to the pflash chip must *actually* go to
the pflash chip.

Design: it had taken us years to get rid of various memory-emulated fake
variable stores. They *all* suck in one way or another, with various
obscure UEFI spec incompatibilities and corner cases. A strictly
pflash-based varstore is not what we should compromise on.

> With this
> patch, I have verified that OS is able to update the runtime variable and
> FC-28 installer is successfully able to complete the installation process.
> 
> If you all agree with approach then I can rework any feedbacks and remove
> the rfc tag from the patch. If you have better suggestions then I am open
> to explore those as well.

I'd like to understand the following:

(1) why does FtwNotificationEvent() set the EFI_MEMORY_RUNTIME attribute
itself, for the pflash range? -- in my opinion, that belongs in the
flash driver.

(2) Whether Star and Eric agree with setting the EFI_MEMORY_RUNTIME
attribute in FtwNotificationEvent() only if the attribute is not already
present.

(3) The implication that you describe, between runtime services/code
being mapped encrypted, and restoring the C-bit failing.

(4) Whether we should modify MarkMemoryRangeForRuntimeAccess() to
install the range into GCD as MMIO. (I feel *very* uncomfortable about
this, however; the current code has existed as-is for years, and
regressions look very risky.)

My strong preference would be a patch for (2).

Thanks,
Laszlo

> Cc: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  .../FvbServicesRuntimeDxe.inf                      |  1 +
>  .../FwBlockService.c                               | 37 +++++++++++++++++++---
>  2 files changed, 34 insertions(+), 4 deletions(-)
> 
> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
> index d7b4ec06c4e6..6bb5c2093790 100644
> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
> @@ -54,6 +54,7 @@ [LibraryClasses]
>    DevicePathLib
>    DxeServicesTableLib
>    MemoryAllocationLib
> +  MemEncryptSevLib
>    PcdLib
>    UefiBootServicesTableLib
>    UefiDriverEntryPoint
> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
> index 558b395dff4a..e82b4ff70961 100644
> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
> @@ -36,6 +36,7 @@
>  #include <Library/DxeServicesTableLib.h>
>  #include <Library/MemoryAllocationLib.h>
>  #include <Library/UefiBootServicesTableLib.h>
> +#include <Library/MemEncryptSevLib.h>
>  
>  #include "FwBlockService.h"
>  #include "QemuFlash.h"
> @@ -966,6 +967,7 @@ FvbInitialize (
>    UINTN                               Length;
>    UINTN                               NumOfBlocks;
>    RETURN_STATUS                       PcdStatus;
> +  EFI_PHYSICAL_ADDRESS                CryptedAddress;
>  
>    if (EFI_ERROR (QemuFlashInitialize ())) {
>      //
> @@ -986,6 +988,24 @@ FvbInitialize (
>    BaseAddress = (UINTN) PcdGet32 (PcdOvmfFdBaseAddress);
>    Length = PcdGet32 (PcdOvmfFirmwareFdSize);
>  
> +  //
> +  // When SEV is enabled, allocate a encrypted buffer which will contain a
> +  // encrypted copy of the Flash image.
> +  //
> +  if (MemEncryptSevIsEnabled ()) {
> +    Status = gBS->AllocatePages (
> +                    AllocateAnyPages,
> +                    EfiRuntimeServicesData,
> +                    EFI_SIZE_TO_PAGES(PcdGet32 (PcdOvmfFirmwareFdSize)),
> +                    &CryptedAddress
> +                    );
> +    ASSERT_EFI_ERROR (Status);
> +
> +    CopyMem((VOID *)CryptedAddress, (VOID *)BaseAddress, Length);
> +
> +    BaseAddress = CryptedAddress;
> +  }
> +
>    Status = InitializeVariableFvHeader ();
>    if (EFI_ERROR (Status)) {
>      DEBUG ((EFI_D_INFO,
> @@ -1091,24 +1111,33 @@ FvbInitialize (
>    //
>    InstallProtocolInterfaces (FvbDevice);
>  
> -  MarkMemoryRangeForRuntimeAccess (BaseAddress, Length);
> +  MarkMemoryRangeForRuntimeAccess (
> +    (UINTN) PcdGet32 (PcdOvmfFdBaseAddress),
> +    Length
> +    );
>  
>    //
>    // Set several PCD values to point to flash
>    //
>    PcdStatus = PcdSet64S (
>      PcdFlashNvStorageVariableBase64,
> -    (UINTN) PcdGet32 (PcdOvmfFlashNvStorageVariableBase)
> +    BaseAddress
>      );
>    ASSERT_RETURN_ERROR (PcdStatus);
>    PcdStatus = PcdSet32S (
>      PcdFlashNvStorageFtwWorkingBase,
> -    PcdGet32 (PcdOvmfFlashNvStorageFtwWorkingBase)
> +    BaseAddress +
> +    PcdGet32(PcdFlashNvStorageVariableSize) +
> +    PcdGet32(PcdOvmfFlashNvStorageEventLogSize)
>      );
> +
>    ASSERT_RETURN_ERROR (PcdStatus);
>    PcdStatus = PcdSet32S (
>      PcdFlashNvStorageFtwSpareBase,
> -    PcdGet32 (PcdOvmfFlashNvStorageFtwSpareBase)
> +    BaseAddress +
> +    PcdGet32(PcdFlashNvStorageVariableSize) +
> +    PcdGet32(PcdOvmfFlashNvStorageEventLogSize) +
> +    PcdGet32(PcdFlashNvStorageFtwWorkingSize)
>      );
>    ASSERT_RETURN_ERROR (PcdStatus);
>  
>