From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mx.groups.io with SMTP id smtpd.web10.38798.1660802320768314312 for ; Wed, 17 Aug 2022 22:58:41 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=XW3zny0/; spf=pass (domain: redhat.com, ip: 170.10.133.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1660802319; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=/3eGXOoU5hKSYLvLhQVxp5KYyFftLRZ9gV9ptdLU0Vo=; b=XW3zny0/kRvaY10pj0+6Ebv+ii40NVmCMiUOg1yqTnD0mUH2w0zWVMHgLscBgvwS2Bu76E fwfd6Qd9Xp4W/ottP0PDtXuZ9lqP1L44s29AGopWuyHNOYv62JAyu/wmkl0cNu+o7oUG32 YX7Uehc4y41R+VpxcpBTMocxIU6zEtc= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-648-_6qkgJwRN8S3ypaRd2bhUw-1; Thu, 18 Aug 2022 01:58:35 -0400 X-MC-Unique: _6qkgJwRN8S3ypaRd2bhUw-1 Received: from smtp.corp.redhat.com (int-mx10.intmail.prod.int.rdu2.redhat.com [10.11.54.10]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 8C86985A589; Thu, 18 Aug 2022 05:58:35 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (unknown [10.39.192.125]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 06B8B492C3B; Thu, 18 Aug 2022 05:58:33 +0000 (UTC) Subject: Re: [PATCH v2 1/2] OvmfPkg: Introduce alternate UefiDriverEntrypoint to inhibit driver load To: Ard Biesheuvel , devel@edk2.groups.io Cc: Yuan Yu , Gerd Hoffmann , Pawel Polawski , Oliver Steffen , Jiewen Yao , "Brian J . Johnson" References: <20220817151157.1941409-1-ardb@kernel.org> <20220817151157.1941409-2-ardb@kernel.org> From: "Laszlo Ersek" Message-ID: <08e278c5-4e7a-b91d-526f-315cfa3398b8@redhat.com> Date: Thu, 18 Aug 2022 07:58:32 +0200 MIME-Version: 1.0 In-Reply-To: <20220817151157.1941409-2-ardb@kernel.org> X-Scanned-By: MIMEDefang 2.85 on 10.11.54.10 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 > --- > 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.
> +Copyright (c) 2022, Google LLC. All rights reserved.
> +SPDX-License-Identifier: BSD-2-Clause-Patent > + > +**/ > + > +#include > + > +#include > + > +#include > +#include > +#include > +#include > +#include > + > +/** > + 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 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.
> +# Copyright (c) 2022, Google LLC. All rights reserved.
> +# > +# 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 >