[PATCH 1/1] MdeModulePkg/VariableRuntimeDxe: avoid double VA conversion of FVB protocol

["Ard Biesheuvel" <ardb@kernel.org>]

For historical reasons, the VariableRuntimeDxe performs virtual address
conversion on the FVB protocol member pointers of the protocol instance
that backs the EFI variable store. However, the driver that produces the
actual instance should be doing this, as it is the owner, and provides
the actual implementation of those methods.

Unfortunately, we cannot simply change this: existing drivers may rely
on this behavior, and so the variable driver should take care to only
convert the pointers when necessary, but leave them alone when the owner
is taking care of this.

The virtual address switch event can be delivered in arbitrary order, and
so we should take care of this in a way that does not rely on whether this
driver converts its pointers either before or after the owner of the FVB
protocol receives the event.

So let's not convert the pointers at all when the event is delivered, but
record the converted addresses in a shadow FVB protocol. This way, we can
check when the variable driver is invoked at runtime whether the switch
has occurred or not, and perform the switch if it hasn't.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
Build tested only.

 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c | 50 +++++++++++++++++---
 1 file changed, 43 insertions(+), 7 deletions(-)

diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c
index 0fca0bb2a9b5..3d83ac4452ee 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c
@@ -37,6 +37,8 @@ EDKII_VAR_CHECK_PROTOCOL            mVarCheck                  = { VarCheckRegis
                                                                     VarCheckVariablePropertySet,
                                                                     VarCheckVariablePropertyGet };
 
+STATIC EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL mFvbProtocolShadow;
+
 /**
   Some Secure Boot Policy Variable may update following other variable changes(SecureBoot follows PK change, etc).
   Record their initial State when variable write service is ready.
@@ -105,8 +107,26 @@ AcquireLockOnlyAtBootTime (
   IN EFI_LOCK                             *Lock
   )
 {
+  EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL  *FvbInstance;
+
   if (!AtRuntime ()) {
     EfiAcquireLock (Lock);
+  } else {
+    //
+    // Check whether we need to swap in the converted pointer values for the
+    // FVB protocol methods
+    //
+    FvbInstance = mVariableModuleGlobal->FvbInstance;
+    if (FvbInstance != NULL &&
+        FvbInstance->GetBlockSize != mFvbProtocolShadow.GetBlockSize) {
+      FvbInstance->GetBlockSize       = mFvbProtocolShadow.GetBlockSize;
+      FvbInstance->GetPhysicalAddress = mFvbProtocolShadow.GetPhysicalAddress;
+      FvbInstance->GetAttributes      = mFvbProtocolShadow.GetAttributes;
+      FvbInstance->SetAttributes      = mFvbProtocolShadow.SetAttributes;
+      FvbInstance->Read               = mFvbProtocolShadow.Read;
+      FvbInstance->Write              = mFvbProtocolShadow.Write;
+      FvbInstance->EraseBlocks        = mFvbProtocolShadow.EraseBlocks;
+    }
   }
 }
 
@@ -247,13 +267,22 @@ VariableClassAddressChangeEvent (
   UINTN          Index;
 
   if (mVariableModuleGlobal->FvbInstance != NULL) {
-    EfiConvertPointer (0x0, (VOID **) &mVariableModuleGlobal->FvbInstance->GetBlockSize);
-    EfiConvertPointer (0x0, (VOID **) &mVariableModuleGlobal->FvbInstance->GetPhysicalAddress);
-    EfiConvertPointer (0x0, (VOID **) &mVariableModuleGlobal->FvbInstance->GetAttributes);
-    EfiConvertPointer (0x0, (VOID **) &mVariableModuleGlobal->FvbInstance->SetAttributes);
-    EfiConvertPointer (0x0, (VOID **) &mVariableModuleGlobal->FvbInstance->Read);
-    EfiConvertPointer (0x0, (VOID **) &mVariableModuleGlobal->FvbInstance->Write);
-    EfiConvertPointer (0x0, (VOID **) &mVariableModuleGlobal->FvbInstance->EraseBlocks);
+    //
+    // This module did not produce the FVB protocol instance that provides the
+    // variable store, so it should not be the one performing the pointer
+    // conversion on its members. However, some FVB protocol implementations
+    // may rely on this behavior, which was present in older versions of this
+    // driver, so we need to perform the conversion if the protocol producer
+    // fails to do so. So let's record the converted values now, and swap them
+    // in later if they haven't changed values.
+    //
+    EfiConvertPointer (0x0, (VOID **)&mFvbProtocolShadow.GetBlockSize);
+    EfiConvertPointer (0x0, (VOID **)&mFvbProtocolShadow.GetPhysicalAddress);
+    EfiConvertPointer (0x0, (VOID **)&mFvbProtocolShadow.GetAttributes);
+    EfiConvertPointer (0x0, (VOID **)&mFvbProtocolShadow.SetAttributes);
+    EfiConvertPointer (0x0, (VOID **)&mFvbProtocolShadow.Read);
+    EfiConvertPointer (0x0, (VOID **)&mFvbProtocolShadow.Write);
+    EfiConvertPointer (0x0, (VOID **)&mFvbProtocolShadow.EraseBlocks);
     EfiConvertPointer (0x0, (VOID **) &mVariableModuleGlobal->FvbInstance);
   }
   EfiConvertPointer (0x0, (VOID **) &mVariableModuleGlobal->PlatformLangCodes);
@@ -454,6 +483,13 @@ FtwNotificationEvent (
   }
   mVariableModuleGlobal->FvbInstance = FvbProtocol;
 
+  //
+  // Store the boot time values of the function pointers so we can compare
+  // them later. This is needed to avoid double conversion during the call
+  // to SetVirtualAddressMap.
+  //
+  CopyMem (&mFvbProtocolShadow, FvbProtocol, sizeof mFvbProtocolShadow);
+
   //
   // Mark the variable storage region of the FLASH as RUNTIME.
   //
-- 
2.30.1