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 16:58:54 +0200, Ard Biesheuvel wrote:
> 
> 
> On 6/2/20 4:49 PM, Leif Lindholm wrote:
> > 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?
> > 
> 
> No. The driver does not know whether the controller is there or not unless
> the platform enumerates the USB hierarchy, at least non-recursively. If the
> USB I/O handles are not produced, this driver will not find the controller.
> 
> If on such a platform, you are using Driver#### to load the driver, you can
> set the reconnect-all flag to work around this.

Right. OK, I agree that's fair enough.
It is a shame though - the support for --reconnect was only added to
efibootmgr on 11 October last year, so isn't currently available in
most distros.

> If the platform firmware incorporates this driver, it needs to take this
> limitation into account, and ensure that the USB I/O handles are produced in
> one way or the other (which it will typically already do while looking for
> the USB keyboard)

Agreed.

> Adding code to this driver to allow it to infer from its execution context
> which of these situations it finds itself in seems intractible to me,
> especially given my recent experiences with AMI firmware, which does not
> quite behave like EDK2 upstream does in this regard.

Yeah. OK, taken all of these points into account:
Reviewed-by: Leif Lindholm <leif@nuviainc.com>

/
    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
> > >