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

[Brijesh Singh <brijesh.singh@amd.com>]

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