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

["Zeng, Star" <star.zeng@intel.com>]

1) My understanding is Variable Driver is managing the variable region in flash although the flash read/write/erase operations are done in flash driver. Current Variable Driver needs the address (to variable region) be converted to virtual address for runtime, and it does not assume flash driver to set the runtime attribute for variable region, but do it by itself.

2) I agree this approach. If the runtime attribute has been set by flash driver, Variable Driver has no need to do that.


Thanks,
Star
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
Sent: Wednesday, June 27, 2018 8:54 PM
To: Brijesh Singh <brijesh.singh@amd.com>; edk2-devel@lists.01.org
Cc: Tom Lendacky <Thomas.Lendacky@amd.com>; Dong, Eric <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>
Subject: Re: [edk2] [RFC PATCH 1/1] OvmfPkg/QemuFlash: Fix Runtime variable access when SEV is enabled

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 
> FwInstance->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);
>  
> 

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel