public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Leif Lindholm" <leif@nuviainc.com>
To: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: devel@edk2.groups.io, Ard Biesheuvel <ard.biesheuvel@arm.com>
Subject: Re: [PATCH edk2-platforms v2] Silicon/ChaosKeyDxe: don't rely on connect all controllers
Date: Tue, 2 Jun 2020 15:49:21 +0100	[thread overview]
Message-ID: <20200602144921.GN28566@vanye> (raw)
In-Reply-To: <20200602133849.5422-1-ard.biesheuvel@linaro.org>

On Tue, Jun 02, 2020 at 15:38:49 +0200, Ard Biesheuvel wrote:
> From: Ard Biesheuvel <ard.biesheuvel@arm.com>
> 
> 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 <ard.biesheuvel@arm.com>
> ---
> 
> 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
> 

  reply	other threads:[~2020-06-02 14:49 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-02 13:38 [PATCH edk2-platforms v2] Silicon/ChaosKeyDxe: don't rely on connect all controllers Ard Biesheuvel
2020-06-02 14:49 ` Leif Lindholm [this message]
2020-06-02 14:58   ` Ard Biesheuvel
2020-06-02 21:39     ` Leif Lindholm
2020-06-02 22:28 ` [edk2-devel] " Andrew Fish
2020-06-03  6:32   ` Ard Biesheuvel
2020-06-03 15:47     ` Andrew Fish
2020-06-03 16:04       ` Ard Biesheuvel

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200602144921.GN28566@vanye \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox