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

* [RFC PATCH 1/1] OvmfPkg/QemuFlash: Fix Runtime variable access when SEV is enabled
@ 2018-06-26 19:39 Brijesh Singh
  0 siblings, 0 replies; 15+ messages in thread
From: Brijesh Singh @ 2018-06-26 19:39 UTC (permalink / raw)
  To: edk2-devel; +Cc: Tom Lendacky, Brijesh Singh, Laszlo Ersek

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". 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 but that was not sufficient because UEFI
runtime services are mapped as "encrypted" in OS page table hence we end up
accessing the flash as encrypted when OS requests to update the variables.

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. 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.

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



^ permalink raw reply related	[flat|nested] 15+ messages in thread