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


Wu, Hao A
 

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
gaoliming
Sent: Monday, March 15, 2021 10:41 AM
To: devel@edk2.groups.io; ardb@kernel.org
Cc: lersek@redhat.com; liming.gao@intel.com; jon@solid-run.com;
leif@nuviainc.com
Subject: 回复: [edk2-devel] [PATCH 1/1]
MdeModulePkg/VariableRuntimeDxe: avoid double VA conversion of FVB
protocol

Ard:
Thanks for your fix. Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn>

Can you include BZ into the commit message? BZ can mention the correct
conversion solution. Other modules should follow the same way to convert
FVB protocol VA.

Thanks Ard and Liming,

With above suggestion from Liming handled:
Reviewed-by: Hao A Wu <hao.a.wu@intel.com>

Best Regards,
Hao Wu



Thanks
Liming
-----邮件原件-----
发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Ard
Biesheuvel
发送时间: 2021年3月13日 7:06
收件人: devel@edk2.groups.io
抄送: lersek@redhat.com; liming.gao@intel.com; jon@solid-run.com;
leif@nuviainc.com; Ard Biesheuvel <ardb@kernel.org>
主题: [edk2-devel] [PATCH 1/1] MdeModulePkg/VariableRuntimeDxe:
avoid
double VA conversion of FVB protocol

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



-=-=-=-=-=-=
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#72734):
https://edk2.groups.io/g/devel/message/72734
Mute This Topic: https://groups.io/mt/81292874/4905953
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub
[gaoliming@byosoft.com.cn]
-=-=-=-=-=-=





Join devel@edk2.groups.io to automatically receive all group messages.