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.biesheu...@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.biesheu...@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 <l...@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
> > >
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#60608): https://edk2.groups.io/g/devel/message/60608
Mute This Topic: https://groups.io/mt/74627343/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
