Re: [PATCH edk2-platforms] Silicon/Openmoko: add driver for ChaosKey RNG USB device

[Leif Lindholm <leif.lindholm@linaro.org>]

On Thu, Aug 24, 2017 at 10:44:46AM +0100, Ard Biesheuvel wrote:
> >>  Silicon/Openmoko/ChaosKeyDxe/ChaosKeyDriver.c | 346 ++++++++++++++++++++
> >>  Silicon/Openmoko/ChaosKeyDxe/ChaosKeyDriver.h |  61 ++++
> >>  Silicon/Openmoko/ChaosKeyDxe/ChaosKeyDxe.inf  |  48 +++
> >>  Silicon/Openmoko/ChaosKeyDxe/ComponentName.c  | 205 ++++++++++++
> >>  Silicon/Openmoko/ChaosKeyDxe/DriverBinding.c  | 256 +++++++++++++++
> >>  5 files changed, 916 insertions(+)
> >
> > This is all a bit new territory, so I don't know what the best way of
> > dealing with this is, but I think we could do with a .dsc somewhere to
> > enable building this driver standalone. My gut feel is that an
> > Openmoko/Openmoko.dsc would be a reasonable candidate.
> 
> Yeah, I can add one. Also useful for building this driver as an EBC binary.

Yes, indeed. Thanks.

> >> diff --git a/Silicon/Openmoko/ChaosKeyDxe/ChaosKeyDriver.c b/Silicon/Openmoko/ChaosKeyDxe/ChaosKeyDriver.c
> >> new file mode 100644
> >> index 000000000000..1870080d2c70
> >> --- /dev/null
> >> +++ b/Silicon/Openmoko/ChaosKeyDxe/ChaosKeyDriver.c
> >> @@ -0,0 +1,346 @@
> >> +/** @file
> >> +  Device driver for the ChaosKey hardware random number generator.
> >> +
> >> +  Copyright (c) 2016 - 2017, Linaro Ltd. All rights reserved.<BR>
> >> +
> >> +  This program and the accompanying materials
> >> +  are licensed and made available under the terms and conditions of the BSD
> >> +  License which accompanies this distribution. The full text of the license may
> >> +  be found at  http://opensource.org/licenses/bsd-license.php.
> >> +
> >> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> >> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> >> +
> >> +**/
> >> +
> >> +#include "ChaosKeyDriver.h"
> >> +
> >> +#include <Library/BaseMemoryLib.h>
> >> +#include <Library/DebugLib.h>
> >> +#include <Library/MemoryAllocationLib.h>
> >> +
> >> +STATIC
> >> +BOOLEAN
> >> +IsBulkInEndpoint (
> >> +  IN  EFI_USB_ENDPOINT_DESCRIPTOR     *Endpoint
> >> +  )
> >> +{
> >> +  if ((Endpoint->Attributes & USB_ENDPOINT_TYPE_MASK) == USB_ENDPOINT_BULK) {
> >> +    if (Endpoint->EndpointAddress & USB_ENDPOINT_DIR_IN) {
> >> +      return TRUE;
> >> +    }
> >> +  }
> >> +  return FALSE;
> >> +}
> >> +
> >> +
> >> +STATIC
> >> +EFI_STATUS
> >> +FindEndpoint (
> >> +  IN  CHAOSKEY_DEV *ChaosKey
> >> +  )
> >> +{
> >> +  EFI_USB_IO_PROTOCOL             *UsbIo;
> >> +  EFI_STATUS                      Status;
> >> +  UINTN                           Index;
> >> +  EFI_USB_INTERFACE_DESCRIPTOR    InterfaceDescriptor;
> >> +
> >> +  UsbIo = ChaosKey->UsbIo;
> >> +
> >> +  //
> >> +  // Get interface & endpoint descriptor
> >> +  //
> >> +  Status = UsbIo->UsbGetInterfaceDescriptor (UsbIo, &InterfaceDescriptor);
> >> +  if (EFI_ERROR (Status)) {
> >> +    return Status;
> >> +  }
> >> +
> >> +  //
> >> +  // The ChaosKey provides two endpoints:
> >> +  // - The first one is the 'cooked' one, to be used as random data input
> >> +  // - The second one is the raw bitstream from the generator, higher
> >> +  //   throughput, but lower randomness.
> >> +  // So locate the first bulk IN endpoint and save it for later use.
> >> +  //
> >> +  for (Index = 0; Index < InterfaceDescriptor.NumEndpoints; Index++) {
> >> +    EFI_USB_ENDPOINT_DESCRIPTOR  Endpoint;
> >> +
> >> +    Status = UsbIo->UsbGetEndpointDescriptor (UsbIo, Index, &Endpoint);
> >> +    if (EFI_ERROR (Status)) {
> >> +      DEBUG ((DEBUG_ERROR, "UsbGetEndPointDescriptor(%d) failed!\n", Index));
> >> +      return Status;
> >> +    }
> >> +
> >> +    if (IsBulkInEndpoint(&Endpoint)) {
> >> +      ChaosKey->EndpointAddress = Endpoint.EndpointAddress;
> >> +      ChaosKey->EndpointSize = Endpoint.MaxPacketSize;
> >> +      return EFI_SUCCESS;
> >> +    }
> >> +  }
> >> +
> >> +  DEBUG ((DEBUG_ERROR, "Failed to locate suitable BULK IN USB endpoint!\n"));
> >> +  return EFI_DEVICE_ERROR;
> >> +}
> >> +
> >> +
> >> +/**
> >> +  Returns information about the random number generation implementation.
> >> +
> >> +  @param[in]     This               A pointer to the EFI_RNG_PROTOCOL instance.
> >> +  @param[in,out] AlgorithmListSize  On input, the size in bytes of AlgorithmList
> >> +                                    On output with a return code of EFI_SUCCESS,
> >> +                                    the size in bytes of the data returned in
> >> +                                    AlgorithmList. On output with a return
> >> +                                    code of EFI_BUFFER_TOO_SMALL, the size of
> >> +                                    AlgorithmList required to obtain the list.
> >> +  @param[out] AlgorithmList         A caller-allocated memory buffer filled by
> >> +                                    the driver with one EFI_RNG_ALGORITHM
> >> +                                    element for each supported RNG algorithm.
> >> +                                    The list must not change across multiple
> >> +                                    calls to the same driver. The first
> >> +                                    algorithm in the list is the default
> >> +                                    algorithm for the driver.
> >> +
> >> +  @retval EFI_SUCCESS               The RNG algorithm list was returned
> >> +                                    successfully.
> >> +  @retval EFI_UNSUPPORTED           The services is not supported by this driver
> >> +  @retval EFI_DEVICE_ERROR          The list of algorithms could not be
> >> +                                    retrieved due to a hardware or firmware
> >> +                                    error.
> >> +  @retval EFI_INVALID_PARAMETER     One or more of the parameters are incorrect.
> >> +  @retval EFI_BUFFER_TOO_SMALL      The buffer RNGAlgorithmList is too small to
> >> +                                    hold the result.
> >> +
> >> +**/
> >> +STATIC
> >> +EFI_STATUS
> >> +EFIAPI
> >> +GetInfo (
> >> +  IN      EFI_RNG_PROTOCOL    *This,
> >> +  IN  OUT UINTN               *AlgorithmListSize,
> >> +  OUT     EFI_RNG_ALGORITHM   *AlgorithmList
> >> +)
> >> +{
> >> +  UINTN Size;
> >> +
> >> +  //
> >> +  // We only implement the raw algorithm
> >> +  //
> >> +  Size = sizeof gEfiRngAlgorithmRaw;
> >> +
> >> +  if (*AlgorithmListSize < Size) {
> >> +    *AlgorithmListSize = Size;
> >> +    return EFI_BUFFER_TOO_SMALL;
> >> +  }
> >> +
> >> +  gBS->CopyMem (AlgorithmList, &gEfiRngAlgorithmRaw, Size);
> >> +  *AlgorithmListSize = Size;
> >> +
> >> +  return EFI_SUCCESS;
> >> +}
> >> +
> >> +
> >> +/**
> >> +  Produces and returns an RNG value using either the default or specified RNG
> >> +  algorithm.
> >> +
> >> +  @param[in]  This                A pointer to the EFI_RNG_PROTOCOL instance.
> >> +  @param[in]  Algorithm           A pointer to the EFI_RNG_ALGORITHM that
> >> +                                  identifies the RNG algorithm to use. May be
> >> +                                  NULL in which case the function will use its
> >> +                                  default RNG algorithm.
> >> +  @param[in]  ValueLength         The length in bytes of the memory buffer
> >> +                                  pointed to by RNGValue. The driver shall
> >> +                                  return exactly this numbers of bytes.
> >> +  @param[out] Value               A caller-allocated memory buffer filled by the
> >> +                                  driver with the resulting RNG value.
> >> +
> >> +  @retval EFI_SUCCESS             The RNG value was returned successfully.
> >> +  @retval EFI_UNSUPPORTED         The algorithm specified by RNGAlgorithm is not
> >> +                                  supported by this driver.
> >> +  @retval EFI_DEVICE_ERROR        An RNG value could not be retrieved due to a
> >> +                                  hardware or firmware error.
> >> +  @retval EFI_NOT_READY           There is not enough random data available to
> >> +                                  satisfy the length requested by
> >> +                                  RNGValueLength.
> >> +  @retval EFI_INVALID_PARAMETER   RNGValue is NULL or RNGValueLength is zero.
> >> +
> >> +**/
> >> +STATIC
> >> +EFI_STATUS
> >> +EFIAPI
> >> +GetRNG (
> >> +  IN EFI_RNG_PROTOCOL   *This,
> >> +  IN EFI_RNG_ALGORITHM  *Algorithm OPTIONAL,
> >> +  IN UINTN              ValueLength,
> >> +  OUT UINT8             *Value
> >> +)
> >> +{
> >> +  EFI_STATUS        Status;
> >> +  CHAOSKEY_DEV      *ChaosKey;
> >> +  EFI_TPL           Tpl;
> >> +  UINT8             Buffer[CHAOSKEY_MAX_EP_SIZE];
> >> +  UINT8             *OutPointer;
> >> +  UINTN             OutSize;
> >> +  UINT32            Result;
> >> +
> >> +  if (Algorithm != NULL && !CompareGuid (Algorithm, &gEfiRngAlgorithmRaw)) {
> >> +    return EFI_UNSUPPORTED;
> >> +  }
> >> +
> >> +  ChaosKey = CHAOSKEY_DEV_FROM_THIS (This);
> >> +
> >> +  while (ValueLength > 0) {
> >> +    //
> >> +    // If more data is requested than the endpoint can deliver in a single
> >> +    // transfer, put it straight into the caller's buffer.
> >> +    //
> >> +    if (ValueLength >= ChaosKey->EndpointSize) {
> >> +      OutPointer = Value;
> >> +    } else {
> >> +      OutPointer = Buffer;
> >> +    }
> >> +    OutSize = ChaosKey->EndpointSize;
> >> +
> >> +    Tpl = gBS->RaiseTPL (TPL_NOTIFY);
> >
> > You mentioned to me on IRC that this should not be necessary, and that
> > UsbIo should adjust tpl as necessary.
> >
> > It is quite likely I misinterpreted behaviour I encountered when using
> > USB passthrough in QEMU for testing (and managed to break the host
> > system's driver).
> 
> I will remove it. We are dealing with an abstract protocol here, not
> hardware, and if completing a single bulk transfer does not tolerate
> interruptions, it should be up to the USB I/O protocol implementation
> to deal with that.

Yes, sounds good.

> >> diff --git a/Silicon/Openmoko/ChaosKeyDxe/DriverBinding.c b/Silicon/Openmoko/ChaosKeyDxe/DriverBinding.c
> >> new file mode 100644
> >> index 000000000000..a1a5a796d38d
> >> --- /dev/null
> >> +++ b/Silicon/Openmoko/ChaosKeyDxe/DriverBinding.c
> >> @@ -0,0 +1,256 @@
> >> +/** @file
> >> +  Device driver for the ChaosKey hardware random number generator.
> >> +
> >> +  Copyright (c) 2016 - 2017, Linaro Ltd. All rights reserved.<BR>
> >> +
> >> +  This program and the accompanying materials
> >> +  are licensed and made available under the terms and conditions of the BSD
> >> +  License which accompanies this distribution. The full text of the license may
> >> +  be found at  http://opensource.org/licenses/bsd-license.php.
> >> +
> >> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> >> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> >> +
> >> +**/
> >> +
> >> +#include <Library/UefiDriverEntryPoint.h>
> >> +
> >> +#include "ChaosKeyDriver.h"
> >> +
> >> +/**
> >> +  Tests to see if this driver supports a given controller.
> >> +
> >> +  @param  This[in]                 A pointer to the EFI_DRIVER_BINDING_PROTOCOL
> >> +                                   instance.
> >> +  @param  ControllerHandle[in]     The handle of the controller to test.
> >> +  @param  RemainingDevicePath[in]  The remaining device path.
> >> +                                   (Ignored - this is not a bus driver.)
> >> +
> >> +  @retval EFI_SUCCESS              The driver supports this controller.
> >> +  @retval EFI_ALREADY_STARTED      The device specified by ControllerHandle is
> >> +                                   already being managed by the driver specified
> >> +                                   by This.
> >> +  @retval EFI_UNSUPPORTED          The device specified by ControllerHandle is
> >> +                                   not supported by the driver specified by This.
> >> +
> >> +**/
> >> +EFI_STATUS
> >> +EFIAPI
> >> +UsbHwrngDriverBindingSupported (
> >> +  IN EFI_DRIVER_BINDING_PROTOCOL  *This,
> >> +  IN EFI_HANDLE                   ControllerHandle,
> >> +  IN EFI_DEVICE_PATH_PROTOCOL     *RemainingDevicePath
> >> +  )
> >> +{
> >> +  EFI_USB_DEVICE_DESCRIPTOR  Device;
> >> +  EFI_USB_IO_PROTOCOL        *UsbIo;
> >> +  EFI_STATUS                 Status;
> >> +
> >> +  //
> >> +  //  Connect to the USB stack
> >> +  //
> >> +  Status = gBS->OpenProtocol (ControllerHandle,
> >> +                              &gEfiUsbIoProtocolGuid,
> >> +                              (VOID **) &UsbIo,
> >> +                              This->DriverBindingHandle,
> >> +                              ControllerHandle,
> >> +                              EFI_OPEN_PROTOCOL_BY_DRIVER);
> >> +  if (EFI_ERROR (Status)) {
> >> +    return Status;
> >> +  }
> >> +
> >> +  //
> >> +  //  Get the interface descriptor to check the USB class and find a transport
> >> +  //  protocol handler.
> >> +  //
> >> +  Status = UsbIo->UsbGetDeviceDescriptor (UsbIo, &Device);
> >> +  if (!EFI_ERROR (Status)) {
> >> +    //
> >> +    //  Validate the adapter
> >> +    //
> >> +    if ((Device.IdVendor != CHAOSKEY_VENDOR_ID) ||
> >> +        (Device.IdProduct != CHAOSKEY_PRODUCT_ID)) {
> >> +      Status = EFI_UNSUPPORTED;
> >> +    } else {
> >> +      DEBUG ((DEBUG_INIT | DEBUG_INFO,
> >> +        "ChaosKey (0x%04x:0x%04x) is my homeboy!\n",
> >> +        Device.IdVendor, Device.IdProduct));
> >
> > That was intended as a cute remark in my original blog post. Perhaps a
> > more descriptive message would be better.
> > "Detected ChaosKey RNG device." or such.
> >
> 
> OK, will change that.

Thanks.

> >> +      Status = EFI_SUCCESS;
> >> +    }
> >> +  }
> >> +
> >> +  //
> >> +  // Clean up.
> >> +  //
> >> +  gBS->CloseProtocol (ControllerHandle,
> >> +                      &gEfiUsbIoProtocolGuid,
> >> +                      This->DriverBindingHandle,
> >> +                      ControllerHandle);
> >> +
> >> +  return Status;
> >> +}
> >> +
> >> +
> >> +/**
> >> +  Starts a device controller or a bus controller.
> >> +
> >> +  @param[in]  This                 A pointer to the EFI_DRIVER_BINDING_PROTOCOL
> >> +                                   instance.
> >> +  @param[in]  ControllerHandle     The handle of the device to start. This
> >> +                                   handle must support a protocol interface that
> >> +                                   supplies an I/O abstraction to the driver.
> >> +  @param[in]  RemainingDevicePath  The remaining portion of the device path.
> >> +                                   (Ignored - this is not a bus driver.)
> >> +
> >> +  @retval EFI_SUCCESS              The device was started.
> >> +  @retval EFI_DEVICE_ERROR         The device could not be started due to a
> >> +                                   device error.
> >> +  @retval EFI_OUT_OF_RESOURCES     The request could not be completed due to a
> >> +                                   lack of resources.
> >> +
> >> +**/
> >> +EFI_STATUS
> >> +EFIAPI
> >> +UsbHwrngDriverBindingStart (
> >> +  IN EFI_DRIVER_BINDING_PROTOCOL  *This,
> >> +  IN EFI_HANDLE                   ControllerHandle,
> >> +  IN EFI_DEVICE_PATH_PROTOCOL     *RemainingDevicePath OPTIONAL
> >> +  )
> >> +{
> >> +  return ChaosKeyInit (This->DriverBindingHandle, ControllerHandle);
> >> +}
> >> +
> >> +
> >> +/**
> >> +  Stops a device controller or a bus controller.
> >> +
> >> +  @param[in]  This              A pointer to the EFI_DRIVER_BINDING_PROTOCOL
> >> +                                instance.
> >> +  @param[in]  ControllerHandle  A handle to the device being stopped. The handle
> >> +                                must support a bus specific I/O protocol for the
> >> +                                driver to use to stop the device.
> >> +  @param[in]  NumberOfChildren  The number of child device handles in
> >> +                                ChildHandleBuffer.
> >> +  @param[in]  ChildHandleBuffer An array of child handles to be freed. May be
> >> +                                NULL if NumberOfChildren is 0.
> >> +
> >> +  @retval EFI_SUCCESS           The device was stopped.
> >> +  @retval EFI_DEVICE_ERROR      The device could not be stopped due to a device
> >> +                                error.
> >> +
> >> +**/
> >> +EFI_STATUS
> >> +EFIAPI
> >> +UsbHwrngDriverBindingStop (
> >> +  IN  EFI_DRIVER_BINDING_PROTOCOL  *This,
> >> +  IN  EFI_HANDLE                  ControllerHandle,
> >> +  IN  UINTN                       NumberOfChildren,
> >> +  IN  EFI_HANDLE                  *ChildHandleBuffer OPTIONAL
> >> +  )
> >> +{
> >> +  return ChaosKeyRelease (This->DriverBindingHandle, ControllerHandle);
> >> +}
> >> +
> >> +
> >> +STATIC
> >> +EFI_DRIVER_BINDING_PROTOCOL  gUsbDriverBinding = {
> >> +  UsbHwrngDriverBindingSupported,
> >> +  UsbHwrngDriverBindingStart,
> >> +  UsbHwrngDriverBindingStop,
> >> +  0xa,
> >> +  NULL,
> >> +  NULL
> >> +};
> >> +
> >> +
> >> +/**
> >> +  The entry point of ChaosKey UEFI Driver.
> >> +
> >> +  @param  ImageHandle                The image handle of the UEFI Driver.
> >> +  @param  SystemTable                A pointer to the EFI System Table.
> >> +
> >> +  @retval  EFI_SUCCESS               The Driver or UEFI Driver exited normally.
> >> +  @retval  EFI_INCOMPATIBLE_VERSION  _gUefiDriverRevision is greater than
> >> +                                     SystemTable->Hdr.Revision.
> >> +
> >> +**/
> >> +EFI_STATUS
> >> +EFIAPI
> >> +EntryPoint (
> >> +  IN  EFI_HANDLE          ImageHandle,
> >> +  IN  EFI_SYSTEM_TABLE    *SystemTable
> >> +  )
> >> +{
> >> +  EFI_STATUS    Status;
> >> +
> >> +  //
> >> +  //  Add the driver to the list of drivers
> >> +  //
> >> +  Status = EfiLibInstallDriverBindingComponentName2 (
> >> +             ImageHandle, SystemTable, &gUsbDriverBinding, ImageHandle,
> >> +             &gChaosKeyDriverComponentName, &gChaosKeyDriverComponentName2);
> >
> > Random question ... do we _want_ to provide
> > EFI_COMPONENT_NAME_PROTOCOL, in addition to
> > EFI_COMPONENT_NAME2_PROTOCOL, for new drivers?
> >
> 
> If this is supposed to be a UEFI Driver Model driver, I'd suggest we
> keep it for compatibility with older firmwares.

I am not strongly opposed to it, but I think the following statement
from UEFI 2.7 suggests that it may be worth starting to consider
leaving it out of new drivers:

"To provide backwards compatibility with preexisting EFI 1.10 drivers,
a UEFI platforms may support deprecated protocols which represent
languages in the ISO 639-2 format. This includes the following
protocols:
UNICODE_COLLATION_INTERFACE,
EFI_DRIVER_CONFIGURATION_PROTOCOL,
EFI_DRIVER_DIAGNOSTICS_PROTOCOL,
and EFI_COMPONENT_NAME_PROTOCOL."

The protocol is mentioned in only one other location in the
specification.

> > I should also point out that I don't actually see a DRIVER NAME when
> > executing the "drivers" command from UEFI Shell.
> 
> That worked fine for me, both for 'drivers' and 'devices'. Which
> platform? Does it display other driver names correctly?

*cough*

No.

Platform/ARM/VExpressPkg/ArmVExpress.dsc.inc:
gEfiMdePkgTokenSpaceGuid.PcdComponentNameDisable|TRUE
Platform/ARM/VExpressPkg/ArmVExpress.dsc.inc:
gEfiMdePkgTokenSpaceGuid.PcdComponentName2Disable|TRUE

And this has been cargo-culted across to other platforms.

Or, well, the FAT driver does show up (how!?).
Ah, FatBinPkg.

Ok, the world makes sense again, I will delete these illogical
overrides on all ARM platforms in edk2-platforms.
And probably from BeagleBoardPkh as well - I guess this is where this
originated, together with Ebl. In an effort to reduce image size.
Since BeagleBoardPkg now includes UefiShell, I don't think we need to
worry about that as much.

It is time for me to start deduplicating the common bits of platform
configuration, originally prototyped by Ryan a couple of years back,
in order to avoid this kind of spread in future.

/
    Leif