From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mx.groups.io with SMTP id smtpd.web10.8237.1591165969930208030 for ; Tue, 02 Jun 2020 23:32:50 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: arm.com, ip: 217.140.110.172, mailfrom: ard.biesheuvel@arm.com) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 850CC55D; Tue, 2 Jun 2020 23:32:48 -0700 (PDT) Received: from [192.168.1.69] (unknown [10.37.8.209]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 7109F3F6CF; Tue, 2 Jun 2020 23:32:47 -0700 (PDT) Subject: Re: [edk2-devel] [PATCH edk2-platforms v2] Silicon/ChaosKeyDxe: don't rely on connect all controllers To: Andrew Fish , edk2-devel-groups-io , Ard Biesheuvel , Mike Kinney Cc: leif@nuviainc.com References: <20200602133849.5422-1-ard.biesheuvel@linaro.org> <77023D58-E75D-48E8-B5CE-A51FA08FE9C7@apple.com> From: "Ard Biesheuvel" Message-ID: Date: Wed, 3 Jun 2020 08:32:44 +0200 User-Agent: Mozilla/5.0 (X11; Linux aarch64; rv:68.0) Gecko/20100101 Thunderbird/68.8.1 MIME-Version: 1.0 In-Reply-To: <77023D58-E75D-48E8-B5CE-A51FA08FE9C7@apple.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: quoted-printable On 6/3/20 12:28 AM, Andrew Fish wrote: >=20 >=20 >> On Jun 2, 2020, at 6:38 AM, Ard Biesheuvel = wrote: >> >> From: Ard Biesheuvel >> >> 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 >> --- >> >> Unfortunately, the previous approach of connecting a short-form USB dev= ice >> 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 instantiat= ed >> by the DXE core. This is what typically happens, given that USB keyboar= ds >> are also exposed via USB I/O protocol instances. The only difference wi= th >> 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 t= o >> 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 d= o >> exist, it is not up to this 3rd party driver to override this policy an= d >> enumerate the entire USB hierarchy. >> >> So this means we need to call UsbHwrngDriverBindingSupported() directly >> 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 drive= r >> to the controller. >> >=20 > Actually there is a point in using gBS->ConnectController(). > 1) You make it impossible for the platform to override the drivers choic= e. The philosophy of EFI has alway been that the drivers should not cary po= licy and the platforms should cary policy. This inverts that. > 2) I=E2=80=99m not sure it is well defined behavior by UEFI to NOT call = gBS->ConnectController(). I took a quick look at the DXE Core and the gBS->= ConnectController() is not doing any book keeping, other than managing the = precedence rules, but it seems like it would be legal to add something in t= he future. + Mike in case I=E2=80=99m off on this one. >=20 The idea is really that by loading this driver, you are indicating that=20 you want it to connect to any supporting controller and produce the RNG=20 protocol, despite the platform policy. The UEFI driver model simply=20 isn't a great fit here, since the RNG is never a boot target and you=20 just want it to connect all the time. It is a shame my first approach did not work: it attempted to connect=20 the short form USB class device path carrying the VID and PID of this=20 controller as the remaining device path on each detected USB host=20 controller, similar to how USB keyboards are connected in the upstream=20 BDS code. But yes, ConnectController() should also work - it will invoke the=20 Supported() method again, and call Start(). I did check that doing this=20 directly amounts to the same in terms of bookkeeping, but I agree that=20 this could potentially change at some point. However, the question is=20 whether connecting the controller to another, higher priority driver is=20 the right thing to do in this case. It is really hard to answer that=20 question in general terms. > Also It would like be a good idea to have a PCD to turn on/off this feat= ure. >=20 Fair enough. > Thanks, >=20 > Andrew Fish >=20 >> 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/Ope= nmoko/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 =3D= { >> }; >> >> >> +STATIC >> +VOID >> +EFIAPI >> +UsbHwrngOnProtocolNotify ( >> + IN EFI_EVENT Event, >> + IN VOID *Context >> + ) >> +{ >> + EFI_STATUS Status; >> + EFI_HANDLE *Handles; >> + UINTN HandleCount; >> + UINTN Index; >> + >> + do { >> + Status =3D gBS->LocateHandleBuffer (ByRegisterNotify, NULL, >> + mProtocolNotifyRegistration, &HandleCount, &Handle= s); >> + if (EFI_ERROR (Status)) { >> + if (Status !=3D EFI_NOT_FOUND) { >> + DEBUG ((DEBUG_WARN, "%a: LocateHandleBuffer() failed - %r\n", >> + __FUNCTION__, Status)); >> + } >> + return; >> + } >> + >> + if (HandleCount =3D=3D 0) { >> + return; >> + } >> + >> + for (Index =3D 0; Index < HandleCount; Index++) { >> + Status =3D UsbHwrngDriverBindingSupported (&gUsbDriverBinding, >> + Handles[Index], NULL); >> + if (EFI_ERROR (Status)) { >> + continue; >> + } >> + >> + // >> + // Attempt to connect the USB device path describing the ChaosKe= y >> + // hardware via the handle describing a USB host controller. >> + // >> + Status =3D UsbHwrngDriverBindingStart (&gUsbDriverBinding, >> + Handles[Index], NULL); >> + DEBUG ((DEBUG_VERBOSE, "%a: UsbHwrngDriverBindingStart () return= ed %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 prov= ide >> + // a boot target. This means that it will not get connected on an or= dinary >> + // 'fast' boot (which only connects the console and boot entry devic= e paths) >> + // unless we take extra measures. >> + // >> + mProtocolNotifyRegistrationEvent =3D EfiCreateProtocolNotifyEvent ( >> + &gEfiUsbIoProtocolGuid, TPL_CAL= LBACK, >> + 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 >> // >> --=20 >> 2.20.1 >> >> >>=20 >> >=20