From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f65.google.com (mail-wm1-f65.google.com [209.85.128.65]) by mx.groups.io with SMTP id smtpd.web12.346.1591134001605645333 for ; Tue, 02 Jun 2020 14:40:02 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@nuviainc-com.20150623.gappssmtp.com header.s=20150623 header.b=HH0oeuDi; spf=pass (domain: nuviainc.com, ip: 209.85.128.65, mailfrom: leif@nuviainc.com) Received: by mail-wm1-f65.google.com with SMTP id r15so4602735wmh.5 for ; Tue, 02 Jun 2020 14:40:01 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nuviainc-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=+FyM8elsNsL9F12PsAbRC49W0eBfZ39CUa/N/XwVvA8=; b=HH0oeuDiugyhvd1u6cqrZyvtbfw9MoeKZ8AKG8GpTothS83L6mtzcLdbybLuIrhPe7 z5Do7XZLGgHUF9ZuO7+x5biq/Ps7uZQo8bfMgdJzMakiFdLrmBCzInwL0mrDUELhpBY4 DlBQesRRvp5VfYnLdtvkIeppcNhjhUdOhvmQzrmeQ47rvE3sOeTpkIDdNrzY7j9pIek2 ANft5Z8kc6bpw4HgOm0++qv6E4NZEVrPqLMtbVsJmJ8cnPJq5vHWsUfTXmuwDkWRKmV2 8Mor26+eZQMUyCw/kEzZDvfnkp5BZ38FgufQotfnQ25w3kk8cpQO6Fs+gYKJ+RnMaE+M TEVg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=+FyM8elsNsL9F12PsAbRC49W0eBfZ39CUa/N/XwVvA8=; b=abVLlBeslW7yBH1NI6Oq6WFLLPfgsr5tDZbrPfEEoW4bFAxCPMiabE+/PyajyXDb7+ 7cyjmPmbaI8kIE4pIR3saUYqQfQn343mWc87LdnbBOb2ZvfmPD2Biou3EM1k5dQY+efr th26OqNhpu74XNwQcrlprYilmwBL0NmeHM9M23PQx++BXVbm5nphXG7/KidcwSVK14dx iO5QYfzmtv2W8bCXvrp/vHoPSxyFrwcqBRevaNI4ftg1Ad+iPVl9iM4QS+SsCa/RUAlh Ml7Y/YLCWO2owIgG19UtGgjg0wuOB/QQ70099jzYQhyifWVdT08orXbuutpYaY7sc0rc 6+Ng== X-Gm-Message-State: AOAM5337hlfNNlQQfyM/xWg31+JtVHBuUFImtrv08v/fMo0p6VdJdq/Y Aaf/eLcv2z3bIcPq7QQY9VI5cg== X-Google-Smtp-Source: ABdhPJy4CjWJtFzJrRx4hmuo3q2vPA/2jM5eTNyVh/cXCYbQJIN3C16pFyYRKKkVEYe8riGR5xQHUQ== X-Received: by 2002:a1c:9ec4:: with SMTP id h187mr1006255wme.27.1591133999810; Tue, 02 Jun 2020 14:39:59 -0700 (PDT) Return-Path: Received: from vanye ([2001:470:1f09:12f0:b26e:bfff:fea9:f1b8]) by smtp.gmail.com with ESMTPSA id 40sm424489wrc.15.2020.06.02.14.39.58 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 02 Jun 2020 14:39:59 -0700 (PDT) Date: Tue, 2 Jun 2020 22:39:57 +0100 From: "Leif Lindholm" To: Ard Biesheuvel Cc: Ard Biesheuvel , devel@edk2.groups.io Subject: Re: [PATCH edk2-platforms v2] Silicon/ChaosKeyDxe: don't rely on connect all controllers Message-ID: <20200602213957.GP28566@vanye> References: <20200602133849.5422-1-ard.biesheuvel@linaro.org> <20200602144921.GN28566@vanye> MIME-Version: 1.0 In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Tue, Jun 02, 2020 at 16:58:54 +0200, Ard Biesheuvel wrote: > > > 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 > > > > > > 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 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. Right. OK, I agree that's fair enough. It is a shame though - the support for --reconnect was only added to efibootmgr on 11 October last year, so isn't currently available in most distros. > 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) Agreed. > 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. Yeah. OK, taken all of these points into account: Reviewed-by: Leif Lindholm / Leif > > > 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 > > >