Re: [PATCH edk2-platforms v2] Silicon/ChaosKeyDxe: don't rely on connect all controllers

["Leif Lindholm" <leif@nuviainc.com>]

On Tue, Jun 02, 2020 at 15:38:49 +0200, Ard Biesheuvel wrote:
> From: Ard Biesheuvel <ard.biesheuvel@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@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
>