From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f67.google.com (mail-wr1-f67.google.com [209.85.221.67]) by mx.groups.io with SMTP id smtpd.web10.12425.1591109365479766690 for ; Tue, 02 Jun 2020 07:49:25 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@nuviainc-com.20150623.gappssmtp.com header.s=20150623 header.b=GqWxwwD9; spf=pass (domain: nuviainc.com, ip: 209.85.221.67, mailfrom: leif@nuviainc.com) Received: by mail-wr1-f67.google.com with SMTP id l10so3676897wrr.10 for ; Tue, 02 Jun 2020 07:49:25 -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=MsH8NpGroYF7sD3oaYQh5cBwQRnJAjf2LhYWLHqWevs=; b=GqWxwwD9ndD4SJ0Gphuk2eHg3cDdiAReV7Zfbq9t/UGEjmVKIFQGpS+ACAwGaVNOQG WWGKPHDa0T0F43Peq21dX5PGtIZp+TvWExU/U9LLbfpy4+MpBVTeIu1bkqenmht9kQrB 3ipbaKByIA9FPMueEiYA8055SlPDUVPpdZGtsXa3gmJpM3YV6EsaZvHetwmynj1IGr6L Q+rWxD7Oyia5j8M+2AeI2JWNqGzvT5ycntJ9TELz22T5glCVSBGYoUj9OcvvtBKhkO19 B7mqBcOlnc1Id0nBubX+96JzmsA5lrmBuxGLX8cbdz0WbKylaml7FYAlSV4rtD/fFaP8 czXQ== 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=MsH8NpGroYF7sD3oaYQh5cBwQRnJAjf2LhYWLHqWevs=; b=nUGlZTzXhDPSJoZ707oZ59TLsTFJ4KEg7ewA6v54ix6OQiVcheSkRuBe+aaLcAHyx2 3GEICMehlL0VDD1nEQGMYm+BBxM+pZOSQU39kV0mztdEDZ5ly5F39f8OLHLxPNsfQzSx IrzCs8V5nW33UryapWwK8I1R3LJluUXnRnSPKT2eFkLpVNVgnl0ZbYwOh7E6ZaIqz6q3 f3rZOQYW5QE4scNd5+dM7Jji3E3q17flUXCNSvQkAMyagDBbLktCL48/ENyNOIppsgce w+dC7YMHHLz6vl7FKU4FaWLWXbFvTvyHax878aoi9ynZdb2BEBbgkmhtZKnHOSSf6CSS rJOA== X-Gm-Message-State: AOAM531C+ANLa7+mF8jU1l4oN4ywrVc9C14xVaS4qyIeHPjTZw20c3PD KWghFEobZ/65Cf7geGFdbTv6Iw== X-Google-Smtp-Source: ABdhPJyBWu80qMCsxu5TEANd1hL4ct9LvQwT9iDZqpTcnScTdz+59toAfVvdTpy+QJ6ALZXuSpIY5g== X-Received: by 2002:a5d:5084:: with SMTP id a4mr8846606wrt.416.1591109363899; Tue, 02 Jun 2020 07:49:23 -0700 (PDT) Return-Path: Received: from vanye ([2001:470:1f09:12f0:b26e:bfff:fea9:f1b8]) by smtp.gmail.com with ESMTPSA id l17sm42401wmi.3.2020.06.02.07.49.23 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 02 Jun 2020 07:49:23 -0700 (PDT) Date: Tue, 2 Jun 2020 15:49:21 +0100 From: "Leif Lindholm" To: Ard Biesheuvel Cc: devel@edk2.groups.io, Ard Biesheuvel Subject: Re: [PATCH edk2-platforms v2] Silicon/ChaosKeyDxe: don't rely on connect all controllers Message-ID: <20200602144921.GN28566@vanye> References: <20200602133849.5422-1-ard.biesheuvel@linaro.org> MIME-Version: 1.0 In-Reply-To: <20200602133849.5422-1-ard.biesheuvel@linaro.org> 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 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? / 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 >