[edk2-devel] [PATCH edk2-platforms v2] Silicon/ChaosKeyDxe: don't rely on connect all controllers
Leif Lindholm
leif at nuviainc.com
Tue Jun 2 14:49:21 UTC 2020
On Tue, Jun 02, 2020 at 15:38:49 +0200, Ard Biesheuvel wrote:
> From: Ard Biesheuvel <ard.biesheuvel at arm.com>
>
> The ChaosKey driver implements the UEFI driver model, and so it is
> not guaranteed that any controllers will be attached to this driver
> unless it is connected explicitly. On many platforms today, this is
> taken care of by the ConnectAll() call that occurs in the BDS, but
> this is not something we should rely on.
>
> So add a protocol notification event that will attempt to connect
> the ChaosKey on each USB I/O protocol instance that carries the
> expected vendor and product IDs. This still relies on the USB host
> controllers to be connected by the platform, which is typically the
> case (given that a USB keyboard is required to interrupt the boot)
>
> On platforms where USB is not connected at all by default, it is really
> not up to a third party driver to meddle with this, and so relying on
> the USB host controllers to have been connected non-reursively is the
> best we can do. Note that third party drivers registered via Driver####
> can set a 'reconnect all' flag if needed to mitigate this.
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel at arm.com>
> ---
>
> Unfortunately, the previous approach of connecting a short-form USB device
> path containing the ChaosKey's VID/PID turned out not to be reliable in
> practice (it doesn't work at all on ThunderX2 with AMI firmware)
>
> So instead, we have to rely on USB I/O protocols having been instantiated
> by the DXE core. This is what typically happens, given that USB keyboards
> are also exposed via USB I/O protocol instances. The only difference with
> a non-fast [ConnectAll()] boot is that those USB I/O protocol instances
> are not connected further automatically, but it is reasonable to expect
> that the handles themselves have been instantiated.
>
> Platforms that do not produce those USB I/O handles would not be able to
> offer the ability to interrupt the boot or enter the menu using a USB
> keyboard, so this is rather rare in practice. Also, if such platforms do
> exist, it is not up to this 3rd party driver to override this policy and
> enumerate the entire USB hierarchy.
I agree in theory.
But what happens when someone explicitly sets up their ChaosKey with a
standalone driver, verifies it working, and at some later date enable
"quick boot" in a BIOS menu?
(I recall some "fun" stories from Peter Jones in this area,
specifically wrt the ability to interrupt boot from a keyboard.)
I'm not saying it should up to a 3rd party driver to override this
policy and enumerate the entire USB hierarchy, but I'm suggesting that
especially for something like ChaosKey, silent failure could be a real
problem.
Does this workaround prevent that?
/
Leif
> to check whether the USB I/O protocol in question has the right VID/PID.
> If that succeeds, there is really no point in using ConnectController(),
> so just call UsbHwrngDriverBindingStart() directly to connect the driver
> to the controller.
>
> Tested on ThunderX2 using a Driver0000 option.
> Silicon/Openmoko/ChaosKeyDxe/DriverBinding.c | 66 ++++++++++++++++++++
> 1 file changed, 66 insertions(+)
>
> diff --git a/Silicon/Openmoko/ChaosKeyDxe/DriverBinding.c b/Silicon/Openmoko/ChaosKeyDxe/DriverBinding.c
> index e7d0d3fe563e..611803c6c339 100644
> --- a/Silicon/Openmoko/ChaosKeyDxe/DriverBinding.c
> +++ b/Silicon/Openmoko/ChaosKeyDxe/DriverBinding.c
> @@ -11,6 +11,9 @@
>
> #include "ChaosKeyDriver.h"
>
> +STATIC VOID *mProtocolNotifyRegistration;
> +STATIC EFI_EVENT mProtocolNotifyRegistrationEvent;
> +
> /**
> Tests to see if this driver supports a given controller.
>
> @@ -157,6 +160,55 @@ EFI_DRIVER_BINDING_PROTOCOL gUsbDriverBinding = {
> };
>
>
> +STATIC
> +VOID
> +EFIAPI
> +UsbHwrngOnProtocolNotify (
> + IN EFI_EVENT Event,
> + IN VOID *Context
> + )
> +{
> + EFI_STATUS Status;
> + EFI_HANDLE *Handles;
> + UINTN HandleCount;
> + UINTN Index;
> +
> + do {
> + Status = gBS->LocateHandleBuffer (ByRegisterNotify, NULL,
> + mProtocolNotifyRegistration, &HandleCount, &Handles);
> + if (EFI_ERROR (Status)) {
> + if (Status != EFI_NOT_FOUND) {
> + DEBUG ((DEBUG_WARN, "%a: LocateHandleBuffer() failed - %r\n",
> + __FUNCTION__, Status));
> + }
> + return;
> + }
> +
> + if (HandleCount == 0) {
> + return;
> + }
> +
> + for (Index = 0; Index < HandleCount; Index++) {
> + Status = UsbHwrngDriverBindingSupported (&gUsbDriverBinding,
> + Handles[Index], NULL);
> + if (EFI_ERROR (Status)) {
> + continue;
> + }
> +
> + //
> + // Attempt to connect the USB device path describing the ChaosKey
> + // hardware via the handle describing a USB host controller.
> + //
> + Status = UsbHwrngDriverBindingStart (&gUsbDriverBinding,
> + Handles[Index], NULL);
> + DEBUG ((DEBUG_VERBOSE, "%a: UsbHwrngDriverBindingStart () returned %r\n",
> + __FUNCTION__, Status));
> + }
> + gBS->FreePool (Handles);
> + } while (1);
> +}
> +
> +
> /**
> The entry point of ChaosKey UEFI Driver.
>
> @@ -185,6 +237,18 @@ EntryPoint (
> NULL, &gChaosKeyDriverComponentName2);
> ASSERT_EFI_ERROR (Status);
>
> + //
> + // This driver produces the EFI Random Number Generator protocol on
> + // compatible USB I/O handles, which is not a protocol that can provide
> + // a boot target. This means that it will not get connected on an ordinary
> + // 'fast' boot (which only connects the console and boot entry device paths)
> + // unless we take extra measures.
> + //
> + mProtocolNotifyRegistrationEvent = EfiCreateProtocolNotifyEvent (
> + &gEfiUsbIoProtocolGuid, TPL_CALLBACK,
> + UsbHwrngOnProtocolNotify, NULL,
> + &mProtocolNotifyRegistration);
> +
> DEBUG ((DEBUG_INIT | DEBUG_INFO, "*** Installed ChaosKey driver! ***\n"));
>
> return EFI_SUCCESS;
> @@ -211,6 +275,8 @@ UnloadImage (
> UINTN HandleCount;
> UINTN Index;
>
> + gBS->CloseEvent (mProtocolNotifyRegistrationEvent);
> +
> //
> // Retrieve all USB I/O handles in the handle database
> //
> --
> 2.20.1
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#60589): https://edk2.groups.io/g/devel/message/60589
Mute This Topic: https://groups.io/mt/74627343/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