public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: Brijesh Singh <brijesh.singh@amd.com>
To: edk2-devel@lists.01.org
Cc: Tom Lendacky <Thomas.Lendacky@amd.com>,
	Brijesh Singh <brijesh.singh@amd.com>,
	Laszlo Ersek <lersek@redhat.com>
Subject: [RFC PATCH 1/1] OvmfPkg/QemuFlash: Fix Runtime variable access when SEV is enabled
Date: Tue, 26 Jun 2018 14:39:03 -0500	[thread overview]
Message-ID: <1530041943-9164-1-git-send-email-brijesh.singh@amd.com> (raw)

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



             reply	other threads:[~2018-06-26 19:39 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-06-26 19:39 Brijesh Singh [this message]
  -- strict thread matches above, loose matches on Subject: below --
2018-06-26 19:46 [RFC PATCH 1/1] OvmfPkg/QemuFlash: Fix Runtime variable access when SEV is enabled Brijesh Singh
2018-06-27 12:54 ` Laszlo Ersek
2018-06-27 16:34   ` Brijesh Singh
2018-06-27 16:37     ` Brijesh Singh
2018-06-27 16:59     ` Laszlo Ersek
2018-06-27 17:49       ` Brijesh Singh
2018-06-28  6:25         ` Zeng, Star
2018-06-28 13:15           ` Laszlo Ersek
2018-06-28 12:57         ` Laszlo Ersek
2018-06-28 13:21           ` Laszlo Ersek
2018-06-28 13:27           ` Brijesh Singh
2018-06-28  6:16   ` Zeng, Star
2018-06-28 13:13     ` Laszlo Ersek
2018-06-29  2:37       ` Zeng, Star

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1530041943-9164-1-git-send-email-brijesh.singh@amd.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox