From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-ej1-f48.google.com (mail-ej1-f48.google.com [209.85.218.48]) by mx.groups.io with SMTP id smtpd.web09.2033.1615635165011745491 for ; Sat, 13 Mar 2021 03:32:45 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@solid-run-com.20150623.gappssmtp.com header.s=20150623 header.b=nJk8SDuu; spf=pass (domain: solid-run.com, ip: 209.85.218.48, mailfrom: jon@solid-run.com) Received: by mail-ej1-f48.google.com with SMTP id p8so58289333ejb.10 for ; Sat, 13 Mar 2021 03:32:44 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=solid-run-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=oTTlgFhADAt/FABOoc1KYHziUZqoDQ53bszLhbvPe0o=; b=nJk8SDuuVkt21woJe3EqSoWG4IrqrKTIfTf6yyBxnHF4AVxhVeEYBEd+kH/geWm13V +ubXeDx63ILZwcF809vePIO2bk3SEPB1n3BuTYuxDLrUits0s+ZVI091ISQOvr59v9tH LWFtPHWDFK6PfS4TDQw53UIo6wxYqxBnEGP26wdOMXIjJmhIFh4Z9LcYxtXQBcyOSuO0 pwUBkXL18LU0FZ7GIo4k5c36wdoJjUuLupDWz6JjcOCd7qQ6NbvQ5+x7Pdki6DLl6ixG kYdkQVlvV+dBcOJo6YwH2PpjvqIjrdtAyPFA7R84MKeUm6eltt/1XKzefCgIDPVAu7xs Glig== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=oTTlgFhADAt/FABOoc1KYHziUZqoDQ53bszLhbvPe0o=; b=WJZEjHFR6mgUJW2NT+5I8dLhLZjj2824pwGaziIaakSPsmyHpLfZjs3WszCXs7Rn6D 6Pbt554FjrfP4+tybrCSEUo1rxMr4QLsRFkQHyAcoYigl/XoMGyNJr0qs/3JhYu7Jeo+ PsH5zno2I18NNu9ZP6/swWdDfRCQJtTAWDTavDI78tBkpS4pLyqW2hPR68VetgScj5tb R7LVd4kQKxJYNF4M+/So1k2K3uwOFHghgRmzf3OMR0Vzufx8utZ0/RruTaZNib2UMJCt NDVb8LD87CuUDxzWu+IlJWmKAKd9bq7H/XDJ96L/hCSfG2fJhsXkVDheuXZ5pFgnEoHd QVig== X-Gm-Message-State: AOAM5337KoLqZnu5qoYVl0mJRWKin/+/Y5x3GBAttL+1LR4FxTQfvwvi sOx6icCX8BciQdVFXUyDP3LrQHk4PSF9DUoUn4stDA== X-Google-Smtp-Source: ABdhPJySg0rStbg4LZfFGJUH1WEGyn+S2qMCn+4+FqPo9z+8vov6wczrluDd28jdQIXvjWAUT+7FDC6DOHj+oB2z8qY= X-Received: by 2002:a17:906:fc1c:: with SMTP id ov28mr13557823ejb.342.1615635163464; Sat, 13 Mar 2021 03:32:43 -0800 (PST) MIME-Version: 1.0 References: <20210312230554.292964-1-ardb@kernel.org> In-Reply-To: <20210312230554.292964-1-ardb@kernel.org> From: "Jon Nettleton" Date: Sat, 13 Mar 2021 12:32:06 +0100 Message-ID: Subject: Re: [PATCH 1/1] MdeModulePkg/VariableRuntimeDxe: avoid double VA conversion of FVB protocol To: Ard Biesheuvel Cc: devel@edk2.groups.io, Laszlo Ersek , liming.gao@intel.com, "Leif Lindholm (Nuvia address)" Content-Type: text/plain; charset="UTF-8" On Sat, Mar 13, 2021 at 12:06 AM Ard Biesheuvel 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 > --- > 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