From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from ma1-aaemail-dr-lapp01.apple.com (ma1-aaemail-dr-lapp01.apple.com [17.171.2.60]) by mx.groups.io with SMTP id smtpd.web11.1405.1591136927212311782 for ; Tue, 02 Jun 2020 15:28:47 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@apple.com header.s=20180706 header.b=VnA3ClCj; spf=pass (domain: apple.com, ip: 17.171.2.60, mailfrom: afish@apple.com) Received: from pps.filterd (ma1-aaemail-dr-lapp01.apple.com [127.0.0.1]) by ma1-aaemail-dr-lapp01.apple.com (8.16.0.42/8.16.0.42) with SMTP id 052MALsB031692; Tue, 2 Jun 2020 15:28:44 -0700 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=apple.com; h=content-type : mime-version : subject : from : in-reply-to : date : cc : content-transfer-encoding : message-id : references : to; s=20180706; bh=wbOuY5X8AbW/XmR5qLZubAV+32ygpPeaRsnL48b5PH8=; b=VnA3ClCjT7WoE2vJQwvJNX48Qee5zo+cgVhrKIE479eOqOIq+A1vVH8NM0sr/+q0eIGL VbYjqfnPDwQcUO9gAQU30uZpV91X7VrdRcfRPsZXRArYXn6zlUcoHcIK8mwfYzuAxDeL uy8fY2yn6bA6Uc5Q9Ssf4KMtNxY9I2x7Iy0jAGQ/M5GNO8kl+zxOPCiBpVdLH5c9R7m5 wkrMivMhTNk+HAKrHQcoldbJUjQhDUCCG9azQWJwO7zN6Lht1WEIJLhH5XV+VDqMBSHT hX2gWAJ8jas+gWyCseOaZV1puczurM3XqIoBlwdnulz2H/GhyDg73VP5dyTH6jekFFL9 Ug== Received: from rn-mailsvcp-mta-lapp01.rno.apple.com (rn-mailsvcp-mta-lapp01.rno.apple.com [10.225.203.149]) by ma1-aaemail-dr-lapp01.apple.com with ESMTP id 31bp52fen2-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128 verify=NO); Tue, 02 Jun 2020 15:28:44 -0700 Received: from rn-mailsvcp-mmp-lapp03.rno.apple.com (rn-mailsvcp-mmp-lapp03.rno.apple.com [17.179.253.16]) by rn-mailsvcp-mta-lapp01.rno.apple.com (Oracle Communications Messaging Server 8.1.0.5.20200312 64bit (built Mar 12 2020)) with ESMTPS id <0QBB001OBL3VXAB0@rn-mailsvcp-mta-lapp01.rno.apple.com>; Tue, 02 Jun 2020 15:28:43 -0700 (PDT) Received: from process_milters-daemon.rn-mailsvcp-mmp-lapp03.rno.apple.com by rn-mailsvcp-mmp-lapp03.rno.apple.com (Oracle Communications Messaging Server 8.1.0.5.20200312 64bit (built Mar 12 2020)) id <0QBB00E00KLZND00@rn-mailsvcp-mmp-lapp03.rno.apple.com>; Tue, 02 Jun 2020 15:28:43 -0700 (PDT) X-Va-A: X-Va-T-CD: e0acb9dc03d22e4581b62f3d752335f3 X-Va-E-CD: 74822e623c64803e02a0556b56b27a0d X-Va-R-CD: 776fe2063c7b471a9f99d46203d58303 X-Va-CD: 0 X-Va-ID: 33d45fad-d628-4f64-ad08-ad3c9f28c40c X-V-A: X-V-T-CD: e0acb9dc03d22e4581b62f3d752335f3 X-V-E-CD: 74822e623c64803e02a0556b56b27a0d X-V-R-CD: 776fe2063c7b471a9f99d46203d58303 X-V-CD: 0 X-V-ID: 43349542-36c7-4c66-adfa-74d23a4ea034 X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.216,18.0.687 definitions=2020-06-02_15:2020-06-02,2020-06-02 signatures=0 Received: from [17.235.27.245] (unknown [17.235.27.245]) by rn-mailsvcp-mmp-lapp03.rno.apple.com (Oracle Communications Messaging Server 8.1.0.5.20200312 64bit (built Mar 12 2020)) with ESMTPSA id <0QBB00T9NL3TP700@rn-mailsvcp-mmp-lapp03.rno.apple.com>; Tue, 02 Jun 2020 15:28:43 -0700 (PDT) MIME-version: 1.0 (Mac OS X Mail 13.4 \(3608.80.23.2.2\)) Subject: Re: [edk2-devel] [PATCH edk2-platforms v2] Silicon/ChaosKeyDxe: don't rely on connect all controllers From: "Andrew Fish" In-reply-to: <20200602133849.5422-1-ard.biesheuvel@linaro.org> Date: Tue, 02 Jun 2020 15:28:40 -0700 Cc: leif@nuviainc.com, Ard Biesheuvel Message-id: <77023D58-E75D-48E8-B5CE-A51FA08FE9C7@apple.com> References: <20200602133849.5422-1-ard.biesheuvel@linaro.org> To: edk2-devel-groups-io , Ard Biesheuvel , Mike Kinney X-Mailer: Apple Mail (2.3608.80.23.2.2) X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.216,18.0.687 definitions=2020-06-02_15:2020-06-02,2020-06-02 signatures=0 Content-type: text/plain; charset=utf-8 Content-transfer-encoding: quoted-printable > On Jun 2, 2020, at 6:38 AM, Ard Biesheuvel w= rote: >=20 > From: Ard Biesheuvel >=20 > 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. >=20 > 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) >=20 > 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. >=20 > Signed-off-by: Ard Biesheuvel > --- >=20 > Unfortunately, the previous approach of connecting a short-form USB devi= ce > 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) >=20 > So instead, we have to rely on USB I/O protocols having been instantiate= d > by the DXE core. This is what typically happens, given that USB keyboard= s > are also exposed via USB I/O protocol instances. The only difference wit= h > 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. >=20 > 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. >=20 > 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 driver > to the controller. >=20 Actually there is a point in using gBS->ConnectController().=20 1) You make it impossible for the platform to override the drivers choice.= The philosophy of EFI has alway been that the drivers should not cary poli= cy and the platforms should cary policy. This inverts that.=20 2) I=E2=80=99m not sure it is well defined behavior by UEFI to NOT call gB= S->ConnectController(). I took a quick look at the DXE Core and the gBS->Co= nnectController() is not doing any book keeping, other than managing the pr= ecedence rules, but it seems like it would be legal to add something in the= future. + Mike in case I=E2=80=99m off on this one.=20 Also It would like be a good idea to have a PCD to turn on/off this featur= e.=20 Thanks, Andrew Fish > Tested on ThunderX2 using a Driver0000 option. >=20 > Silicon/Openmoko/ChaosKeyDxe/DriverBinding.c | 66 ++++++++++++++++++++ > 1 file changed, 66 insertions(+) >=20 > diff --git a/Silicon/Openmoko/ChaosKeyDxe/DriverBinding.c b/Silicon/Open= moko/ChaosKeyDxe/DriverBinding.c > index e7d0d3fe563e..611803c6c339 100644 > --- a/Silicon/Openmoko/ChaosKeyDxe/DriverBinding.c > +++ b/Silicon/Openmoko/ChaosKeyDxe/DriverBinding.c > @@ -11,6 +11,9 @@ >=20 > #include "ChaosKeyDriver.h" >=20 > +STATIC VOID *mProtocolNotifyRegistration; > +STATIC EFI_EVENT mProtocolNotifyRegistrationEvent; > + > /** > Tests to see if this driver supports a given controller. >=20 > @@ -157,6 +160,55 @@ EFI_DRIVER_BINDING_PROTOCOL gUsbDriverBinding =3D = { > }; >=20 >=20 > +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, &Handles= ); > + 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 ChaosKey > + // hardware via the handle describing a USB host controller. > + // > + Status =3D UsbHwrngDriverBindingStart (&gUsbDriverBinding, > + Handles[Index], NULL); > + DEBUG ((DEBUG_VERBOSE, "%a: UsbHwrngDriverBindingStart () returne= d %r\n", > + __FUNCTION__, Status)); > + } > + gBS->FreePool (Handles); > + } while (1); > +} > + > + > /** > The entry point of ChaosKey UEFI Driver. >=20 > @@ -185,6 +237,18 @@ EntryPoint ( > NULL, &gChaosKeyDriverComponentName2); > ASSERT_EFI_ERROR (Status); >=20 > + // > + // This driver produces the EFI Random Number Generator protocol on > + // compatible USB I/O handles, which is not a protocol that can provi= de > + // a boot target. This means that it will not get connected on an ord= inary > + // 'fast' boot (which only connects the console and boot entry device= paths) > + // unless we take extra measures. > + // > + mProtocolNotifyRegistrationEvent =3D EfiCreateProtocolNotifyEvent ( > + &gEfiUsbIoProtocolGuid, TPL_CALL= BACK, > + UsbHwrngOnProtocolNotify, NULL, > + &mProtocolNotifyRegistration); > + > DEBUG ((DEBUG_INIT | DEBUG_INFO, "*** Installed ChaosKey driver! ***\n= ")); >=20 > return EFI_SUCCESS; > @@ -211,6 +275,8 @@ UnloadImage ( > UINTN HandleCount; > UINTN Index; >=20 > + gBS->CloseEvent (mProtocolNotifyRegistrationEvent); > + > // > // Retrieve all USB I/O handles in the handle database > // > --=20 > 2.20.1 >=20 >=20 >=20 >=20