Re: [PATCH v2 1/2] OvmfPkg: Introduce alternate UefiDriverEntrypoint to inhibit driver load

["Laszlo Ersek" <lersek@redhat.com>]

On 08/17/22 17:11, Ard Biesheuvel wrote:
> Add a new library that can be incorporated into any driver built from
> source, and which permits loading of the driver to be inhibited based on
> the value of a QEMU fw_cfg boolean variable. This will be used in a
> subsequent patch to allow dispatch of the IPv6 and IPv6 network protocol

(1) Still a typo? Did you mean "IPv4 and IPv6"?

> driver to be controlled from the QEMU command line.
> 
> This approach is based on the notion that all UEFI and DXE drivers share
> a single UefiDriverEntryPoint implementation, which we can easily swap
> out at build time with one that will abort execution based on the value
> of some QEMU fw_cfg variable.
> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  OvmfPkg/Library/UefiDriverEntryPointFwCfgOverrideLib/UefiDriverEntryPointFwCfgOverrideLib.c   | 147 ++++++++++++++++++++
>  OvmfPkg/Library/UefiDriverEntryPointFwCfgOverrideLib/UefiDriverEntryPointFwCfgOverrideLib.inf |  57 ++++++++
>  OvmfPkg/OvmfPkg.dec                                                                           |   4 +
>  3 files changed, 208 insertions(+)
> 
> diff --git a/OvmfPkg/Library/UefiDriverEntryPointFwCfgOverrideLib/UefiDriverEntryPointFwCfgOverrideLib.c b/OvmfPkg/Library/UefiDriverEntryPointFwCfgOverrideLib/UefiDriverEntryPointFwCfgOverrideLib.c
> new file mode 100644
> index 000000000000..6eaf0cfd16ad
> --- /dev/null
> +++ b/OvmfPkg/Library/UefiDriverEntryPointFwCfgOverrideLib/UefiDriverEntryPointFwCfgOverrideLib.c
> @@ -0,0 +1,147 @@
> +/** @file
> +  Entry point to a EFI/DXE driver. This version is specific to QEMU, and ties
> +  dispatch of the driver in question on the value of a QEMU fw_cfg boolean
> +  variable which is referenced by name via a fixed pointer PCD.
> +
> +Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2022, Google LLC. All rights reserved.<BR>
> +SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#include <Uefi.h>
> +
> +#include <Protocol/LoadedImage.h>
> +
> +#include <Library/BaseLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/QemuFwCfgSimpleParserLib.h>
> +#include <Library/UefiBootServicesTableLib.h>
> +#include <Library/UefiDriverEntryPoint.h>
> +
> +/**
> +  Unloads an image from memory.
> +
> +  This function is a callback that a driver registers to do cleanup
> +  when the UnloadImage boot service function is called.
> +
> +  @param  ImageHandle The handle to the image to unload.
> +
> +  @return Status returned by all unload().
> +
> +**/
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +_DriverUnloadHandler (
> +  EFI_HANDLE  ImageHandle
> +  )
> +{
> +  EFI_STATUS  Status;
> +
> +  //
> +  // If an UnloadImage() handler is specified, then call it
> +  //
> +  Status = ProcessModuleUnloadList (ImageHandle);
> +
> +  //
> +  // If the driver specific unload handler does not return an error, then call
> +  // all of the library destructors.  If the unload handler returned an error,
> +  // then the driver can not be unloaded, and the library destructors should
> +  // not be called
> +  //
> +  if (!EFI_ERROR (Status)) {
> +    ProcessLibraryDestructorList (ImageHandle, gST);
> +  }
> +
> +  //
> +  // Return the status from the driver specific unload handler
> +  //
> +  return Status;
> +}
> +
> +/**
> +  The entry point of PE/COFF Image for a DXE Driver, DXE Runtime Driver, or
> +  UEFI Driver.
> +
> +  @param  ImageHandle                The image handle of the DXE Driver, DXE
> +                                     Runtime Driver, or UEFI Driver.
> +  @param  SystemTable                A pointer to the EFI System Table.
> +
> +  @retval  EFI_SUCCESS               The DXE Driver, DXE Runtime Driver, or
> +                                     UEFI Driver exited normally.
> +  @retval  EFI_INCOMPATIBLE_VERSION  _gUefiDriverRevision is greater than
> +                                     SystemTable->Hdr.Revision.
> +  @retval  Other                     Return value from
> +                                     ProcessModuleEntryPointList().
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +_ModuleEntryPoint (
> +  IN EFI_HANDLE        ImageHandle,
> +  IN EFI_SYSTEM_TABLE  *SystemTable
> +  )
> +{
> +  EFI_STATUS                 Status;
> +  EFI_LOADED_IMAGE_PROTOCOL  *LoadedImage;
> +  RETURN_STATUS              RetStatus;
> +  BOOLEAN                    Enabled;
> +
> +  if (_gUefiDriverRevision != 0) {
> +    //
> +    // Make sure that the EFI/UEFI spec revision of the platform is >= EFI/UEFI
> +    // spec revision of the driver
> +    //
> +    if (SystemTable->Hdr.Revision < _gUefiDriverRevision) {
> +      return EFI_INCOMPATIBLE_VERSION;
> +    }
> +  }
> +
> +  //
> +  // Call constructor for all libraries
> +  //
> +  ProcessLibraryConstructorList (ImageHandle, SystemTable);
> +
> +  //
> +  //  Install unload handler...
> +  //
> +  if (_gDriverUnloadImageCount != 0) {
> +    Status = gBS->HandleProtocol (
> +                    ImageHandle,
> +                    &gEfiLoadedImageProtocolGuid,
> +                    (VOID **)&LoadedImage
> +                    );
> +    ASSERT_EFI_ERROR (Status);
> +    LoadedImage->Unload = _DriverUnloadHandler;
> +  }
> +
> +  RetStatus = QemuFwCfgParseBool (
> +                FixedPcdGetPtr (PcdEntryPointOverrideFwCfgVarName),
> +                &Enabled);
> +  if (!RETURN_ERROR (RetStatus) && !Enabled) {
> +    //
> +    // The QEMU fw_cfg variable tells us not to load this image.  So abort.
> +    //
> +    Status = EFI_ABORTED;
> +  } else {
> +    //
> +    // Call the driver entry point
> +    //
> +    Status = ProcessModuleEntryPointList (ImageHandle, SystemTable);
> +  }

Right, I think this is the way to do it -- we've constructed all the lib
instances, so now we can rely on PCDs and QemuFwCfg.

I'm OK with this version, just one idea: in order to avoid the code
duplication (and to benefit from future improvements to the original
entry point lib in MdePkg), we could introduce a new hook API to the
original -- forwarding the ImageHandle and SystemTable parameters to it
--, provide a Null instance implementation for the API, for the general
case, in MdePkg, and provide an fw_cfg-based implementation for OVMF /
ArmVirt.

I think it's worth a try; if Michael, Liming and Zhiguang don't like the
bit of additional complexity in MdePkg, you can still merge this
version. If you don't want to patch MdePkg though, I won't insist, of
course.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Laszlo

> +
> +  //
> +  // If all of the drivers returned errors, or we if are aborting, then invoke
> +  // all of the library destructors
> +  //
> +  if (EFI_ERROR (Status)) {
> +    ProcessLibraryDestructorList (ImageHandle, SystemTable);
> +  }
> +
> +  //
> +  // Return the cumulative return status code from all of the driver entry
> +  // points
> +  //
> +  return Status;
> +}
> diff --git a/OvmfPkg/Library/UefiDriverEntryPointFwCfgOverrideLib/UefiDriverEntryPointFwCfgOverrideLib.inf b/OvmfPkg/Library/UefiDriverEntryPointFwCfgOverrideLib/UefiDriverEntryPointFwCfgOverrideLib.inf
> new file mode 100644
> index 000000000000..263e00ceef66
> --- /dev/null
> +++ b/OvmfPkg/Library/UefiDriverEntryPointFwCfgOverrideLib/UefiDriverEntryPointFwCfgOverrideLib.inf
> @@ -0,0 +1,57 @@
> +## @file
> +#  Entry point to a EFI/DXE driver. This version is specific to QEMU, and ties
> +#  dispatch of the driver in question on the value of a QEMU fw_cfg boolean
> +#  variable which is referenced by name via a fixed pointer PCD.
> +#
> +# Copyright (c) 2007 - 2018, Intel Corporation. All rights reserved.<BR>
> +# Copyright (c) 2022, Google LLC. All rights reserved.<BR>
> +#
> +#  SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +#
> +##
> +
> +[Defines]
> +  INF_VERSION                    = 1.29
> +  BASE_NAME                      = UefiDriverEntryPointFwCfgOverrideLib
> +  FILE_GUID                      = 73349b79-f148-43b8-b24e-9098a6f3e1db
> +  MODULE_TYPE                    = UEFI_DRIVER
> +  VERSION_STRING                 = 1.0
> +  LIBRARY_CLASS                  = UefiDriverEntryPoint|DXE_DRIVER DXE_RUNTIME_DRIVER UEFI_DRIVER
> +
> +[Sources]
> +  UefiDriverEntryPointFwCfgOverrideLib.c
> +
> +[Packages]
> +  MdePkg/MdePkg.dec
> +  OvmfPkg/OvmfPkg.dec
> +
> +[LibraryClasses]
> +  BaseLib
> +  DebugLib
> +  QemuFwCfgSimpleParserLib
> +  UefiBootServicesTableLib
> +
> +[Protocols]
> +  gEfiLoadedImageProtocolGuid                   ## SOMETIMES_CONSUMES
> +
> +[FixedPcd]
> +  gUefiOvmfPkgTokenSpaceGuid.PcdEntryPointOverrideFwCfgVarName
> +
> +#
> +# For UEFI drivers, these architectural protocols defined in PI 1.0 spec need
> +# to be appended and merged to the final dependency section.
> +#
> +[Depex.common.UEFI_DRIVER]
> +  gEfiBdsArchProtocolGuid AND
> +  gEfiCpuArchProtocolGuid AND
> +  gEfiMetronomeArchProtocolGuid AND
> +  gEfiMonotonicCounterArchProtocolGuid AND
> +  gEfiRealTimeClockArchProtocolGuid AND
> +  gEfiResetArchProtocolGuid AND
> +  gEfiRuntimeArchProtocolGuid AND
> +  gEfiSecurityArchProtocolGuid AND
> +  gEfiTimerArchProtocolGuid AND
> +  gEfiVariableWriteArchProtocolGuid AND
> +  gEfiVariableArchProtocolGuid AND
> +  gEfiWatchdogTimerArchProtocolGuid
> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
> index 5af76a540529..9816aa41377d 100644
> --- a/OvmfPkg/OvmfPkg.dec
> +++ b/OvmfPkg/OvmfPkg.dec
> @@ -399,6 +399,10 @@ [PcdsFixedAtBuild]
>    ## The Tdx accept page size. 0x1000(4k),0x200000(2M)
>    gUefiOvmfPkgTokenSpaceGuid.PcdTdxAcceptPageSize|0x200000|UINT32|0x65
>  
> +  ## The QEMU fw_cfg variable that UefiDriverEntryPointFwCfgOverrideLib will
> +  #  check to decide whether to abort dispatch of the driver it is linked into.
> +  gUefiOvmfPkgTokenSpaceGuid.PcdEntryPointOverrideFwCfgVarName|""|VOID*|0x68
> +
>  [PcdsDynamic, PcdsDynamicEx]
>    gUefiOvmfPkgTokenSpaceGuid.PcdEmuVariableEvent|0|UINT64|2
>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashVariablesEnable|FALSE|BOOLEAN|0x10
>