回复: [edk2-devel] [PATCH 1/1] MdeModulePkg/VariableRuntimeDxe: avoid double VA conversion of FVB protocol

["gaoliming" <gaoliming@byosoft.com.cn>]

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