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

["Jon Nettleton" <jon@solid-run.com>]

On Sat, Mar 13, 2021 at 12:06 AM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> 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
>
You can add my

Tested-By: Jon Nettleton <jon@solid-run.com>