[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

[Brijesh Singh]

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

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

[Laszlo Ersek]

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

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

[Brijesh Singh]

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

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

[Brijesh Singh]

On 06/27/2018 11:34 AM, Brijesh Singh wrote:
> I think (2) will solve the complete issue, we still need to figure how

I meant to say (2) will *not* solve the complete issue !

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

[Laszlo Ersek]

On 06/27/18 18:34, Brijesh Singh wrote:
> On 06/27/2018 07:54 AM, Laszlo Ersek wrote:
>> On 06/26/18 21:46, Brijesh Singh wrote:

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

This error occurs because (I think) you modified only the AddMemorySpace
call. If you change the GCD type on that, then please update the
subsequent AllocatePages as well, from EfiRuntimeServicesData to
EfiMemoryMappedIO.

The spec says about the latter enum constant, "Used by system firmware
to request that a memory-mapped IO region be mapped by the OS to a
virtual address so it can be accessed by EFI runtime services." It seems
appropriate (and I'm a bit confused why we haven't used the MMIO GCD and
UEFI enum values for the memory type, all this time.)

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

Indeed.

(And, this is only a problem when SMM is not used, i.e. when the full
variable driver stack is non-SMM, just DXE. In the SMM case, the SMM
page tables are used, and the OS cannot interfere with that.)

Anyway, in the pure DXE / runtime driver case, do you think a guest
kernel patch will be necessary too? Perhaps if you change the UEFI
memmap entry type (see AllocatePages above) to MMIO, then the guest
kernel could technically honor that.

Thanks
Laszlo

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

[Brijesh Singh]

On 06/27/2018 11:59 AM, Laszlo Ersek wrote:
> On 06/27/18 18:34, Brijesh Singh wrote:
>> On 06/27/2018 07:54 AM, Laszlo Ersek wrote:
>>> On 06/26/18 21:46, Brijesh Singh wrote:
> 
>>>> 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)
> 
> This error occurs because (I think) you modified only the AddMemorySpace
> call. If you change the GCD type on that, then please update the
> subsequent AllocatePages as well, from EfiRuntimeServicesData to
> EfiMemoryMappedIO.
> 

Here is what I have.

--- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
+++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
@@ -847,7 +847,7 @@ MarkMemoryRangeForRuntimeAccess (
                    );

    Status = gDS->AddMemorySpace (
-                  EfiGcdMemoryTypeSystemMemory,
+                  EfiGcdMemoryTypeMemoryMappedIo,
                    BaseAddress,
                    Length,
                    EFI_MEMORY_UC | EFI_MEMORY_RUNTIME
@@ -856,7 +856,7 @@ MarkMemoryRangeForRuntimeAccess (

    Status = gBS->AllocatePages (
                    AllocateAddress,
-                  EfiRuntimeServicesData,
+                  EfiMemoryMappedIO,
                    EFI_SIZE_TO_PAGES (Length),
                    &BaseAddress
                    );

I am still getting the error assertion failure. I can debug to see what 
is going on.


> The spec says about the latter enum constant, "Used by system firmware
> to request that a memory-mapped IO region be mapped by the OS to a
> virtual address so it can be accessed by EFI runtime services." It seems
> appropriate (and I'm a bit confused why we haven't used the MMIO GCD and
> UEFI enum values for the memory type, all this time.)
> 
>> 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).
> 
> Indeed.
> 
> (And, this is only a problem when SMM is not used, i.e. when the full
> variable driver stack is non-SMM, just DXE. In the SMM case, the SMM
> page tables are used, and the OS cannot interfere with that.)
> 

Good point, I will try it and let you know. As you say since SMM uses
UEFI page table hence after fixing FtwNotificationEvent(..) we should be 
good.


> Anyway, in the pure DXE / runtime driver case, do you think a guest
> kernel patch will be necessary too? Perhaps if you change the UEFI
> memmap entry type (see AllocatePages above) to MMIO, then the guest
> kernel could technically honor that.
> 


Theoretically speaking, if we are able to make this memory region as
mmio then OS should be able to map it with C=0.


-Brijesh

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

[Zeng, Star]

My understanding is MMIO is not managed by UEFI memory services, but GCD services.
PI spec says " If the memory range specified by BaseAddress and Length is of type EfiGcdMemoryTypeSystemMemory or EfiGcdMemoryTypeMoreReliable, then the memory range may be automatically *allocated* for use by the *UEFI memory services*." in AddMemorySpace() description.

For MMIO, the code needs to use AddMemorySpace() + AllocateMemorySpace().


Thanks,
Star
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Brijesh Singh
Sent: Thursday, June 28, 2018 1:50 AM
To: Laszlo Ersek <lersek@redhat.com>; edk2-devel@lists.01.org
Cc: Tom Lendacky <Thomas.Lendacky@amd.com>; brijesh.singh@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/27/2018 11:59 AM, Laszlo Ersek wrote:
> On 06/27/18 18:34, Brijesh Singh wrote:
>> On 06/27/2018 07:54 AM, Laszlo Ersek wrote:
>>> On 06/26/18 21:46, Brijesh Singh wrote:
> 
>>>> 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)
> 
> This error occurs because (I think) you modified only the 
> AddMemorySpace call. If you change the GCD type on that, then please 
> update the subsequent AllocatePages as well, from 
> EfiRuntimeServicesData to EfiMemoryMappedIO.
> 

Here is what I have.

--- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
+++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
@@ -847,7 +847,7 @@ MarkMemoryRangeForRuntimeAccess (
                    );

    Status = gDS->AddMemorySpace (
-                  EfiGcdMemoryTypeSystemMemory,
+                  EfiGcdMemoryTypeMemoryMappedIo,
                    BaseAddress,
                    Length,
                    EFI_MEMORY_UC | EFI_MEMORY_RUNTIME @@ -856,7 +856,7 @@ MarkMemoryRangeForRuntimeAccess (

    Status = gBS->AllocatePages (
                    AllocateAddress,
-                  EfiRuntimeServicesData,
+                  EfiMemoryMappedIO,
                    EFI_SIZE_TO_PAGES (Length),
                    &BaseAddress
                    );

I am still getting the error assertion failure. I can debug to see what is going on.


> The spec says about the latter enum constant, "Used by system firmware
> to request that a memory-mapped IO region be mapped by the OS to a
> virtual address so it can be accessed by EFI runtime services." It seems
> appropriate (and I'm a bit confused why we haven't used the MMIO GCD and
> UEFI enum values for the memory type, all this time.)
> 
>> 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).
> 
> Indeed.
> 
> (And, this is only a problem when SMM is not used, i.e. when the full
> variable driver stack is non-SMM, just DXE. In the SMM case, the SMM
> page tables are used, and the OS cannot interfere with that.)
> 

Good point, I will try it and let you know. As you say since SMM uses
UEFI page table hence after fixing FtwNotificationEvent(..) we should be 
good.


> Anyway, in the pure DXE / runtime driver case, do you think a guest
> kernel patch will be necessary too? Perhaps if you change the UEFI
> memmap entry type (see AllocatePages above) to MMIO, then the guest
> kernel could technically honor that.
> 


Theoretically speaking, if we are able to make this memory region as
mmio then OS should be able to map it with C=0.


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

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

[Laszlo Ersek]

On 06/28/18 08:25, Zeng, Star wrote:
> My understanding is MMIO is not managed by UEFI memory services, but GCD services.
> PI spec says " If the memory range specified by BaseAddress and Length is of type EfiGcdMemoryTypeSystemMemory or EfiGcdMemoryTypeMoreReliable, then the memory range may be automatically *allocated* for use by the *UEFI memory services*." in AddMemorySpace() description.
> 
> For MMIO, the code needs to use AddMemorySpace() + AllocateMemorySpace().

Right, after seeing today the (updated) AllocatePages() failure,
reported by Brijesh, I came to the same conclusion. Thank you for
correcting my initial suggestion! It's not easy to keep this details in
mind :)

Laszlo

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

[Laszlo Ersek]

On 06/27/18 19:49, Brijesh Singh wrote:
> 
> 
> On 06/27/2018 11:59 AM, Laszlo Ersek wrote:
>> On 06/27/18 18:34, Brijesh Singh wrote:
>>> On 06/27/2018 07:54 AM, Laszlo Ersek wrote:
>>>> On 06/26/18 21:46, Brijesh Singh wrote:
>>
>>>>> 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)
>>
>> This error occurs because (I think) you modified only the AddMemorySpace
>> call. If you change the GCD type on that, then please update the
>> subsequent AllocatePages as well, from EfiRuntimeServicesData to
>> EfiMemoryMappedIO.
>>
> 
> Here is what I have.
> 
> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
> @@ -847,7 +847,7 @@ MarkMemoryRangeForRuntimeAccess (
>                    );
> 
>    Status = gDS->AddMemorySpace (
> -                  EfiGcdMemoryTypeSystemMemory,
> +                  EfiGcdMemoryTypeMemoryMappedIo,
>                    BaseAddress,
>                    Length,
>                    EFI_MEMORY_UC | EFI_MEMORY_RUNTIME
> @@ -856,7 +856,7 @@ MarkMemoryRangeForRuntimeAccess (
> 
>    Status = gBS->AllocatePages (
>                    AllocateAddress,
> -                  EfiRuntimeServicesData,
> +                  EfiMemoryMappedIO,
>                    EFI_SIZE_TO_PAGES (Length),
>                    &BaseAddress
>                    );
> 
> I am still getting the error assertion failure. I can debug to see what
> is going on.

Hmmm. Indeed, memory space added to GCD need not immediately show up in
the UEFI memory map, for the UEFI memory allocation services to allocate
from. IIRC, the PI spec says that *system memory* added like this *may*
show up immediately:

"""
If the memory range specified by BaseAddress and Length is of type
EfiGcdMemoryTypeSystemMemory or EfiGcdMemoryTypeMoreReliable, then the
memory range may be automatically allocated for use by the UEFI memory
services.
"""

and, indeed in edk2, it happens at once; but it doesn't apply to MMIO at
all.

So, can you replace the AllocatePages call with the following:

  Status = gDS->AllocateMemorySpace (
                  EfiGcdAllocateAddress,
                  EfiGcdMemoryTypeMemoryMappedIo,
                  0,                              // Alignment
                  EFI_SIZE_TO_PAGES (Length),
                  &BaseAddress,
                  gImageHandle,
                  NULL                            // DeviceHandle
                  );

>> The spec says about the latter enum constant, "Used by system firmware
>> to request that a memory-mapped IO region be mapped by the OS to a
>> virtual address so it can be accessed by EFI runtime services." It seems
>> appropriate (and I'm a bit confused why we haven't used the MMIO GCD and
>> UEFI enum values for the memory type, all this time.)
>>
>>> 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).
>>
>> Indeed.
>>
>> (And, this is only a problem when SMM is not used, i.e. when the full
>> variable driver stack is non-SMM, just DXE. In the SMM case, the SMM
>> page tables are used, and the OS cannot interfere with that.)
>>
> 
> Good point, I will try it and let you know. As you say since SMM uses
> UEFI page table

More correctly: SMM drivers use, at runtime as well, the page table in
SMRAM that was created by the firmware.

> hence after fixing FtwNotificationEvent(..) we should be
> good.

No, that's not precise -- FtwNotificationEvent() is not used in SMM *at
all*.

I checked that before I sent my previous email; namely,
FtwNotificationEvent() is in
"MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c", which is not
built into the SMM variant of the variable driver.

Please see the variable driver stacks in OVMF:

- non-SMM:

  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
  OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.inf
  MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe.inf
  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf

In the non-SMM case, we have a normal runtime driver,
VariableRuntimeDxe.inf, that implements the variable service. It relies
on the FTW driver (another runtime driver) for the FTW protocol.
Finally, both the variable driver and the FTW driver rely on the FVB
(firmware volume block) protocol, which may come from the QEMU flash
driver, or from the emulation driver. Again, all of these are runtime
drivers. "VariableRuntimeDxe.inf" includes the "VariableDxe.c" file,
hence FtwNotificationEvent() is relevant.

- SMM:

  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf
  MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.inf
  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.inf

In the SMM case, we have a *split* variable driver,
"VariableSmmRuntimeDxe.inf" (which is a normal runtime driver) and
"VariableSmm.inf" (a "traditional SMM" driver). The runtime half is
unprivileged and only formats a message for the privileged SMM half,
submits the message, and then parses the response. The rest of the
variable stack, namely the privileged half of the variable driver, and
the FTW driver, and the QEMU flash (FVB) driver, all stay within SMM --
they are traditional SMM drivers too.

"VariableSmmRuntimeDxe.inf" does not include "VariableDxe.c". Instead,
"VariableSmm.inf" (the privileged half) includes "VariableSmm.c", and
that source file waits for the *SMM* FVB protocol, in the
SmmFtwNotificationEvent() function. But, this notification function,
unlike the DXE counterpart FtwNotificationEvent(), does not care about
the GCD entry attributes -- because in SMM, separate (protected) page
tables are used anyway.

If you put FtwNotificationEvent() and SmmFtwNotificationEvent() side by
side, you'll see that they perform almost identical steps:
- "Ensure [SMM] FTW protocol is installed"
- "Find the proper [SMM] FVB protocol for variable"
- "Mark the variable storage region of the FLASH as RUNTIME" -- this is
  DXE only!
- VariableWriteServiceInitialize
- "Install the Variable Write Architectural protocol"

(I skipped RecordSecureBootPolicyVarData(), because that always happens
in the DXE half. Anyway it's irrelevant here.)

Thus, my expectation is that the current issue manifests itself only if
you build OVMF without -D SMM_REQUIRE.

>> Anyway, in the pure DXE / runtime driver case, do you think a guest
>> kernel patch will be necessary too? Perhaps if you change the UEFI
>> memmap entry type (see AllocatePages above) to MMIO, then the guest
>> kernel could technically honor that.
>>
> 
> 
> Theoretically speaking, if we are able to make this memory region as
> mmio then OS should be able to map it with C=0.

Thanks
Laszlo

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

[Laszlo Ersek]

On 06/28/18 14:57, Laszlo Ersek wrote:
> On 06/27/18 19:49, Brijesh Singh wrote:
>>
>>
>> On 06/27/2018 11:59 AM, Laszlo Ersek wrote:
>>> On 06/27/18 18:34, Brijesh Singh wrote:
>>>> On 06/27/2018 07:54 AM, Laszlo Ersek wrote:
>>>>> On 06/26/18 21:46, Brijesh Singh wrote:
>>>
>>>>>> 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)
>>>
>>> This error occurs because (I think) you modified only the AddMemorySpace
>>> call. If you change the GCD type on that, then please update the
>>> subsequent AllocatePages as well, from EfiRuntimeServicesData to
>>> EfiMemoryMappedIO.
>>>
>>
>> Here is what I have.
>>
>> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
>> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
>> @@ -847,7 +847,7 @@ MarkMemoryRangeForRuntimeAccess (
>>                    );
>>
>>    Status = gDS->AddMemorySpace (
>> -                  EfiGcdMemoryTypeSystemMemory,
>> +                  EfiGcdMemoryTypeMemoryMappedIo,
>>                    BaseAddress,
>>                    Length,
>>                    EFI_MEMORY_UC | EFI_MEMORY_RUNTIME
>> @@ -856,7 +856,7 @@ MarkMemoryRangeForRuntimeAccess (
>>
>>    Status = gBS->AllocatePages (
>>                    AllocateAddress,
>> -                  EfiRuntimeServicesData,
>> +                  EfiMemoryMappedIO,
>>                    EFI_SIZE_TO_PAGES (Length),
>>                    &BaseAddress
>>                    );
>>
>> I am still getting the error assertion failure. I can debug to see what
>> is going on.
> 
> Hmmm. Indeed, memory space added to GCD need not immediately show up in
> the UEFI memory map, for the UEFI memory allocation services to allocate
> from. IIRC, the PI spec says that *system memory* added like this *may*
> show up immediately:
> 
> """
> If the memory range specified by BaseAddress and Length is of type
> EfiGcdMemoryTypeSystemMemory or EfiGcdMemoryTypeMoreReliable, then the
> memory range may be automatically allocated for use by the UEFI memory
> services.
> """
> 
> and, indeed in edk2, it happens at once; but it doesn't apply to MMIO at
> all.
> 
> So, can you replace the AllocatePages call with the following:
> 
>   Status = gDS->AllocateMemorySpace (
>                   EfiGcdAllocateAddress,
>                   EfiGcdMemoryTypeMemoryMappedIo,
>                   0,                              // Alignment
>                   EFI_SIZE_TO_PAGES (Length),
>                   &BaseAddress,
>                   gImageHandle,
>                   NULL                            // DeviceHandle
>                   );
> 
>>> The spec says about the latter enum constant, "Used by system firmware
>>> to request that a memory-mapped IO region be mapped by the OS to a
>>> virtual address so it can be accessed by EFI runtime services." It seems
>>> appropriate (and I'm a bit confused why we haven't used the MMIO GCD and
>>> UEFI enum values for the memory type, all this time.)
>>>
>>>> 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).
>>>
>>> Indeed.
>>>
>>> (And, this is only a problem when SMM is not used, i.e. when the full
>>> variable driver stack is non-SMM, just DXE. In the SMM case, the SMM
>>> page tables are used, and the OS cannot interfere with that.)
>>>
>>
>> Good point, I will try it and let you know. As you say since SMM uses
>> UEFI page table
> 
> More correctly: SMM drivers use, at runtime as well, the page table in
> SMRAM that was created by the firmware.
> 
>> hence after fixing FtwNotificationEvent(..) we should be
>> good.
> 
> No, that's not precise -- FtwNotificationEvent() is not used in SMM *at
> all*.

Sigh, I misunderstood you. You meant, "after fixing DXE, we should be
good, because the the issue only affects DXE".

I mistook your statement as "after we fix DXE, both DXE and SMM will be
OK, because the fix affects both DXE and SMM". That was not a correct
statement (because the fix only affects DXE; SMM is unaffected and needs
no fix), which is why I attempted to correct it. Of course, you never
*said* that statement. :)

Sorry about the noise!
Laszlo

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

[Brijesh Singh]

On 06/28/2018 07:57 AM, Laszlo Ersek wrote:

[...]

>>
>> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
>> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
>> @@ -847,7 +847,7 @@ MarkMemoryRangeForRuntimeAccess (
>>                     );
>>
>>     Status = gDS->AddMemorySpace (
>> -                  EfiGcdMemoryTypeSystemMemory,
>> +                  EfiGcdMemoryTypeMemoryMappedIo,
>>                     BaseAddress,
>>                     Length,
>>                     EFI_MEMORY_UC | EFI_MEMORY_RUNTIME
>> @@ -856,7 +856,7 @@ MarkMemoryRangeForRuntimeAccess (
>>
>>     Status = gBS->AllocatePages (
>>                     AllocateAddress,
>> -                  EfiRuntimeServicesData,
>> +                  EfiMemoryMappedIO,
>>                     EFI_SIZE_TO_PAGES (Length),
>>                     &BaseAddress
>>                     );
>>
>> I am still getting the error assertion failure. I can debug to see what
>> is going on.
> 
> Hmmm. Indeed, memory space added to GCD need not immediately show up in
> the UEFI memory map, for the UEFI memory allocation services to allocate
> from. IIRC, the PI spec says that *system memory* added like this *may*
> show up immediately:
> 
> """
> If the memory range specified by BaseAddress and Length is of type
> EfiGcdMemoryTypeSystemMemory or EfiGcdMemoryTypeMoreReliable, then the
> memory range may be automatically allocated for use by the UEFI memory
> services.
> """
> 
> and, indeed in edk2, it happens at once; but it doesn't apply to MMIO at
> all.
> 
> So, can you replace the AllocatePages call with the following:
> 
>    Status = gDS->AllocateMemorySpace (
>                    EfiGcdAllocateAddress,
>                    EfiGcdMemoryTypeMemoryMappedIo,
>                    0,                              // Alignment
>                    EFI_SIZE_TO_PAGES (Length),
>                    &BaseAddress,
>                    gImageHandle,
>                    NULL                            // DeviceHandle
>                    );
> 


I did realized this in my yesterday's debug. I was looking at other
drivers (mainly PciBridge...) on how it adds the MMIO. I no longer
get the ASSERT. thank you !

After changing this system boots fine but I am getting Linux kernel
crashes when we attempt to update the UEFI runtime variables. I am
still debugging and trying to to root cause it.



>>
>> Good point, I will try it and let you know. As you say since SMM uses
>> UEFI page table
> 
> More correctly: SMM drivers use, at runtime as well, the page table in
> SMRAM that was created by the firmware.
> 
>> hence after fixing FtwNotificationEvent(..) we should be
>> good.
> 
> No, that's not precise -- FtwNotificationEvent() is not used in SMM *at
> all*.
> 
> I checked that before I sent my previous email; namely,
> FtwNotificationEvent() is in
> "MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c", which is not
> built into the SMM variant of the variable driver.
> 
> Please see the variable driver stacks in OVMF:
> 
> - non-SMM:
> 
>    OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
>    OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.inf
>    MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe.inf
>    MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
> 
> In the non-SMM case, we have a normal runtime driver,
> VariableRuntimeDxe.inf, that implements the variable service. It relies
> on the FTW driver (another runtime driver) for the FTW protocol.
> Finally, both the variable driver and the FTW driver rely on the FVB
> (firmware volume block) protocol, which may come from the QEMU flash
> driver, or from the emulation driver. Again, all of these are runtime
> drivers. "VariableRuntimeDxe.inf" includes the "VariableDxe.c" file,
> hence FtwNotificationEvent() is relevant.
> 
> - SMM:
> 
>    OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf
>    MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.inf
>    MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
>    MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.inf
> 
> In the SMM case, we have a *split* variable driver,
> "VariableSmmRuntimeDxe.inf" (which is a normal runtime driver) and
> "VariableSmm.inf" (a "traditional SMM" driver). The runtime half is
> unprivileged and only formats a message for the privileged SMM half,
> submits the message, and then parses the response. The rest of the
> variable stack, namely the privileged half of the variable driver, and
> the FTW driver, and the QEMU flash (FVB) driver, all stay within SMM --
> they are traditional SMM drivers too.
> 
> "VariableSmmRuntimeDxe.inf" does not include "VariableDxe.c". Instead,
> "VariableSmm.inf" (the privileged half) includes "VariableSmm.c", and
> that source file waits for the *SMM* FVB protocol, in the
> SmmFtwNotificationEvent() function. But, this notification function,
> unlike the DXE counterpart FtwNotificationEvent(), does not care about
> the GCD entry attributes -- because in SMM, separate (protected) page
> tables are used anyway.
> 
> If you put FtwNotificationEvent() and SmmFtwNotificationEvent() side by
> side, you'll see that they perform almost identical steps:
> - "Ensure [SMM] FTW protocol is installed"
> - "Find the proper [SMM] FVB protocol for variable"
> - "Mark the variable storage region of the FLASH as RUNTIME" -- this is
>    DXE only!
> - VariableWriteServiceInitialize
> - "Install the Variable Write Architectural protocol"
> 
> (I skipped RecordSecureBootPolicyVarData(), because that always happens
> in the DXE half. Anyway it's irrelevant here.)
> 
> Thus, my expectation is that the current issue manifests itself only if
> you build OVMF without -D SMM_REQUIRE.
> 


Actually I was not aware that there are two different stack (SMM vs
non-SMM). And you are absolutely right. The issue does not happen
when using -D SMM_REQUIRE flag. So, I think we just need to focus
on making non SMM work.

-Brijesh

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

[Zeng, Star]

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

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

[Laszlo Ersek]

On 06/28/18 08:16, Zeng, Star wrote:
> 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.

Great, thank you!

Yesterday, after I sent my email(s), I got concerned for a moment,
because it wasn't clear to me what *runtime driver* actually owned the
flash range.

However, the FVB protocol itself must be provided by a runtime driver on
UEFI/PI platforms where the variable write service implementation --
indirectly or directly -- depends on FVB, for flash writes. Indeed in
edk2, we have at least the following FVB drivers:

$ git grep -l -E \
    -e 'EfiFirmwareVolumeBlock2?ProtocolGuid' --and -e 'PROD'

EmulatorPkg/FvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
Nt32Pkg/FvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.inf
OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
Vlv2TbltDevicePkg/FvbRuntimeDxe/FvbRuntimeDxe.inf
Vlv2TbltDevicePkg/FvbRuntimeDxe/FvbSmmDxe.inf

All of them are DXE_RUNTIME_DRIVER modules [*] and they all call
EfiConvertPointer().

[*] "Vlv2TbltDevicePkg/FvbRuntimeDxe/FvbSmmDxe.inf" is marked as a
DXE_DRIVER (not runtime), and indeed it doesn't call
EfiConvertPointer(). I don't know what this driver is good for; the INF
file is not included in any DSC file in edk2. I guess it can be used for
flash access on an SMM platform as long as you never boot an OS / never
call ExitBootServices(). In that case however, runtime aspects have no
meaning at all.

Thanks!
Laszlo

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

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

[Zeng, Star]

My understading:

Vlv2TbltDevicePkg/FvbRuntimeDxe/FvbSmmDxe.inf is DXE counterpart of Vlv2TbltDevicePkg/FvbRuntimeDxe/FvbSmm.inf, to produce EfiFirmwareVolumeBlock2?ProtocolGuid based on gEfiSmmFirmwareVolumeBlockProtocolGuid by SMM comm.

Platform can choose FvbSmmDxe + FvbSmm, or FvbRuntimeDxe + FvbSmm.


Thanks,
Star
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
Sent: Thursday, June 28, 2018 9:14 PM
To: Zeng, Star <star.zeng@intel.com>; Brijesh Singh <brijesh.singh@amd.com>; edk2-devel@lists.01.org
Cc: Tom Lendacky <Thomas.Lendacky@amd.com>; Dong, Eric <eric.dong@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/28/18 08:16, Zeng, Star wrote:
> 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.

Great, thank you!

Yesterday, after I sent my email(s), I got concerned for a moment, because it wasn't clear to me what *runtime driver* actually owned the flash range.

However, the FVB protocol itself must be provided by a runtime driver on UEFI/PI platforms where the variable write service implementation -- indirectly or directly -- depends on FVB, for flash writes. Indeed in edk2, we have at least the following FVB drivers:

$ git grep -l -E \
    -e 'EfiFirmwareVolumeBlock2?ProtocolGuid' --and -e 'PROD'

EmulatorPkg/FvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
Nt32Pkg/FvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.inf
OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
Vlv2TbltDevicePkg/FvbRuntimeDxe/FvbRuntimeDxe.inf
Vlv2TbltDevicePkg/FvbRuntimeDxe/FvbSmmDxe.inf

All of them are DXE_RUNTIME_DRIVER modules [*] and they all call EfiConvertPointer().

[*] "Vlv2TbltDevicePkg/FvbRuntimeDxe/FvbSmmDxe.inf" is marked as a DXE_DRIVER (not runtime), and indeed it doesn't call EfiConvertPointer(). I don't know what this driver is good for; the INF file is not included in any DSC file in edk2. I guess it can be used for flash access on an SMM platform as long as you never boot an OS / never call ExitBootServices(). In that case however, runtime aspects have no meaning at all.

Thanks!
Laszlo

> 
> 
> 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.in
>> +++ f
>> @@ -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
> 

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

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

[Brijesh Singh]

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