From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from ma1-aaemail-dr-lapp02.apple.com (ma1-aaemail-dr-lapp02.apple.com [17.171.2.68]) by mx.groups.io with SMTP id smtpd.web12.16794.1591199242642474253 for ; Wed, 03 Jun 2020 08:47:23 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@apple.com header.s=20180706 header.b=KOB/I+TF; spf=pass (domain: apple.com, ip: 17.171.2.68, mailfrom: afish@apple.com) Received: from pps.filterd (ma1-aaemail-dr-lapp02.apple.com [127.0.0.1]) by ma1-aaemail-dr-lapp02.apple.com (8.16.0.42/8.16.0.42) with SMTP id 053FctOS007729; Wed, 3 Jun 2020 08:47:19 -0700 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=apple.com; h=from : message-id : content-type : mime-version : subject : date : in-reply-to : cc : to : references; s=20180706; bh=LmhHZ4sA6l4GeV/NvaJ3A6BDz8LhxFUvSgd5jC6qdZY=; b=KOB/I+TFn4kSibtXvlPWROJIJjRTXfHIC8pPyXkda+GiMuZNsWwbAjbRAw+2CxtG2BoK 2DgyAGToGZY9rOy+/8LGU2qTToTlH/EPV8Yd3uKcQnLW/crOnLHyPTL1F2fKpDAugz/D 5O0At1V1tI4c+F/pQRA+3id2oGIZdhXBY/R4awe7c/El3Dh8XdYjWRY+nKsbnzls+ax5 ZBs0+2LVGKJPMt9ze7U8M83OgO0bYUfoSOktzS1vlwvBl2NCGt0j8cVunjWmpvTOUSti sR8DPpPZxo6olssS7Qc72t7mdSmZYtrHQ5iYWYbxqiYhMLCiW99YYBjilepSAnAaMUia nQ== Received: from rn-mailsvcp-mta-lapp01.rno.apple.com (rn-mailsvcp-mta-lapp01.rno.apple.com [10.225.203.149]) by ma1-aaemail-dr-lapp02.apple.com with ESMTP id 31bm5smm71-3 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128 verify=NO); Wed, 03 Jun 2020 08:47:19 -0700 Received: from rn-mailsvcp-mmp-lapp02.rno.apple.com (rn-mailsvcp-mmp-lapp02.rno.apple.com [17.179.253.15]) 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 <0QBC00RY4X6UCE70@rn-mailsvcp-mta-lapp01.rno.apple.com>; Wed, 03 Jun 2020 08:47:18 -0700 (PDT) Received: from process_milters-daemon.rn-mailsvcp-mmp-lapp02.rno.apple.com by rn-mailsvcp-mmp-lapp02.rno.apple.com (Oracle Communications Messaging Server 8.1.0.5.20200312 64bit (built Mar 12 2020)) id <0QBC00Y00X4ZNR00@rn-mailsvcp-mmp-lapp02.rno.apple.com>; Wed, 03 Jun 2020 08:47:18 -0700 (PDT) X-Va-A: X-Va-T-CD: 678bf7de5df0d9ff994f556fd1b44182 X-Va-E-CD: 74822e623c64803e02a0556b56b27a0d X-Va-R-CD: 776fe2063c7b471a9f99d46203d58303 X-Va-CD: 0 X-Va-ID: 09434260-66da-47fe-ae64-6dd1f659ad01 X-V-A: X-V-T-CD: 678bf7de5df0d9ff994f556fd1b44182 X-V-E-CD: 74822e623c64803e02a0556b56b27a0d X-V-R-CD: 776fe2063c7b471a9f99d46203d58303 X-V-CD: 0 X-V-ID: 0c86bbc8-3cb8-4d3e-a48e-97493afb7bac X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.216,18.0.687 definitions=2020-06-03_13:2020-06-02,2020-06-03 signatures=0 Received: from [17.235.27.245] (unknown [17.235.27.245]) by rn-mailsvcp-mmp-lapp02.rno.apple.com (Oracle Communications Messaging Server 8.1.0.5.20200312 64bit (built Mar 12 2020)) with ESMTPSA id <0QBC010LYX6SRA00@rn-mailsvcp-mmp-lapp02.rno.apple.com>; Wed, 03 Jun 2020 08:47:18 -0700 (PDT) From: "Andrew Fish" Message-id: 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 Date: Wed, 03 Jun 2020 08:47:16 -0700 In-reply-to: Cc: Ard Biesheuvel , Mike Kinney , leif@nuviainc.com To: devel@edk2.groups.io, ard.biesheuvel@arm.com References: <20200602133849.5422-1-ard.biesheuvel@linaro.org> <77023D58-E75D-48E8-B5CE-A51FA08FE9C7@apple.com> 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-03_13:2020-06-02,2020-06-03 signatures=0 Content-type: multipart/alternative; boundary="Apple-Mail=_A1ACC8E7-D5DA-4D99-907F-51F2239F8B8C" --Apple-Mail=_A1ACC8E7-D5DA-4D99-907F-51F2239F8B8C Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 > On Jun 2, 2020, at 11:32 PM, Ard Biesheuvel wro= te: >=20 > On 6/3/20 12:28 AM, Andrew Fish wrote: >>> On Jun 2, 2020, at 6:38 AM, Ard Biesheuvel = wrote: >>>=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 reall= y >>> 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 de= vice >>> path containing the ChaosKey's VID/PID turned out not to be reliable i= n >>> 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 instantia= ted >>> by the DXE core. This is what typically happens, given that USB keyboa= rds >>> are also exposed via USB I/O protocol instances. The only difference w= ith >>> a non-fast [ConnectAll()] boot is that those USB I/O protocol instance= s >>> are not connected further automatically, but it is reasonable to expec= t >>> 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 a= nd >>> enumerate the entire USB hierarchy. >>>=20 >>> So this means we need to call UsbHwrngDriverBindingSupported() directl= y >>> to check whether the USB I/O protocol in question has the right VID/PI= D. >>> If that succeeds, there is really no point in using ConnectController(= ), >>> so just call UsbHwrngDriverBindingStart() directly to connect the driv= er >>> 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 choi= ce. The philosophy of EFI has alway been that the drivers should not cary p= olicy 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 = the 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 = you want it to connect to any supporting controller and produce the RNG pro= tocol, despite the platform policy. The UEFI driver model simply isn't a gr= eat fit here, since the RNG is never a boot target and you just want it to = connect all the time. >=20 I understand the issue. On the fast boot path only things in the NVRAM var= iables, or hard coded as BDS policy get connected. For example in our custo= m BDS we have code that connects devices related to the T2 and we have cust= om code that only connects them at the known location. These devices produc= e services that other EFI Driver Model drivers use when they connect.=20 > It is a shame my first approach did not work: it attempted to connect th= e short form USB class device path carrying the VID and PID of this control= ler as the remaining device path on each detected USB host controller, simi= lar to how USB keyboards are connected in the upstream BDS code. >=20 Is there a BZ for that? I=E2=80=99ve gotten feedback over the years that o= ur BDS implementation is hard to understand. Maybe we should invest some ti= me in documentation on how to use it? Thanks, Andrew Fish > But yes, ConnectController() should also work - it will invoke the Suppo= rted() method again, and call Start(). I did check that doing this directly= amounts to the same in terms of bookkeeping, but I agree that this could p= otentially change at some point. However, the question is whether connectin= g the controller to another, higher priority driver is the right thing to d= o in this case. It is really hard to answer that question in general terms. >=20 >> Also It would like be a good idea to have a PCD to turn on/off this fea= ture. >=20 > Fair enough. >=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/Op= enmoko/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, &Handl= es); >>> + 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 ChaosK= ey >>> + // hardware via the handle describing a USB host controller. >>> + // >>> + Status =3D UsbHwrngDriverBindingStart (&gUsbDriverBinding, >>> + Handles[Index], NULL); >>> + DEBUG ((DEBUG_VERBOSE, "%a: UsbHwrngDriverBindingStart () retur= ned %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 pro= vide >>> + // a boot target. This means that it will not get connected on an o= rdinary >>> + // 'fast' boot (which only connects the console and boot entry devi= ce paths) >>> + // unless we take extra measures. >>> + // >>> + mProtocolNotifyRegistrationEvent =3D EfiCreateProtocolNotifyEvent ( >>> + &gEfiUsbIoProtocolGuid, TPL_CA= LLBACK, >>> + 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 >=20 >=20 --Apple-Mail=_A1ACC8E7-D5DA-4D99-907F-51F2239F8B8C Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8

On Jun 2, 20= 20, at 11:32 PM, Ard Biesheuvel <ard.biesheuvel@arm.com> wrote:

On 6/3/20 12:28 AM, Andrew Fis= h wrote:
<= blockquote type=3D"cite" class=3D"">On Jun 2, 2020, at 6:38 AM, Ard Biesheu= vel <ard.biesheu= vel@linaro.org> wrote:

From: Ard Bieshe= uvel <ard.biesheuve= l@arm.com>

The ChaosKey driver implemen= ts the UEFI driver model, and so it is
not guaranteed that an= y controllers will be attached to this driver
unless it is co= nnected explicitly. On many platforms today, this is
taken ca= re of by the ConnectAll() call that occurs in the BDS, but
th= is is not something we should rely on.

So add = a protocol notification event that will attempt to connect
th= e 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<= br class=3D"">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 drive= r to meddle with this, and so relying on
the USB host control= lers 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 connect= ing a short-form USB device
path containing the ChaosKey's VI= D/PID turned out not to be reliable in
practice (it doesn't w= ork 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 k= eyboards
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 au= tomatically, but it is reasonable to expect
that the handles = themselves have been instantiated.

Platforms t= hat 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 platf= orms do
exist, it is not up to this 3rd party driver to overr= ide this policy and
enumerate the entire USB hierarchy.

So this means we need to call UsbHwrngDriverBinding= Supported() directly
to check whether the USB I/O protocol in= question has the right VID/PID.
If that succeeds, there is r= eally no point in using ConnectController(),
so just call Usb= HwrngDriverBindingStart() directly to connect the driver
to t= he controller.

Actually there is = a point in using gBS->ConnectController().
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 policy and the platf= orms should cary policy. This inverts that.
2) I=E2=80=99m no= t sure it is well defined behavior by UEFI to NOT call gBS->ConnectContr= oller(). I took a quick look at the DXE Core and the gBS->ConnectControl= ler() is not doing any book keeping, other than managing the precedence rul= es, but it seems like it would be legal to add something in the future. + M= ike in case I=E2=80=99m off on this one.

The idea is really that by loading t= his driver, you are indicating that you want it to connect to any supportin= g controller and produce the RNG protocol, despite the platform policy. The= UEFI driver model simply isn't a great fit here, since the RNG is never a = boot target and you just want it to connect all the time.


I understand the issue. On the= fast boot path only things in the NVRAM variables, or hard coded as BDS po= licy get connected. For example in our custom BDS we have code that connect= s devices related to the T2 and we have custom code that only connects them= at the known location. These devices produce services that other EFI Drive= r Model drivers use when they connect. 

It is a shame my first ap= proach did not work: it attempted to connect the short form USB class devic= e path carrying the VID and PID of this controller as the remaining device = path on each detected USB host controller, similar to how USB keyboards are= connected in the upstream BDS code.


Is there a BZ for that? I=E2=80=99ve gotten feedback o= ver the years that our BDS implementation is hard to understand. Maybe we s= hould invest some time in documentation on how to use it?

Thanks,

Andrew Fish<= /div>
But yes, ConnectController() should also work - it will invoke the S= upported() method again, and call Start(). I did check that doing this dire= ctly amounts to the same in terms of bookkeeping, but I agree that this cou= ld potentially change at some point. However, the question is whether conne= cting the controller to another, higher priority driver is the right thing = to do in this case. It is really hard to answer that question in general te= rms.

Also It would like be a good idea to have= a PCD to turn on/off this feature.

Fair enough.

Thanks,
Andrew Fish
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/Silico= n/Openmoko/ChaosKeyDxe/DriverBinding.c
index e7d0d3fe563e..61= 1803c6c339 100644
--- a/Silicon/Openmoko/ChaosKeyDxe/DriverBi= nding.c
+++ b/Silicon/Openmoko/ChaosKeyDxe/DriverBinding.c@@ -11,6 +11,9 @@

#include "ChaosK= eyDriver.h"

+STATIC VOID    &nb= sp;    *mProtocolNotifyRegistration;
+STA= TIC EFI_EVENT    mProtocolNotifyRegistrationEvent;
+
/**
  Tests to see if this d= river supports a given controller.

@@ -157,6 += 160,55 @@ EFI_DRIVER_BINDING_PROTOCOL  gUsbDriverBinding =3D {
};


+STATIC
+VOID
+EFIAPI
+UsbHwrngOnProtocolNotify (
+  IN EFI_EVENT        &n= bsp;   Event,
+  IN VOID   &nbs= p;            &= nbsp;*Context
+  )
+{
+ &nbs= p;EFI_STATUS           &n= bsp;  Status;
+  EFI_HANDLE    =           *Handles;
+  UINTN         &nbs= p;         HandleCount;
+  UINTN          = ;         Index;
+
+  do {
+    Status =3D = gBS->LocateHandleBuffer (ByRegisterNotify, NULL,
+  &= nbsp;           &nbs= p;     mProtocolNotifyRegistration, &HandleCou= nt, &Handles);
+    if (EFI_ERROR (Status)= ) {
+      if (Status !=3D EFI_NOT_F= OUND) {
+        DEBUG ((D= EBUG_WARN, "%a: LocateHandleBuffer() failed - %r\n",
+  =         __FUNCTION__, Status));
+        }
+ &n= bsp;    return;
+    }
+
+    if (HandleCount =3D=3D 0) {<= br class=3D"">+      return;
+  = ;  }
+
+    for (Index= =3D 0; Index < HandleCount; Index++) {
+   &nbs= p;  Status =3D UsbHwrngDriverBindingSupported (&gUsbDriverBin= ding,
+          = ;       Handles[Index], NULL);
+      if (EFI_ERROR (Status)) {
+        continue;
= +      }
+
+  &nb= sp;   //
+      // At= tempt to connect the USB device path describing the ChaosKey
= +      // hardware via the handle describing a USB= host controller.
+      //
+      Status =3D UsbHwrngDriverBindingStar= t (&gUsbDriverBinding,
+      &n= bsp;          Handles[Ind= ex], NULL);
+      DEBUG ((DEBUG_VER= BOSE, "%a: UsbHwrngDriverBindingStart () returned %r\n",
+ &n= bsp;      __FUNCTION__, Status));
+    }
+    gBS->FreePool= (Handles);
+  } while (1);
+}
+
+
/**
  The entry = point of ChaosKey UEFI Driver.

@@ -185,6 +237,= 18 @@ EntryPoint (
       =       NULL, &gChaosKeyDriverComponentName= 2);
  ASSERT_EFI_ERROR (Status);

+  //
+  // This driver produces the EFI= Random Number Generator protocol on
+  // compatible US= B I/O handles, which is not a protocol that can provide
+ &nb= sp;// a boot target. This means that it will not get connected on an ordina= ry
+  // 'fast' boot (which only connects the console an= d boot entry device paths)
+  // unless we take extra me= asures.
+  //
+  mProtocolNotifyRegis= trationEvent =3D EfiCreateProtocolNotifyEvent (
+   = ;            &n= bsp;            = ;           &gEf= iUsbIoProtocolGuid, TPL_CALLBACK,
+     &= nbsp;           &nbs= p;            &= nbsp;        UsbHwrngOnProtocolNoti= fy, NULL,
+         &= nbsp;           &nbs= p;            &= nbsp;    &mProtocolNotifyRegistration);
+
  DEBUG ((DEBUG_INIT | DEBUG_INFO, "*** Instal= led ChaosKey driver! ***\n"));

  ret= urn EFI_SUCCESS;
@@ -211,6 +275,8 @@ UnloadImage (
  UINTN       HandleCount;  UINTN       Index;
+  gBS->CloseEvent (mProtocolNotifyRegi= strationEvent);
+
  //
=   // Retrieve all USB I/O handles in the handle database
  //
-- 
2.20.1






--Apple-Mail=_A1ACC8E7-D5DA-4D99-907F-51F2239F8B8C--