[edk2-devel] [PATCH 1/2] OvmfPkg: Introduce NULL class library to inhibit driver load
Laszlo Ersek
lersek at redhat.com
Tue Aug 16 12:30:08 UTC 2022
On 08/15/22 11:40, 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) typo? (should be "IPv4 and IPv6" I think)
> driver to be controlled from the QEMU command line.
>
> Signed-off-by: Ard Biesheuvel <ardb at kernel.org>
> ---
> OvmfPkg/Library/DriverLoadInhibitorLib/DriverLoadInhibitorLib.c | 30 ++++++++++++++++++++
> OvmfPkg/Library/DriverLoadInhibitorLib/DriverLoadInhibitorLib.inf | 28 ++++++++++++++++++
> OvmfPkg/OvmfPkg.dec | 4 +++
> 3 files changed, 62 insertions(+)
>
> diff --git a/OvmfPkg/Library/DriverLoadInhibitorLib/DriverLoadInhibitorLib.c b/OvmfPkg/Library/DriverLoadInhibitorLib/DriverLoadInhibitorLib.c
> new file mode 100644
> index 000000000000..dc8544bc38be
> --- /dev/null
> +++ b/OvmfPkg/Library/DriverLoadInhibitorLib/DriverLoadInhibitorLib.c
> @@ -0,0 +1,30 @@
> +// @file
> +// Copyright (c) 2022, Google LLC. All rights reserved.<BR>
> +// SPDX-License-Identifier: BSD-2-Clause-Patent
> +//
> +
> +#include <PiDxe.h>
> +
> +#include <Library/QemuFwCfgSimpleParserLib.h>
> +#include <Library/UefiBootServicesTableLib.h>
> +
> +STATIC CHAR16 mExitData[] = L"Driver dispatch inhibited by QEMU fw_cfg variable.";
> +
> +EFI_STATUS
> +EFIAPI
> +DriverLoadInhibitorLibConstructor (
> + IN EFI_HANDLE Handle,
> + IN EFI_SYSTEM_TABLE *SystemTable
> + )
> +{
> + RETURN_STATUS Status;
> + BOOLEAN Enabled;
> +
> + Status = QemuFwCfgParseBool (FixedPcdGetPtr (PcdDriverInhibitorFwCfgVarName),
> + &Enabled);
> + if (!RETURN_ERROR (Status) && !Enabled) {
> + return gBS->Exit (Handle, EFI_REQUEST_UNLOAD_IMAGE, sizeof mExitData,
> + mExitData);
(2) Per UEFI spec, ExitData should be allocated with
gBS->AllocatePool().
(3) EFI_REQUEST_UNLOAD_IMAGE is from the PI spec; while not wrong, I
think it's strange to use here. EFI_ABORTED or something similar from
the UEFI spec would be a better fit IMO.
(4) And then, the big problem:
I agree that returning an error from the constructor would not be
beneficial, as it would cause an assertion to fail in the
ProcessLibraryConstructorList() function, in the generated "AutoGen.c"
file.
However, calling gBS->Exit() from a constructor seems unsafe to me, with
regard to library destructors.
Now, in the current case (considering patch#2), this unsafety is not
visible. That's because:
(quoting ProcessLibraryConstructorList() and
ProcessLibraryDestructorList() from
"Build/OvmfX64/NOOPT_GCC5/X64/NetworkPkg/Ip4Dxe/Ip4Dxe/DEBUG/AutoGen.c",
from an earlier build on my machine anyway):
> VOID
> EFIAPI
> ProcessLibraryConstructorList (
> IN EFI_HANDLE ImageHandle,
> IN EFI_SYSTEM_TABLE *SystemTable
> )
> {
> EFI_STATUS Status;
>
> Status = PlatformDebugLibIoPortConstructor ();
> ASSERT_RETURN_ERROR (Status);
>
> Status = UefiBootServicesTableLibConstructor (ImageHandle, SystemTable);
> ASSERT_EFI_ERROR (Status);
>
> Status = DevicePathLibConstructor (ImageHandle, SystemTable);
> ASSERT_EFI_ERROR (Status);
>
> Status = UefiRuntimeServicesTableLibConstructor (ImageHandle, SystemTable);
> ASSERT_EFI_ERROR (Status);
>
> Status = UefiLibConstructor (ImageHandle, SystemTable);
> ASSERT_EFI_ERROR (Status);
>
> Status = UefiHiiServicesLibConstructor (ImageHandle, SystemTable);
> ASSERT_EFI_ERROR (Status);
>
> Status = DpcLibConstructor (ImageHandle, SystemTable);
> ASSERT_EFI_ERROR (Status);
>
> }
>
>
>
> VOID
> EFIAPI
> ProcessLibraryDestructorList (
> IN EFI_HANDLE ImageHandle,
> IN EFI_SYSTEM_TABLE *SystemTable
> )
> {
>
> }
there is no library destruction to speak of -- none of the used library
instances have resources they need to release at destruction time.
However, the general case looks problematic. The new library constructor
call would be inserted *somewhere* in ProcessLibraryConstructorList() --
the insertion point is likely "mostly unspecified", as no library
instance depends on DriverLoadInhibitorLib, and DriverLoadInhibitorLib
seems to depend on relatively few lib classes too. Therefore, in theory
anyway, the new lib constructor could call gBS->Exit() somewhere in the
middle of ProcessLibraryConstructorList(), with only some of the library
constructors having been executed.
Then the questions are
- does gBS->Exit() call ProcessLibraryDestructorList() or not?
- if it does not, that could lead to memory leaks.
- If it does though, is ProcessLibraryDestructorList() smart enough to
call only those destructors whose constructors have previously run?
- If not, it could call destructors on never-constructed data.
Unfortunately, this looks really tough to figure out; testing it (with
some actual library destructors) could be easier.
FWIW, there are two call sites for ProcessLibraryDestructorList() (for
UEFI/DXE drivers anyway); both in
"MdePkg/Library/UefiDriverEntryPoint/DriverEntryPoint.c":
- One is inside the _ModuleEntryPoint() function.
This call is reached only when the function designated as ENTRY_POINT
in the driver's INF file returns (note, said function is not the
actual entry point function of the driver -- the actual entry point is
the _ModuleEntryPoint() function).
When gBS->Exit() is called, the CoreExit() function
[MdeModulePkg/Core/Dxe/Image/Image.c] long-jumps back to
CoreStartImage(), and no part of the driver's _ModuleEntryPoint() is
again used. So the first ProcessLibraryDestructorList() call site,
namely the one in ModuleEntryPoint(), is not reached when gBS->Exit()
is called.
- The other ProcessLibraryDestructorList() call site is in
_DriverUnloadHandler()
[MdePkg/Library/UefiDriverEntryPoint/DriverEntryPoint.c].
Now it's not easy at all to say whether gBS->Exit() utilizes this
function or not, when it unloads the image (because, per spec,
gBS->Exit() *is* responsible for unloading the image).
However, we need not track that down right now, to see that the
proposed patch is unsafe in this aspect. That's because
_ModuleEntryPoint() does the following:
> //
> // 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;
> }
In other words, even if CoreExit() might call Unload -->
_DriverUnloadHandler() --> ProcessLibraryDestructorList() somewhere,
_ModuleEntryPoint() sets "Unload" to "_DriverUnloadHandler" only
*after* ProcessLibraryConstructorList() returns. And the proposed
patch calls gBS->Exit() from *inside* ProcessLibraryConstructorList(),
that is, when "Unload" is not set yet.
On physical machines, I've seen firmware options for disabling the IP
stack entirely; I wonder how those firmwares do it...
Laszlo
On 08/15/22 11:40, Ard Biesheuvel wrote:
> + }
> + return EFI_SUCCESS;
> +}
> diff --git a/OvmfPkg/Library/DriverLoadInhibitorLib/DriverLoadInhibitorLib.inf b/OvmfPkg/Library/DriverLoadInhibitorLib/DriverLoadInhibitorLib.inf
> new file mode 100644
> index 000000000000..ed521d12d335
> --- /dev/null
> +++ b/OvmfPkg/Library/DriverLoadInhibitorLib/DriverLoadInhibitorLib.inf
> @@ -0,0 +1,28 @@
> +## @file
> +# Copyright (c) 2022, Google LLC. All rights reserved.<BR>
> +# SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +##
> +
> +[Defines]
> + INF_VERSION = 1.29
> + BASE_NAME = DriverLoadInhibitorLib
> + FILE_GUID = af4c2c0b-f7ed-4d61-ad97-5953982c3531
> + MODULE_TYPE = DXE_DRIVER
> + VERSION_STRING = 1.0
> + LIBRARY_CLASS = NULL
> + CONSTRUCTOR = DriverLoadInhibitorLibConstructor
> +
> +[Sources]
> + DriverLoadInhibitorLib.c
> +
> +[LibraryClasses]
> + QemuFwCfgSimpleParserLib
> + UefiBootServicesTableLib
> +
> +[Packages]
> + MdePkg/MdePkg.dec
> + OvmfPkg/OvmfPkg.dec
> +
> +[FixedPcd]
> + gUefiOvmfPkgTokenSpaceGuid.PcdDriverInhibitorFwCfgVarName
> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
> index 5af76a540529..e9a22cab088c 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 DriverLoadInhibitorLib will check to
> + # decide whether to abort dispatch of the driver it is linked into.
> + gUefiOvmfPkgTokenSpaceGuid.PcdDriverInhibitorFwCfgVarName|""|VOID*|0x68
> +
> [PcdsDynamic, PcdsDynamicEx]
> gUefiOvmfPkgTokenSpaceGuid.PcdEmuVariableEvent|0|UINT64|2
> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashVariablesEnable|FALSE|BOOLEAN|0x10
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#92476): https://edk2.groups.io/g/devel/message/92476
Mute This Topic: https://groups.io/mt/93032846/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list