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.web11.17526.1591200273831746428 for ; Wed, 03 Jun 2020 09:04:34 -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 659DF55D; Wed, 3 Jun 2020 09:04:32 -0700 (PDT) Received: from [192.168.1.69] (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 664723F52E; Wed, 3 Jun 2020 09:04:30 -0700 (PDT) Subject: Re: [edk2-devel] [PATCH edk2-platforms v2] Silicon/ChaosKeyDxe: don't rely on connect all controllers To: Andrew Fish , devel@edk2.groups.io Cc: Ard Biesheuvel , Mike Kinney , leif@nuviainc.com References: <20200602133849.5422-1-ard.biesheuvel@linaro.org> <77023D58-E75D-48E8-B5CE-A51FA08FE9C7@apple.com> From: "Ard Biesheuvel" Message-ID: <663d0397-ab3e-9b16-e3ba-5b1747d6cb8f@arm.com> Date: Wed, 3 Jun 2020 18:04:28 +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: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: quoted-printable On 6/3/20 5:47 PM, Andrew Fish wrote: >=20 >=20 >> On Jun 2, 2020, at 11:32 PM, Ard Biesheuvel > > wrote: >> >> On 6/3/20 12:28 AM, Andrew Fish wrote: >>>> On Jun 2, 2020, at 6:38 AM, Ard Biesheuvel=20 >>>> > 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 rea= lly >>>> not up to a third party driver to meddle with this, and so relying o= n >>>> the USB host controllers to have been connected non-reursively is th= e >>>> 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=20 >>>> 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=20 >>>> instantiated >>>> by the DXE core. This is what typically happens, given that USB=20 >>>> keyboards >>>> are also exposed via USB I/O protocol instances. The only difference= =20 >>>> with >>>> a non-fast [ConnectAll()] boot is that those USB I/O protocol instan= ces >>>> are not connected further automatically, but it is reasonable to exp= ect >>>> that the handles themselves have been instantiated. >>>> >>>> Platforms that do not produce those USB I/O handles would not be abl= e to >>>> offer the ability to interrupt the boot or enter the menu using a US= B >>>> keyboard, so this is rather rare in practice. Also, if such platform= s do >>>> exist, it is not up to this 3rd party driver to override this policy= and >>>> enumerate the entire USB hierarchy. >>>> >>>> So this means we need to call UsbHwrngDriverBindingSupported() direc= tly >>>> 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 ConnectControlle= r(), >>>> so just call UsbHwrngDriverBindingStart() directly to connect the dr= iver >>>> to the controller. >>>> >>> Actually there is a point in using gBS->ConnectController(). >>> 1) You make it impossible for the platform to override the drivers=20 >>> choice. The philosophy of EFI has alway been that the drivers should=20 >>> not cary policy and the platforms should cary policy. This inverts th= at. >>> 2) I=E2=80=99m not sure it is well defined behavior by UEFI to NOT ca= ll=20 >>> gBS->ConnectController(). I took a quick look at the DXE Core and the= =20 >>> gBS->ConnectController() is not doing any book keeping, other than=20 >>> managing the precedence rules, but it seems like it would be legal to= =20 >>> add something in the future. + Mike in case I=E2=80=99m off on this o= ne. >> >> The idea is really that by loading this driver, you are indicating=20 >> that you want it to connect to any supporting controller and produce=20 >> the RNG protocol, despite the platform policy. The UEFI driver model=20 >> simply isn't a great fit here, since the RNG is never a boot target=20 >> 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=20 > variables, or hard coded as BDS policy get connected. For example in ou= r=20 > custom BDS we have code that connects devices related to the T2 and we=20 > have custom code that only connects them at the known location. These=20 > devices produce services that other EFI Driver Model drivers use when=20 > they connect. >=20 Sure. If the controller is known to live at a particular location on a=20 particular platform, you can just connect it. The idea of this driver is=20 that you can install it on any platform, plug in the USB stick and off=20 you go, which is substantially more complicated. >> 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. >> >=20 > Is there a BZ for that? I=E2=80=99ve gotten feedback over the years tha= t our BDS=20 > implementation is hard to understand. Maybe we should invest some time=20 > in documentation on how to use it? >=20 I'm not sure that would help. I was hoping we could rely on spec'ed=20 behavior here, rather than how some BDSes might behave. Note that the AMI BDS behaved differently here than the EDK2 one.