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.
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)
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.
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 (#60591): https://edk2.groups.io/g/devel/message/60591 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] -=-=-=-=-=-=-=-=-=-=-=-
