Thanks for the quick feedback Laszlo !
On 06/27/2018 07:54 AM, Laszlo Ersek wrote:
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?
The issue has been always there and we never noticed. I noticed it after
I saw installation failure. I can easily reproduce it with simple
efibootmgr command:
'efibootmgr -o 0003,0004' -- e.g change boot order
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).
If Star and Eric agrees with your proposal then I can create a patch to
fix this issue.
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).
Sorry, I was meaning to say both the "code" and "data" are mapped as
encrypted by the OS.
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.
Right, the memory is marked as 'system ram' and not 'mmio'.
Just to experiment, I did try changing it to 'mmio' to see if OS will
map this region as "unencrypted" but ovmf fails with below error
message after changing it from systemRAM->mmio
ConvertPages: failed to find range FFC00000 - FFFFFFFF
ASSERT_EFI_ERROR (Status = Not Found)
ASSERT [FvbServicesRuntimeDxe]
/home/amd/workdir/upstream/edk2/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServie.c(864):
!EFI_ERROR (Status)
Since this efi runtime data is mapped as C=1 by the OS, hence when OS
asks efi to update the runtime variable we end up accessing the memory
region with C=1 (runtime services are executed using OS pagetable).
... 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.
While hacking this I knew it was not an idle approach but wanted to
start the discussion to find an acceptable solution.
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.
Agreed, if all the access to the flash chip was going to go through
FVB protocol members then we could use bounce buffer at the lowest
level.
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).
I think (2) will solve the complete issue, we still need to figure how
to communicate the OS to map this flash memory range as 'unencrypted'
so that efi runtime services can update the variables correctly.
Thanks,
Laszlo
Cc: Laszlo Ersek <ler...@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Brijesh Singh <brijesh.si...@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
