[RFC PATCH 1/2] efi: import USB I/O related declarations from the UEFI spec

[RFC PATCH 1/2] efi: import USB I/O related declarations from the UEFI spec

[Ard Biesheuvel]

In preparation of adding support for the Chaoskey USB stick to the
UEFI stub, import the USB I/O protocol declarations and related types
to linux/efi.h.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 include/linux/efi.h | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 66 insertions(+)

diff --git a/include/linux/efi.h b/include/linux/efi.h
index 12e05118657c..253749cd9b62 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -22,6 +22,7 @@
 #include <linux/pstore.h>
 #include <linux/range.h>
 #include <linux/reboot.h>
+#include <linux/usb/ch9.h>
 #include <linux/uuid.h>
 #include <linux/screen_info.h>
 
@@ -622,6 +623,7 @@ void efi_native_runtime_setup(void);
 #define EFI_MEMORY_ATTRIBUTES_TABLE_GUID	EFI_GUID(0xdcfa911d, 0x26eb, 0x469f,  0xa2, 0x20, 0x38, 0xb7, 0xdc, 0x46, 0x12, 0x20)
 #define EFI_CONSOLE_OUT_DEVICE_GUID		EFI_GUID(0xd3b36f2c, 0xd551, 0x11d4,  0x9a, 0x46, 0x00, 0x90, 0x27, 0x3f, 0xc1, 0x4d)
 #define APPLE_PROPERTIES_PROTOCOL_GUID		EFI_GUID(0x91bd12fe, 0xf6c3, 0x44fb,  0xa5, 0xb7, 0x51, 0x22, 0xab, 0x30, 0x3a, 0xe0)
+#define EFI_USB_IO_PROTOCOL_GUID		EFI_GUID(0x2b2f68d6, 0x0cd2, 0x44cf,  0x8e, 0x8b, 0xbb, 0xa2, 0x0b, 0x1b, 0x5b, 0x75)
 
 #define EFI_IMAGE_SECURITY_DATABASE_GUID	EFI_GUID(0xd719b2cb, 0x3d3a, 0x4596,  0xa3, 0xbc, 0xda, 0xd0, 0x0e, 0x67, 0x65, 0x6f)
 #define EFI_SHIM_LOCK_GUID			EFI_GUID(0x605dab50, 0xe046, 0x4300,  0xab, 0xb6, 0x3d, 0xd8, 0x10, 0xdd, 0x8b, 0x23)
@@ -1569,4 +1571,68 @@ struct linux_efi_random_seed {
 	u8	bits[];
 };
 
+typedef enum {
+	EfiUsbDataIn,
+	EfiUsbDataOut,
+	EfiUsbNoData
+} efi_usb_data_direction_t;
+
+#define EFI_USB_NOERROR			0x0000
+#define EFI_USB_ERR_NOTEXECUTE		0x0001
+#define EFI_USB_ERR_STALL		0x0002
+#define EFI_USB_ERR_BUFFER		0x0004
+#define EFI_USB_ERR_BABBLE		0x0008
+#define EFI_USB_ERR_NAK			0x0010
+#define EFI_USB_ERR_CRC			0x0020
+#define EFI_USB_ERR_TIMEOUT		0x0040
+#define EFI_USB_ERR_BITSTUFF		0x0080
+#define EFI_USB_ERR_SYSTEM		0x0100
+
+typedef struct {
+	u8	request_type;
+	u8	request;
+	u16	value;
+	u16	index;
+	u16	length;
+} efi_usb_device_request_t;
+
+typedef efi_status_t (*efi_async_usb_transfer_callback_t)(void *, unsigned long,
+							  void *, u32);
+
+typedef struct efi_usb_io_protocol {
+	efi_status_t (*control_transfer)(struct efi_usb_io_protocol *,
+					 efi_usb_device_request_t *,
+					 efi_usb_data_direction_t,
+					 u32, void *, unsigned long, u32 *);
+	efi_status_t (*bulk_transfer)(struct efi_usb_io_protocol *, u8, void *,
+				      unsigned long *, unsigned long, u32 *);
+	efi_status_t (*async_int_transfer)(struct efi_usb_io_protocol *, u8,
+					  efi_bool_t, unsigned long,
+					  unsigned long,
+					  efi_async_usb_transfer_callback_t,
+					  void *);
+	efi_status_t (*sync_int_transfer)(struct efi_usb_io_protocol *, u8,
+					  void *, unsigned long *,
+					  unsigned long, u32 *);
+	efi_status_t (*isoc_transfer)(struct efi_usb_io_protocol *, u8, void *,
+				      unsigned long, u32 *);
+	efi_status_t (*async_isoc_transfer)(struct efi_usb_io_protocol *, u8 *,
+					    void *, unsigned long,
+					    efi_async_usb_transfer_callback_t,
+					    void *);
+	efi_status_t (*get_device_descriptor)(struct efi_usb_io_protocol *,
+					      struct usb_device_descriptor *);
+	efi_status_t (*get_config_descriptor)(struct efi_usb_io_protocol *,
+					      struct usb_config_descriptor *);
+	efi_status_t (*get_iface_descriptor)(struct efi_usb_io_protocol *,
+					     struct usb_interface_descriptor *);
+	efi_status_t (*get_ep_descriptor)(struct efi_usb_io_protocol *, u8,
+					  struct usb_endpoint_descriptor *);
+	efi_status_t (*get_string_descriptor)(struct efi_usb_io_protocol *,
+					       u16, u8, efi_char16_t **);
+	efi_status_t (*get_supported_languages)(struct efi_usb_io_protocol *,
+						u16 **, u16 *);
+	efi_status_t (*port_reset)(struct efi_usb_io_protocol *);
+} efi_usb_io_protocol_t;
+
 #endif /* _LINUX_EFI_H */
-- 
2.11.0

[RFC PATCH 2/2] efi: libstub: add support for the Chaoskey RNG USB stick to the stub

[Ard Biesheuvel]

Early entropy is hard to come by, especially on non-x86 systems that
lack an architected instruction and are not as uniform as PCs.
Fortunately, on UEFI systems, we can invoke the EFI_RNG_PROTOCOL,
which exposes the platform specific entropy source in a generic way.
We use this protocol to fill the RNG UEFI config table (which is used
during early boot to seed the kernel's entropy pool), and on ARM and
arm64, we invoke it to seed KASLR as well.

Sadly, EFI_RNG_PROTOCOL is not widely implemented, and so it would be
nice to have a fallback for UEFI systems that lack it. So implement
this fallback based on the Chaoskey RNG USB stick, which should be
exposed using the standard UEFI USB I/O protocol if the firmware has
USB support.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/firmware/efi/Kconfig                   |   7 ++
 drivers/firmware/efi/libstub/Makefile          |   2 +-
 drivers/firmware/efi/libstub/efistub.h         |   3 +
 drivers/firmware/efi/libstub/random-chaoskey.c | 126 +++++++++++++++++++++++++
 4 files changed, 137 insertions(+), 1 deletion(-)
 create mode 100644 drivers/firmware/efi/libstub/random-chaoskey.c

diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
index 2b4c39fdfa91..448a6186d9fb 100644
--- a/drivers/firmware/efi/Kconfig
+++ b/drivers/firmware/efi/Kconfig
@@ -87,6 +87,13 @@ config EFI_RUNTIME_WRAPPERS
 config EFI_ARMSTUB
 	bool
 
+config EFI_RNG_CHAOSKEY
+	bool "Chaoskey USB RNG support in the UEFI stub"
+	help
+	  Fall back to a Chaoskey RNG USB stick in case EFI_RNG_PROTOCOL
+	  is not implemented by the firmware. This is used for early
+	  seeding of the entropy pool, and for KASLR on ARM and arm64.
+
 config EFI_BOOTLOADER_CONTROL
 	tristate "EFI Bootloader Control"
 	depends on EFI_VARS
diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
index dedf9bde44db..fad6bc1d0e2a 100644
--- a/drivers/firmware/efi/libstub/Makefile
+++ b/drivers/firmware/efi/libstub/Makefile
@@ -40,7 +40,7 @@ $(obj)/lib-%.o: $(srctree)/lib/%.c FORCE
 
 lib-$(CONFIG_EFI_ARMSTUB)	+= arm-stub.o fdt.o string.o random.o \
 				   $(patsubst %.c,lib-%.o,$(arm-deps))
-
+lib-$(CONFIG_EFI_RNG_CHAOSKEY)	+= random-chaoskey.o
 lib-$(CONFIG_ARM)		+= arm32-stub.o
 lib-$(CONFIG_ARM64)		+= arm64-stub.o
 CFLAGS_arm64-stub.o 		:= -DTEXT_OFFSET=$(TEXT_OFFSET)
diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
index 3afff5facd3b..9579ddb5937d 100644
--- a/drivers/firmware/efi/libstub/efistub.h
+++ b/drivers/firmware/efi/libstub/efistub.h
@@ -62,6 +62,9 @@ efi_status_t efi_random_alloc(efi_system_table_t *sys_table_arg,
 			      unsigned long size, unsigned long align,
 			      unsigned long *addr, unsigned long random_seed);
 
+efi_status_t efi_get_random_chaoskey(efi_system_table_t *sys_table_arg,
+				     unsigned long size, u8 *out);
+
 efi_status_t check_platform_features(efi_system_table_t *sys_table_arg);
 
 efi_status_t efi_random_get_seed(efi_system_table_t *sys_table_arg);
diff --git a/drivers/firmware/efi/libstub/random-chaoskey.c b/drivers/firmware/efi/libstub/random-chaoskey.c
new file mode 100644
index 000000000000..3f94ec3df9ff
--- /dev/null
+++ b/drivers/firmware/efi/libstub/random-chaoskey.c
@@ -0,0 +1,126 @@
+/*
+ * Copyright (C) 2017 Linaro Ltd;  <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+
+#include <linux/efi.h>
+#include <asm/efi.h>
+
+#include "efistub.h"
+
+#define CHAOSKEY_VENDOR_ID		0x1d50	/* OpenMoko */
+#define CHAOSKEY_PRODUCT_ID		0x60c6	/* ChaosKey */
+
+static efi_usb_io_protocol_t *chaoskey_usb_io;
+static unsigned long chaoskey_usb_ep_addr, chaoskey_usb_ep_size;
+
+static void locate_chaoskey(efi_system_table_t *sys_table_arg)
+{
+	efi_guid_t usb_io_guid = EFI_USB_IO_PROTOCOL_GUID;
+	unsigned long bufsize = 0, i, j;
+	efi_handle_t *handles;
+	efi_status_t status;
+
+	chaoskey_usb_io = (void *)-1;
+
+	/* find all USB devices that UEFI knows about */
+	status = efi_call_early(locate_handle, EFI_LOCATE_BY_PROTOCOL,
+				&usb_io_guid, NULL, &bufsize, NULL);
+	if (status != EFI_BUFFER_TOO_SMALL)
+		return;
+
+	status = efi_call_early(allocate_pool, EFI_LOADER_DATA, bufsize,
+				(void **)&handles);
+	if (status != EFI_SUCCESS)
+		return;
+
+	status = efi_call_early(locate_handle, EFI_LOCATE_BY_PROTOCOL,
+				&usb_io_guid, NULL, &bufsize, handles);
+	if (status != EFI_SUCCESS)
+		goto out;
+
+	for (i = 0; i < bufsize / sizeof(efi_handle_t); i++) {
+		struct usb_device_descriptor dev;
+		struct usb_interface_descriptor iface;
+		efi_usb_io_protocol_t *p;
+
+		status = efi_call_early(handle_protocol, handles[i],
+					&usb_io_guid, (void **)&p);
+		if (status != EFI_SUCCESS)
+			continue; /* shouldn't happen */
+
+		/* get the device descriptor so we can check the vid/pid */
+		status = p->get_device_descriptor(p, &dev);
+		if (status != EFI_SUCCESS)
+			continue; /* shouldn't happen either */
+
+		if (dev.idVendor != CHAOSKEY_VENDOR_ID ||
+		    dev.idProduct != CHAOSKEY_PRODUCT_ID)
+			continue;
+
+		/* get the number of endpoints from the interface descriptor */
+		status = p->get_iface_descriptor(p, &iface);
+		if (status != EFI_SUCCESS)
+			continue;
+
+		/* locate the first BULK IN endpoint */
+		for (j = 0; j < iface.bNumEndpoints; j++) {
+			struct usb_endpoint_descriptor ep;
+
+			status = p->get_ep_descriptor(p, j, &ep);
+			if (status != EFI_SUCCESS)
+				continue;
+
+			if (!usb_endpoint_dir_in(&ep) ||
+			    usb_endpoint_type(&ep) != USB_ENDPOINT_XFER_BULK)
+				continue;
+
+			pr_efi(sys_table_arg, "Using ChaosKey for entropy\n");
+
+			chaoskey_usb_io = p;
+			chaoskey_usb_ep_addr = ep.bEndpointAddress;
+			chaoskey_usb_ep_size = usb_endpoint_maxp(&ep);
+
+			goto out;
+		}
+	}
+
+out:
+	efi_call_early(free_pool, handles);
+}
+
+efi_status_t efi_get_random_chaoskey(efi_system_table_t *sys_table_arg,
+				     unsigned long size, u8 *out)
+{
+	efi_status_t status;
+
+	if (chaoskey_usb_io == NULL)
+		locate_chaoskey(sys_table_arg);
+	if (chaoskey_usb_io == (void *)-1)
+		return EFI_NOT_FOUND;
+
+	while (size > 0) {
+		u8 buf[chaoskey_usb_ep_size];
+		unsigned long bsize = sizeof(buf);
+		u32 stat;
+
+		status = chaoskey_usb_io->bulk_transfer(chaoskey_usb_io,
+							chaoskey_usb_ep_addr,
+							buf, &bsize, 50, &stat);
+		if (status != EFI_SUCCESS) {
+			pr_efi_err(sys_table_arg,
+				   "ChaosKey USB communication failed!\n");
+			return EFI_NOT_FOUND;
+		}
+
+		bsize = min(size, bsize);
+		memcpy(out, buf, bsize);
+		out += bsize;
+		size -= bsize;
+	}
+	return EFI_SUCCESS;
+}
-- 
2.11.0

Re: [RFC PATCH 2/2] efi: libstub: add support for the Chaoskey RNG USB stick to the stub

[Greg KH]

On Sat, Aug 19, 2017 at 04:17:40PM +0100, Ard Biesheuvel wrote:
> Early entropy is hard to come by, especially on non-x86 systems that
> lack an architected instruction and are not as uniform as PCs.
> Fortunately, on UEFI systems, we can invoke the EFI_RNG_PROTOCOL,
> which exposes the platform specific entropy source in a generic way.
> We use this protocol to fill the RNG UEFI config table (which is used
> during early boot to seed the kernel's entropy pool), and on ARM and
> arm64, we invoke it to seed KASLR as well.
> 
> Sadly, EFI_RNG_PROTOCOL is not widely implemented, and so it would be
> nice to have a fallback for UEFI systems that lack it. So implement
> this fallback based on the Chaoskey RNG USB stick, which should be
> exposed using the standard UEFI USB I/O protocol if the firmware has
> USB support.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  drivers/firmware/efi/Kconfig                   |   7 ++
>  drivers/firmware/efi/libstub/Makefile          |   2 +-
>  drivers/firmware/efi/libstub/efistub.h         |   3 +
>  drivers/firmware/efi/libstub/random-chaoskey.c | 126 +++++++++++++++++++++++++
>  4 files changed, 137 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/firmware/efi/libstub/random-chaoskey.c
> 
> diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
> index 2b4c39fdfa91..448a6186d9fb 100644
> --- a/drivers/firmware/efi/Kconfig
> +++ b/drivers/firmware/efi/Kconfig
> @@ -87,6 +87,13 @@ config EFI_RUNTIME_WRAPPERS
>  config EFI_ARMSTUB
>  	bool
>  
> +config EFI_RNG_CHAOSKEY
> +	bool "Chaoskey USB RNG support in the UEFI stub"
> +	help
> +	  Fall back to a Chaoskey RNG USB stick in case EFI_RNG_PROTOCOL
> +	  is not implemented by the firmware. This is used for early
> +	  seeding of the entropy pool, and for KASLR on ARM and arm64.
> +
>  config EFI_BOOTLOADER_CONTROL
>  	tristate "EFI Bootloader Control"
>  	depends on EFI_VARS
> diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
> index dedf9bde44db..fad6bc1d0e2a 100644
> --- a/drivers/firmware/efi/libstub/Makefile
> +++ b/drivers/firmware/efi/libstub/Makefile
> @@ -40,7 +40,7 @@ $(obj)/lib-%.o: $(srctree)/lib/%.c FORCE
>  
>  lib-$(CONFIG_EFI_ARMSTUB)	+= arm-stub.o fdt.o string.o random.o \
>  				   $(patsubst %.c,lib-%.o,$(arm-deps))
> -
> +lib-$(CONFIG_EFI_RNG_CHAOSKEY)	+= random-chaoskey.o
>  lib-$(CONFIG_ARM)		+= arm32-stub.o
>  lib-$(CONFIG_ARM64)		+= arm64-stub.o
>  CFLAGS_arm64-stub.o 		:= -DTEXT_OFFSET=$(TEXT_OFFSET)
> diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
> index 3afff5facd3b..9579ddb5937d 100644
> --- a/drivers/firmware/efi/libstub/efistub.h
> +++ b/drivers/firmware/efi/libstub/efistub.h
> @@ -62,6 +62,9 @@ efi_status_t efi_random_alloc(efi_system_table_t *sys_table_arg,
>  			      unsigned long size, unsigned long align,
>  			      unsigned long *addr, unsigned long random_seed);
>  
> +efi_status_t efi_get_random_chaoskey(efi_system_table_t *sys_table_arg,
> +				     unsigned long size, u8 *out);
> +
>  efi_status_t check_platform_features(efi_system_table_t *sys_table_arg);
>  
>  efi_status_t efi_random_get_seed(efi_system_table_t *sys_table_arg);
> diff --git a/drivers/firmware/efi/libstub/random-chaoskey.c b/drivers/firmware/efi/libstub/random-chaoskey.c
> new file mode 100644
> index 000000000000..3f94ec3df9ff
> --- /dev/null
> +++ b/drivers/firmware/efi/libstub/random-chaoskey.c
> @@ -0,0 +1,126 @@
> +/*
> + * Copyright (C) 2017 Linaro Ltd;  <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + */
> +
> +#include <linux/efi.h>
> +#include <asm/efi.h>
> +
> +#include "efistub.h"
> +
> +#define CHAOSKEY_VENDOR_ID		0x1d50	/* OpenMoko */
> +#define CHAOSKEY_PRODUCT_ID		0x60c6	/* ChaosKey */
> +
> +static efi_usb_io_protocol_t *chaoskey_usb_io;
> +static unsigned long chaoskey_usb_ep_addr, chaoskey_usb_ep_size;
> +
> +static void locate_chaoskey(efi_system_table_t *sys_table_arg)
> +{
> +	efi_guid_t usb_io_guid = EFI_USB_IO_PROTOCOL_GUID;
> +	unsigned long bufsize = 0, i, j;
> +	efi_handle_t *handles;
> +	efi_status_t status;
> +
> +	chaoskey_usb_io = (void *)-1;
> +
> +	/* find all USB devices that UEFI knows about */
> +	status = efi_call_early(locate_handle, EFI_LOCATE_BY_PROTOCOL,
> +				&usb_io_guid, NULL, &bufsize, NULL);
> +	if (status != EFI_BUFFER_TOO_SMALL)
> +		return;
> +
> +	status = efi_call_early(allocate_pool, EFI_LOADER_DATA, bufsize,
> +				(void **)&handles);
> +	if (status != EFI_SUCCESS)
> +		return;
> +
> +	status = efi_call_early(locate_handle, EFI_LOCATE_BY_PROTOCOL,
> +				&usb_io_guid, NULL, &bufsize, handles);
> +	if (status != EFI_SUCCESS)
> +		goto out;
> +
> +	for (i = 0; i < bufsize / sizeof(efi_handle_t); i++) {
> +		struct usb_device_descriptor dev;
> +		struct usb_interface_descriptor iface;
> +		efi_usb_io_protocol_t *p;
> +
> +		status = efi_call_early(handle_protocol, handles[i],
> +					&usb_io_guid, (void **)&p);
> +		if (status != EFI_SUCCESS)
> +			continue; /* shouldn't happen */
> +
> +		/* get the device descriptor so we can check the vid/pid */
> +		status = p->get_device_descriptor(p, &dev);
> +		if (status != EFI_SUCCESS)
> +			continue; /* shouldn't happen either */
> +
> +		if (dev.idVendor != CHAOSKEY_VENDOR_ID ||
> +		    dev.idProduct != CHAOSKEY_PRODUCT_ID)

idVendor and idProduct are little endian types.

You have run sparse on this code, right?


> +			continue;
> +
> +		/* get the number of endpoints from the interface descriptor */
> +		status = p->get_iface_descriptor(p, &iface);
> +		if (status != EFI_SUCCESS)
> +			continue;
> +
> +		/* locate the first BULK IN endpoint */
> +		for (j = 0; j < iface.bNumEndpoints; j++) {
> +			struct usb_endpoint_descriptor ep;

Don't we have built-in usb functions for this now?  Why not use them
instead of hand-rolling your own logic?

thanks,

greg k-h

Re: [RFC PATCH 2/2] efi: libstub: add support for the Chaoskey RNG USB stick to the stub

[Ard Biesheuvel]

On 2 September 2017 at 07:45, Greg KH <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org> wrote:
> On Sat, Aug 19, 2017 at 04:17:40PM +0100, Ard Biesheuvel wrote:
>> Early entropy is hard to come by, especially on non-x86 systems that
>> lack an architected instruction and are not as uniform as PCs.
>> Fortunately, on UEFI systems, we can invoke the EFI_RNG_PROTOCOL,
>> which exposes the platform specific entropy source in a generic way.
>> We use this protocol to fill the RNG UEFI config table (which is used
>> during early boot to seed the kernel's entropy pool), and on ARM and
>> arm64, we invoke it to seed KASLR as well.
>>
>> Sadly, EFI_RNG_PROTOCOL is not widely implemented, and so it would be
>> nice to have a fallback for UEFI systems that lack it. So implement
>> this fallback based on the Chaoskey RNG USB stick, which should be
>> exposed using the standard UEFI USB I/O protocol if the firmware has
>> USB support.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> ---
>>  drivers/firmware/efi/Kconfig                   |   7 ++
>>  drivers/firmware/efi/libstub/Makefile          |   2 +-
>>  drivers/firmware/efi/libstub/efistub.h         |   3 +
>>  drivers/firmware/efi/libstub/random-chaoskey.c | 126 +++++++++++++++++++++++++
>>  4 files changed, 137 insertions(+), 1 deletion(-)
>>  create mode 100644 drivers/firmware/efi/libstub/random-chaoskey.c
>>
>> diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
>> index 2b4c39fdfa91..448a6186d9fb 100644
>> --- a/drivers/firmware/efi/Kconfig
>> +++ b/drivers/firmware/efi/Kconfig
>> @@ -87,6 +87,13 @@ config EFI_RUNTIME_WRAPPERS
>>  config EFI_ARMSTUB
>>       bool
>>
>> +config EFI_RNG_CHAOSKEY
>> +     bool "Chaoskey USB RNG support in the UEFI stub"
>> +     help
>> +       Fall back to a Chaoskey RNG USB stick in case EFI_RNG_PROTOCOL
>> +       is not implemented by the firmware. This is used for early
>> +       seeding of the entropy pool, and for KASLR on ARM and arm64.
>> +
>>  config EFI_BOOTLOADER_CONTROL
>>       tristate "EFI Bootloader Control"
>>       depends on EFI_VARS
>> diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
>> index dedf9bde44db..fad6bc1d0e2a 100644
>> --- a/drivers/firmware/efi/libstub/Makefile
>> +++ b/drivers/firmware/efi/libstub/Makefile
>> @@ -40,7 +40,7 @@ $(obj)/lib-%.o: $(srctree)/lib/%.c FORCE
>>
>>  lib-$(CONFIG_EFI_ARMSTUB)    += arm-stub.o fdt.o string.o random.o \
>>                                  $(patsubst %.c,lib-%.o,$(arm-deps))
>> -
>> +lib-$(CONFIG_EFI_RNG_CHAOSKEY)       += random-chaoskey.o
>>  lib-$(CONFIG_ARM)            += arm32-stub.o
>>  lib-$(CONFIG_ARM64)          += arm64-stub.o
>>  CFLAGS_arm64-stub.o          := -DTEXT_OFFSET=$(TEXT_OFFSET)
>> diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
>> index 3afff5facd3b..9579ddb5937d 100644
>> --- a/drivers/firmware/efi/libstub/efistub.h
>> +++ b/drivers/firmware/efi/libstub/efistub.h
>> @@ -62,6 +62,9 @@ efi_status_t efi_random_alloc(efi_system_table_t *sys_table_arg,
>>                             unsigned long size, unsigned long align,
>>                             unsigned long *addr, unsigned long random_seed);
>>
>> +efi_status_t efi_get_random_chaoskey(efi_system_table_t *sys_table_arg,
>> +                                  unsigned long size, u8 *out);
>> +
>>  efi_status_t check_platform_features(efi_system_table_t *sys_table_arg);
>>
>>  efi_status_t efi_random_get_seed(efi_system_table_t *sys_table_arg);
>> diff --git a/drivers/firmware/efi/libstub/random-chaoskey.c b/drivers/firmware/efi/libstub/random-chaoskey.c
>> new file mode 100644
>> index 000000000000..3f94ec3df9ff
>> --- /dev/null
>> +++ b/drivers/firmware/efi/libstub/random-chaoskey.c
>> @@ -0,0 +1,126 @@
>> +/*
>> + * Copyright (C) 2017 Linaro Ltd;  <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + */
>> +
>> +#include <linux/efi.h>
>> +#include <asm/efi.h>
>> +
>> +#include "efistub.h"
>> +
>> +#define CHAOSKEY_VENDOR_ID           0x1d50  /* OpenMoko */
>> +#define CHAOSKEY_PRODUCT_ID          0x60c6  /* ChaosKey */
>> +
>> +static efi_usb_io_protocol_t *chaoskey_usb_io;
>> +static unsigned long chaoskey_usb_ep_addr, chaoskey_usb_ep_size;
>> +
>> +static void locate_chaoskey(efi_system_table_t *sys_table_arg)
>> +{
>> +     efi_guid_t usb_io_guid = EFI_USB_IO_PROTOCOL_GUID;
>> +     unsigned long bufsize = 0, i, j;
>> +     efi_handle_t *handles;
>> +     efi_status_t status;
>> +
>> +     chaoskey_usb_io = (void *)-1;
>> +
>> +     /* find all USB devices that UEFI knows about */
>> +     status = efi_call_early(locate_handle, EFI_LOCATE_BY_PROTOCOL,
>> +                             &usb_io_guid, NULL, &bufsize, NULL);
>> +     if (status != EFI_BUFFER_TOO_SMALL)
>> +             return;
>> +
>> +     status = efi_call_early(allocate_pool, EFI_LOADER_DATA, bufsize,
>> +                             (void **)&handles);
>> +     if (status != EFI_SUCCESS)
>> +             return;
>> +
>> +     status = efi_call_early(locate_handle, EFI_LOCATE_BY_PROTOCOL,
>> +                             &usb_io_guid, NULL, &bufsize, handles);
>> +     if (status != EFI_SUCCESS)
>> +             goto out;
>> +
>> +     for (i = 0; i < bufsize / sizeof(efi_handle_t); i++) {
>> +             struct usb_device_descriptor dev;
>> +             struct usb_interface_descriptor iface;
>> +             efi_usb_io_protocol_t *p;
>> +
>> +             status = efi_call_early(handle_protocol, handles[i],
>> +                                     &usb_io_guid, (void **)&p);
>> +             if (status != EFI_SUCCESS)
>> +                     continue; /* shouldn't happen */
>> +
>> +             /* get the device descriptor so we can check the vid/pid */
>> +             status = p->get_device_descriptor(p, &dev);
>> +             if (status != EFI_SUCCESS)
>> +                     continue; /* shouldn't happen either */
>> +
>> +             if (dev.idVendor != CHAOSKEY_VENDOR_ID ||
>> +                 dev.idProduct != CHAOSKEY_PRODUCT_ID)
>
> idVendor and idProduct are little endian types.
>
> You have run sparse on this code, right?
>
>
>> +                     continue;
>> +
>> +             /* get the number of endpoints from the interface descriptor */
>> +             status = p->get_iface_descriptor(p, &iface);
>> +             if (status != EFI_SUCCESS)
>> +                     continue;
>> +
>> +             /* locate the first BULK IN endpoint */
>> +             for (j = 0; j < iface.bNumEndpoints; j++) {
>> +                     struct usb_endpoint_descriptor ep;
>
> Don't we have built-in usb functions for this now?  Why not use them
> instead of hand-rolling your own logic?
>

This is UEFI stub code, which is executed with the kernel image or
decompressor loaded at a completely different address than it was
linked at. Reusing inline functions is usually fine, but anything that
requires linking kernel objects into the decompressor is a bit
cumbersome.

In any case, these patches are RFC only, and it is quite obvious that
having a Chaoskey USB driver in the firmware that produces the
EFI_RNG_PROTOCOL is much better, and the UEFI stub code can just call
into the protocol to get entropy.

Re: [RFC PATCH 2/2] efi: libstub: add support for the Chaoskey RNG USB stick to the stub

[Greg KH]

On Sat, Sep 02, 2017 at 09:18:34AM +0100, Ard Biesheuvel wrote:
> On 2 September 2017 at 07:45, Greg KH <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org> wrote:
> > On Sat, Aug 19, 2017 at 04:17:40PM +0100, Ard Biesheuvel wrote:
> >> Early entropy is hard to come by, especially on non-x86 systems that
> >> lack an architected instruction and are not as uniform as PCs.
> >> Fortunately, on UEFI systems, we can invoke the EFI_RNG_PROTOCOL,
> >> which exposes the platform specific entropy source in a generic way.
> >> We use this protocol to fill the RNG UEFI config table (which is used
> >> during early boot to seed the kernel's entropy pool), and on ARM and
> >> arm64, we invoke it to seed KASLR as well.
> >>
> >> Sadly, EFI_RNG_PROTOCOL is not widely implemented, and so it would be
> >> nice to have a fallback for UEFI systems that lack it. So implement
> >> this fallback based on the Chaoskey RNG USB stick, which should be
> >> exposed using the standard UEFI USB I/O protocol if the firmware has
> >> USB support.
> >>
> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> >> ---
> >>  drivers/firmware/efi/Kconfig                   |   7 ++
> >>  drivers/firmware/efi/libstub/Makefile          |   2 +-
> >>  drivers/firmware/efi/libstub/efistub.h         |   3 +
> >>  drivers/firmware/efi/libstub/random-chaoskey.c | 126 +++++++++++++++++++++++++
> >>  4 files changed, 137 insertions(+), 1 deletion(-)
> >>  create mode 100644 drivers/firmware/efi/libstub/random-chaoskey.c
> >>
> >> diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
> >> index 2b4c39fdfa91..448a6186d9fb 100644
> >> --- a/drivers/firmware/efi/Kconfig
> >> +++ b/drivers/firmware/efi/Kconfig
> >> @@ -87,6 +87,13 @@ config EFI_RUNTIME_WRAPPERS
> >>  config EFI_ARMSTUB
> >>       bool
> >>
> >> +config EFI_RNG_CHAOSKEY
> >> +     bool "Chaoskey USB RNG support in the UEFI stub"
> >> +     help
> >> +       Fall back to a Chaoskey RNG USB stick in case EFI_RNG_PROTOCOL
> >> +       is not implemented by the firmware. This is used for early
> >> +       seeding of the entropy pool, and for KASLR on ARM and arm64.
> >> +
> >>  config EFI_BOOTLOADER_CONTROL
> >>       tristate "EFI Bootloader Control"
> >>       depends on EFI_VARS
> >> diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
> >> index dedf9bde44db..fad6bc1d0e2a 100644
> >> --- a/drivers/firmware/efi/libstub/Makefile
> >> +++ b/drivers/firmware/efi/libstub/Makefile
> >> @@ -40,7 +40,7 @@ $(obj)/lib-%.o: $(srctree)/lib/%.c FORCE
> >>
> >>  lib-$(CONFIG_EFI_ARMSTUB)    += arm-stub.o fdt.o string.o random.o \
> >>                                  $(patsubst %.c,lib-%.o,$(arm-deps))
> >> -
> >> +lib-$(CONFIG_EFI_RNG_CHAOSKEY)       += random-chaoskey.o
> >>  lib-$(CONFIG_ARM)            += arm32-stub.o
> >>  lib-$(CONFIG_ARM64)          += arm64-stub.o
> >>  CFLAGS_arm64-stub.o          := -DTEXT_OFFSET=$(TEXT_OFFSET)
> >> diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
> >> index 3afff5facd3b..9579ddb5937d 100644
> >> --- a/drivers/firmware/efi/libstub/efistub.h
> >> +++ b/drivers/firmware/efi/libstub/efistub.h
> >> @@ -62,6 +62,9 @@ efi_status_t efi_random_alloc(efi_system_table_t *sys_table_arg,
> >>                             unsigned long size, unsigned long align,
> >>                             unsigned long *addr, unsigned long random_seed);
> >>
> >> +efi_status_t efi_get_random_chaoskey(efi_system_table_t *sys_table_arg,
> >> +                                  unsigned long size, u8 *out);
> >> +
> >>  efi_status_t check_platform_features(efi_system_table_t *sys_table_arg);
> >>
> >>  efi_status_t efi_random_get_seed(efi_system_table_t *sys_table_arg);
> >> diff --git a/drivers/firmware/efi/libstub/random-chaoskey.c b/drivers/firmware/efi/libstub/random-chaoskey.c
> >> new file mode 100644
> >> index 000000000000..3f94ec3df9ff
> >> --- /dev/null
> >> +++ b/drivers/firmware/efi/libstub/random-chaoskey.c
> >> @@ -0,0 +1,126 @@
> >> +/*
> >> + * Copyright (C) 2017 Linaro Ltd;  <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> >> + *
> >> + * This program is free software; you can redistribute it and/or modify
> >> + * it under the terms of the GNU General Public License version 2 as
> >> + * published by the Free Software Foundation.
> >> + *
> >> + */
> >> +
> >> +#include <linux/efi.h>
> >> +#include <asm/efi.h>
> >> +
> >> +#include "efistub.h"
> >> +
> >> +#define CHAOSKEY_VENDOR_ID           0x1d50  /* OpenMoko */
> >> +#define CHAOSKEY_PRODUCT_ID          0x60c6  /* ChaosKey */
> >> +
> >> +static efi_usb_io_protocol_t *chaoskey_usb_io;
> >> +static unsigned long chaoskey_usb_ep_addr, chaoskey_usb_ep_size;
> >> +
> >> +static void locate_chaoskey(efi_system_table_t *sys_table_arg)
> >> +{
> >> +     efi_guid_t usb_io_guid = EFI_USB_IO_PROTOCOL_GUID;
> >> +     unsigned long bufsize = 0, i, j;
> >> +     efi_handle_t *handles;
> >> +     efi_status_t status;
> >> +
> >> +     chaoskey_usb_io = (void *)-1;
> >> +
> >> +     /* find all USB devices that UEFI knows about */
> >> +     status = efi_call_early(locate_handle, EFI_LOCATE_BY_PROTOCOL,
> >> +                             &usb_io_guid, NULL, &bufsize, NULL);
> >> +     if (status != EFI_BUFFER_TOO_SMALL)
> >> +             return;
> >> +
> >> +     status = efi_call_early(allocate_pool, EFI_LOADER_DATA, bufsize,
> >> +                             (void **)&handles);
> >> +     if (status != EFI_SUCCESS)
> >> +             return;
> >> +
> >> +     status = efi_call_early(locate_handle, EFI_LOCATE_BY_PROTOCOL,
> >> +                             &usb_io_guid, NULL, &bufsize, handles);
> >> +     if (status != EFI_SUCCESS)
> >> +             goto out;
> >> +
> >> +     for (i = 0; i < bufsize / sizeof(efi_handle_t); i++) {
> >> +             struct usb_device_descriptor dev;
> >> +             struct usb_interface_descriptor iface;
> >> +             efi_usb_io_protocol_t *p;
> >> +
> >> +             status = efi_call_early(handle_protocol, handles[i],
> >> +                                     &usb_io_guid, (void **)&p);
> >> +             if (status != EFI_SUCCESS)
> >> +                     continue; /* shouldn't happen */
> >> +
> >> +             /* get the device descriptor so we can check the vid/pid */
> >> +             status = p->get_device_descriptor(p, &dev);
> >> +             if (status != EFI_SUCCESS)
> >> +                     continue; /* shouldn't happen either */
> >> +
> >> +             if (dev.idVendor != CHAOSKEY_VENDOR_ID ||
> >> +                 dev.idProduct != CHAOSKEY_PRODUCT_ID)
> >
> > idVendor and idProduct are little endian types.
> >
> > You have run sparse on this code, right?
> >
> >
> >> +                     continue;
> >> +
> >> +             /* get the number of endpoints from the interface descriptor */
> >> +             status = p->get_iface_descriptor(p, &iface);
> >> +             if (status != EFI_SUCCESS)
> >> +                     continue;
> >> +
> >> +             /* locate the first BULK IN endpoint */
> >> +             for (j = 0; j < iface.bNumEndpoints; j++) {
> >> +                     struct usb_endpoint_descriptor ep;
> >
> > Don't we have built-in usb functions for this now?  Why not use them
> > instead of hand-rolling your own logic?
> >
> 
> This is UEFI stub code, which is executed with the kernel image or
> decompressor loaded at a completely different address than it was
> linked at. Reusing inline functions is usually fine, but anything that
> requires linking kernel objects into the decompressor is a bit
> cumbersome.

So this is not Linux kernel code?  Or is it?  I'm totally confused now.

> In any case, these patches are RFC only, and it is quite obvious that
> having a Chaoskey USB driver in the firmware that produces the
> EFI_RNG_PROTOCOL is much better, and the UEFI stub code can just call
> into the protocol to get entropy.

Ok, why not write that and not have to deal with any Linux code at all?

thanks,

greg k-h

Re: [RFC PATCH 2/2] efi: libstub: add support for the Chaoskey RNG USB stick to the stub

[Ard Biesheuvel]

On 2 September 2017 at 09:26, Greg KH <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org> wrote:
> On Sat, Sep 02, 2017 at 09:18:34AM +0100, Ard Biesheuvel wrote:
>> On 2 September 2017 at 07:45, Greg KH <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org> wrote:
>> > On Sat, Aug 19, 2017 at 04:17:40PM +0100, Ard Biesheuvel wrote:
>> >> Early entropy is hard to come by, especially on non-x86 systems that
>> >> lack an architected instruction and are not as uniform as PCs.
>> >> Fortunately, on UEFI systems, we can invoke the EFI_RNG_PROTOCOL,
>> >> which exposes the platform specific entropy source in a generic way.
>> >> We use this protocol to fill the RNG UEFI config table (which is used
>> >> during early boot to seed the kernel's entropy pool), and on ARM and
>> >> arm64, we invoke it to seed KASLR as well.
>> >>
>> >> Sadly, EFI_RNG_PROTOCOL is not widely implemented, and so it would be
>> >> nice to have a fallback for UEFI systems that lack it. So implement
>> >> this fallback based on the Chaoskey RNG USB stick, which should be
>> >> exposed using the standard UEFI USB I/O protocol if the firmware has
>> >> USB support.
>> >>
>> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> >> ---
>> >>  drivers/firmware/efi/Kconfig                   |   7 ++
>> >>  drivers/firmware/efi/libstub/Makefile          |   2 +-
>> >>  drivers/firmware/efi/libstub/efistub.h         |   3 +
>> >>  drivers/firmware/efi/libstub/random-chaoskey.c | 126 +++++++++++++++++++++++++
>> >>  4 files changed, 137 insertions(+), 1 deletion(-)
>> >>  create mode 100644 drivers/firmware/efi/libstub/random-chaoskey.c
>> >>
>> >> diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
>> >> index 2b4c39fdfa91..448a6186d9fb 100644
>> >> --- a/drivers/firmware/efi/Kconfig
>> >> +++ b/drivers/firmware/efi/Kconfig
>> >> @@ -87,6 +87,13 @@ config EFI_RUNTIME_WRAPPERS
>> >>  config EFI_ARMSTUB
>> >>       bool
>> >>
>> >> +config EFI_RNG_CHAOSKEY
>> >> +     bool "Chaoskey USB RNG support in the UEFI stub"
>> >> +     help
>> >> +       Fall back to a Chaoskey RNG USB stick in case EFI_RNG_PROTOCOL
>> >> +       is not implemented by the firmware. This is used for early
>> >> +       seeding of the entropy pool, and for KASLR on ARM and arm64.
>> >> +
>> >>  config EFI_BOOTLOADER_CONTROL
>> >>       tristate "EFI Bootloader Control"
>> >>       depends on EFI_VARS
>> >> diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
>> >> index dedf9bde44db..fad6bc1d0e2a 100644
>> >> --- a/drivers/firmware/efi/libstub/Makefile
>> >> +++ b/drivers/firmware/efi/libstub/Makefile
>> >> @@ -40,7 +40,7 @@ $(obj)/lib-%.o: $(srctree)/lib/%.c FORCE
>> >>
>> >>  lib-$(CONFIG_EFI_ARMSTUB)    += arm-stub.o fdt.o string.o random.o \
>> >>                                  $(patsubst %.c,lib-%.o,$(arm-deps))
>> >> -
>> >> +lib-$(CONFIG_EFI_RNG_CHAOSKEY)       += random-chaoskey.o
>> >>  lib-$(CONFIG_ARM)            += arm32-stub.o
>> >>  lib-$(CONFIG_ARM64)          += arm64-stub.o
>> >>  CFLAGS_arm64-stub.o          := -DTEXT_OFFSET=$(TEXT_OFFSET)
>> >> diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
>> >> index 3afff5facd3b..9579ddb5937d 100644
>> >> --- a/drivers/firmware/efi/libstub/efistub.h
>> >> +++ b/drivers/firmware/efi/libstub/efistub.h
>> >> @@ -62,6 +62,9 @@ efi_status_t efi_random_alloc(efi_system_table_t *sys_table_arg,
>> >>                             unsigned long size, unsigned long align,
>> >>                             unsigned long *addr, unsigned long random_seed);
>> >>
>> >> +efi_status_t efi_get_random_chaoskey(efi_system_table_t *sys_table_arg,
>> >> +                                  unsigned long size, u8 *out);
>> >> +
>> >>  efi_status_t check_platform_features(efi_system_table_t *sys_table_arg);
>> >>
>> >>  efi_status_t efi_random_get_seed(efi_system_table_t *sys_table_arg);
>> >> diff --git a/drivers/firmware/efi/libstub/random-chaoskey.c b/drivers/firmware/efi/libstub/random-chaoskey.c
>> >> new file mode 100644
>> >> index 000000000000..3f94ec3df9ff
>> >> --- /dev/null
>> >> +++ b/drivers/firmware/efi/libstub/random-chaoskey.c
>> >> @@ -0,0 +1,126 @@
>> >> +/*
>> >> + * Copyright (C) 2017 Linaro Ltd;  <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> >> + *
>> >> + * This program is free software; you can redistribute it and/or modify
>> >> + * it under the terms of the GNU General Public License version 2 as
>> >> + * published by the Free Software Foundation.
>> >> + *
>> >> + */
>> >> +
>> >> +#include <linux/efi.h>
>> >> +#include <asm/efi.h>
>> >> +
>> >> +#include "efistub.h"
>> >> +
>> >> +#define CHAOSKEY_VENDOR_ID           0x1d50  /* OpenMoko */
>> >> +#define CHAOSKEY_PRODUCT_ID          0x60c6  /* ChaosKey */
>> >> +
>> >> +static efi_usb_io_protocol_t *chaoskey_usb_io;
>> >> +static unsigned long chaoskey_usb_ep_addr, chaoskey_usb_ep_size;
>> >> +
>> >> +static void locate_chaoskey(efi_system_table_t *sys_table_arg)
>> >> +{
>> >> +     efi_guid_t usb_io_guid = EFI_USB_IO_PROTOCOL_GUID;
>> >> +     unsigned long bufsize = 0, i, j;
>> >> +     efi_handle_t *handles;
>> >> +     efi_status_t status;
>> >> +
>> >> +     chaoskey_usb_io = (void *)-1;
>> >> +
>> >> +     /* find all USB devices that UEFI knows about */
>> >> +     status = efi_call_early(locate_handle, EFI_LOCATE_BY_PROTOCOL,
>> >> +                             &usb_io_guid, NULL, &bufsize, NULL);
>> >> +     if (status != EFI_BUFFER_TOO_SMALL)
>> >> +             return;
>> >> +
>> >> +     status = efi_call_early(allocate_pool, EFI_LOADER_DATA, bufsize,
>> >> +                             (void **)&handles);
>> >> +     if (status != EFI_SUCCESS)
>> >> +             return;
>> >> +
>> >> +     status = efi_call_early(locate_handle, EFI_LOCATE_BY_PROTOCOL,
>> >> +                             &usb_io_guid, NULL, &bufsize, handles);
>> >> +     if (status != EFI_SUCCESS)
>> >> +             goto out;
>> >> +
>> >> +     for (i = 0; i < bufsize / sizeof(efi_handle_t); i++) {
>> >> +             struct usb_device_descriptor dev;
>> >> +             struct usb_interface_descriptor iface;
>> >> +             efi_usb_io_protocol_t *p;
>> >> +
>> >> +             status = efi_call_early(handle_protocol, handles[i],
>> >> +                                     &usb_io_guid, (void **)&p);
>> >> +             if (status != EFI_SUCCESS)
>> >> +                     continue; /* shouldn't happen */
>> >> +
>> >> +             /* get the device descriptor so we can check the vid/pid */
>> >> +             status = p->get_device_descriptor(p, &dev);
>> >> +             if (status != EFI_SUCCESS)
>> >> +                     continue; /* shouldn't happen either */
>> >> +
>> >> +             if (dev.idVendor != CHAOSKEY_VENDOR_ID ||
>> >> +                 dev.idProduct != CHAOSKEY_PRODUCT_ID)
>> >
>> > idVendor and idProduct are little endian types.
>> >
>> > You have run sparse on this code, right?
>> >
>> >
>> >> +                     continue;
>> >> +
>> >> +             /* get the number of endpoints from the interface descriptor */
>> >> +             status = p->get_iface_descriptor(p, &iface);
>> >> +             if (status != EFI_SUCCESS)
>> >> +                     continue;
>> >> +
>> >> +             /* locate the first BULK IN endpoint */
>> >> +             for (j = 0; j < iface.bNumEndpoints; j++) {
>> >> +                     struct usb_endpoint_descriptor ep;
>> >
>> > Don't we have built-in usb functions for this now?  Why not use them
>> > instead of hand-rolling your own logic?
>> >
>>
>> This is UEFI stub code, which is executed with the kernel image or
>> decompressor loaded at a completely different address than it was
>> linked at. Reusing inline functions is usually fine, but anything that
>> requires linking kernel objects into the decompressor is a bit
>> cumbersome.
>
> So this is not Linux kernel code?  Or is it?  I'm totally confused now.
>

All code that lives under drivers/firmware/efi/libstub is linked into
a PE/COFF binary that is either embedded into the decompressor (x86,
ARM) or the kernel proper (arm64) but with a separate __efistub_
namespace. It implements the so-called OS loader (in UEFI speak) that
loads the images and interfaces with the boot environment in other
ways to prepare for OS boot. At this point, we are still running in
the 1:1 mapping set up by UEFI, and the PE/COFF image (which contains
the entire kernel image as a payload, in one way or another) is loaded
at an arbitrary offset by the firmware, and invoked from there.

>> In any case, these patches are RFC only, and it is quite obvious that
>> having a Chaoskey USB driver in the firmware that produces the
>> EFI_RNG_PROTOCOL is much better, and the UEFI stub code can just call
>> into the protocol to get entropy.
>
> Ok, why not write that and not have to deal with any Linux code at all?
>

Oh, we did write it. And we are leaning towards withdrawing these
patches, but only because adding support for individual devices is a
bit icky. The way these patches interface with the firmware is no
different from all the other Linux code that does so.

Re: [RFC PATCH 1/2] efi: import USB I/O related declarations from the UEFI spec

[Greg KH]

On Sat, Aug 19, 2017 at 04:17:39PM +0100, Ard Biesheuvel wrote:
> In preparation of adding support for the Chaoskey USB stick to the
> UEFI stub, import the USB I/O protocol declarations and related types
> to linux/efi.h.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  include/linux/efi.h | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 66 insertions(+)
> 
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index 12e05118657c..253749cd9b62 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -22,6 +22,7 @@
>  #include <linux/pstore.h>
>  #include <linux/range.h>
>  #include <linux/reboot.h>
> +#include <linux/usb/ch9.h>
>  #include <linux/uuid.h>
>  #include <linux/screen_info.h>
>  
> @@ -622,6 +623,7 @@ void efi_native_runtime_setup(void);
>  #define EFI_MEMORY_ATTRIBUTES_TABLE_GUID	EFI_GUID(0xdcfa911d, 0x26eb, 0x469f,  0xa2, 0x20, 0x38, 0xb7, 0xdc, 0x46, 0x12, 0x20)
>  #define EFI_CONSOLE_OUT_DEVICE_GUID		EFI_GUID(0xd3b36f2c, 0xd551, 0x11d4,  0x9a, 0x46, 0x00, 0x90, 0x27, 0x3f, 0xc1, 0x4d)
>  #define APPLE_PROPERTIES_PROTOCOL_GUID		EFI_GUID(0x91bd12fe, 0xf6c3, 0x44fb,  0xa5, 0xb7, 0x51, 0x22, 0xab, 0x30, 0x3a, 0xe0)
> +#define EFI_USB_IO_PROTOCOL_GUID		EFI_GUID(0x2b2f68d6, 0x0cd2, 0x44cf,  0x8e, 0x8b, 0xbb, 0xa2, 0x0b, 0x1b, 0x5b, 0x75)
>  
>  #define EFI_IMAGE_SECURITY_DATABASE_GUID	EFI_GUID(0xd719b2cb, 0x3d3a, 0x4596,  0xa3, 0xbc, 0xda, 0xd0, 0x0e, 0x67, 0x65, 0x6f)
>  #define EFI_SHIM_LOCK_GUID			EFI_GUID(0x605dab50, 0xe046, 0x4300,  0xab, 0xb6, 0x3d, 0xd8, 0x10, 0xdd, 0x8b, 0x23)
> @@ -1569,4 +1571,68 @@ struct linux_efi_random_seed {
>  	u8	bits[];
>  };
>  
> +typedef enum {
> +	EfiUsbDataIn,
> +	EfiUsbDataOut,
> +	EfiUsbNoData
> +} efi_usb_data_direction_t;
> +
> +#define EFI_USB_NOERROR			0x0000
> +#define EFI_USB_ERR_NOTEXECUTE		0x0001
> +#define EFI_USB_ERR_STALL		0x0002
> +#define EFI_USB_ERR_BUFFER		0x0004
> +#define EFI_USB_ERR_BABBLE		0x0008
> +#define EFI_USB_ERR_NAK			0x0010
> +#define EFI_USB_ERR_CRC			0x0020
> +#define EFI_USB_ERR_TIMEOUT		0x0040
> +#define EFI_USB_ERR_BITSTUFF		0x0080
> +#define EFI_USB_ERR_SYSTEM		0x0100
> +
> +typedef struct {
> +	u8	request_type;
> +	u8	request;
> +	u16	value;
> +	u16	index;
> +	u16	length;
> +} efi_usb_device_request_t;

A typedef?

Also, those values are little-endian, right?  And finally, they are
crossing the user/kernel boundry, so they should be using the __ variant
of the variable type, right?

And finally, why does efi.h care about device specific stuff like this?
Are you going to want to add all different types of efi devices here in
the future?  That does not seem wise from a maintaince point-of-view...

thanks,

greg k-h

Re: [RFC PATCH 1/2] efi: import USB I/O related declarations from the UEFI spec

[Ard Biesheuvel]

On 2 September 2017 at 07:41, Greg KH <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org> wrote:
> On Sat, Aug 19, 2017 at 04:17:39PM +0100, Ard Biesheuvel wrote:
>> In preparation of adding support for the Chaoskey USB stick to the
>> UEFI stub, import the USB I/O protocol declarations and related types
>> to linux/efi.h.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> ---
>>  include/linux/efi.h | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 66 insertions(+)
>>
>> diff --git a/include/linux/efi.h b/include/linux/efi.h
>> index 12e05118657c..253749cd9b62 100644
>> --- a/include/linux/efi.h
>> +++ b/include/linux/efi.h
>> @@ -22,6 +22,7 @@
>>  #include <linux/pstore.h>
>>  #include <linux/range.h>
>>  #include <linux/reboot.h>
>> +#include <linux/usb/ch9.h>
>>  #include <linux/uuid.h>
>>  #include <linux/screen_info.h>
>>
>> @@ -622,6 +623,7 @@ void efi_native_runtime_setup(void);
>>  #define EFI_MEMORY_ATTRIBUTES_TABLE_GUID     EFI_GUID(0xdcfa911d, 0x26eb, 0x469f,  0xa2, 0x20, 0x38, 0xb7, 0xdc, 0x46, 0x12, 0x20)
>>  #define EFI_CONSOLE_OUT_DEVICE_GUID          EFI_GUID(0xd3b36f2c, 0xd551, 0x11d4,  0x9a, 0x46, 0x00, 0x90, 0x27, 0x3f, 0xc1, 0x4d)
>>  #define APPLE_PROPERTIES_PROTOCOL_GUID               EFI_GUID(0x91bd12fe, 0xf6c3, 0x44fb,  0xa5, 0xb7, 0x51, 0x22, 0xab, 0x30, 0x3a, 0xe0)
>> +#define EFI_USB_IO_PROTOCOL_GUID             EFI_GUID(0x2b2f68d6, 0x0cd2, 0x44cf,  0x8e, 0x8b, 0xbb, 0xa2, 0x0b, 0x1b, 0x5b, 0x75)
>>
>>  #define EFI_IMAGE_SECURITY_DATABASE_GUID     EFI_GUID(0xd719b2cb, 0x3d3a, 0x4596,  0xa3, 0xbc, 0xda, 0xd0, 0x0e, 0x67, 0x65, 0x6f)
>>  #define EFI_SHIM_LOCK_GUID                   EFI_GUID(0x605dab50, 0xe046, 0x4300,  0xab, 0xb6, 0x3d, 0xd8, 0x10, 0xdd, 0x8b, 0x23)
>> @@ -1569,4 +1571,68 @@ struct linux_efi_random_seed {
>>       u8      bits[];
>>  };
>>
>> +typedef enum {
>> +     EfiUsbDataIn,
>> +     EfiUsbDataOut,
>> +     EfiUsbNoData
>> +} efi_usb_data_direction_t;
>> +
>> +#define EFI_USB_NOERROR                      0x0000
>> +#define EFI_USB_ERR_NOTEXECUTE               0x0001
>> +#define EFI_USB_ERR_STALL            0x0002
>> +#define EFI_USB_ERR_BUFFER           0x0004
>> +#define EFI_USB_ERR_BABBLE           0x0008
>> +#define EFI_USB_ERR_NAK                      0x0010
>> +#define EFI_USB_ERR_CRC                      0x0020
>> +#define EFI_USB_ERR_TIMEOUT          0x0040
>> +#define EFI_USB_ERR_BITSTUFF         0x0080
>> +#define EFI_USB_ERR_SYSTEM           0x0100
>> +
>> +typedef struct {
>> +     u8      request_type;
>> +     u8      request;
>> +     u16     value;
>> +     u16     index;
>> +     u16     length;
>> +} efi_usb_device_request_t;
>
> A typedef?
>

Yes, we have plenty of those already in linux/efi.h for types defined
by the UEFI spec.

> Also, those values are little-endian, right?  And finally, they are
> crossing the user/kernel boundry, so they should be using the __ variant
> of the variable type, right?
>

These are only used in code that runs in UEFI context, i.e., before
the decompressor runs and before ExitBootServices(). So none of these
concerns apply, AFAICT.

> And finally, why does efi.h care about device specific stuff like this?
> Are you going to want to add all different types of efi devices here in
> the future?  That does not seem wise from a maintaince point-of-view...
>

No. But having the ability to talk to USB devices in the firmware
context may be useful (given patch #2), although it would be *much*
better if the firmware had its own driver for the Chaoskey (and we
implemented one as well in the Tianocore project)

Re: [RFC PATCH 1/2] efi: import USB I/O related declarations from the UEFI spec

[Greg KH]

On Sat, Sep 02, 2017 at 09:15:37AM +0100, Ard Biesheuvel wrote:
> On 2 September 2017 at 07:41, Greg KH <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org> wrote:
> > On Sat, Aug 19, 2017 at 04:17:39PM +0100, Ard Biesheuvel wrote:
> >> In preparation of adding support for the Chaoskey USB stick to the
> >> UEFI stub, import the USB I/O protocol declarations and related types
> >> to linux/efi.h.
> >>
> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> >> ---
> >>  include/linux/efi.h | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 66 insertions(+)
> >>
> >> diff --git a/include/linux/efi.h b/include/linux/efi.h
> >> index 12e05118657c..253749cd9b62 100644
> >> --- a/include/linux/efi.h
> >> +++ b/include/linux/efi.h
> >> @@ -22,6 +22,7 @@
> >>  #include <linux/pstore.h>
> >>  #include <linux/range.h>
> >>  #include <linux/reboot.h>
> >> +#include <linux/usb/ch9.h>
> >>  #include <linux/uuid.h>
> >>  #include <linux/screen_info.h>
> >>
> >> @@ -622,6 +623,7 @@ void efi_native_runtime_setup(void);
> >>  #define EFI_MEMORY_ATTRIBUTES_TABLE_GUID     EFI_GUID(0xdcfa911d, 0x26eb, 0x469f,  0xa2, 0x20, 0x38, 0xb7, 0xdc, 0x46, 0x12, 0x20)
> >>  #define EFI_CONSOLE_OUT_DEVICE_GUID          EFI_GUID(0xd3b36f2c, 0xd551, 0x11d4,  0x9a, 0x46, 0x00, 0x90, 0x27, 0x3f, 0xc1, 0x4d)
> >>  #define APPLE_PROPERTIES_PROTOCOL_GUID               EFI_GUID(0x91bd12fe, 0xf6c3, 0x44fb,  0xa5, 0xb7, 0x51, 0x22, 0xab, 0x30, 0x3a, 0xe0)
> >> +#define EFI_USB_IO_PROTOCOL_GUID             EFI_GUID(0x2b2f68d6, 0x0cd2, 0x44cf,  0x8e, 0x8b, 0xbb, 0xa2, 0x0b, 0x1b, 0x5b, 0x75)
> >>
> >>  #define EFI_IMAGE_SECURITY_DATABASE_GUID     EFI_GUID(0xd719b2cb, 0x3d3a, 0x4596,  0xa3, 0xbc, 0xda, 0xd0, 0x0e, 0x67, 0x65, 0x6f)
> >>  #define EFI_SHIM_LOCK_GUID                   EFI_GUID(0x605dab50, 0xe046, 0x4300,  0xab, 0xb6, 0x3d, 0xd8, 0x10, 0xdd, 0x8b, 0x23)
> >> @@ -1569,4 +1571,68 @@ struct linux_efi_random_seed {
> >>       u8      bits[];
> >>  };
> >>
> >> +typedef enum {
> >> +     EfiUsbDataIn,
> >> +     EfiUsbDataOut,
> >> +     EfiUsbNoData
> >> +} efi_usb_data_direction_t;
> >> +
> >> +#define EFI_USB_NOERROR                      0x0000
> >> +#define EFI_USB_ERR_NOTEXECUTE               0x0001
> >> +#define EFI_USB_ERR_STALL            0x0002
> >> +#define EFI_USB_ERR_BUFFER           0x0004
> >> +#define EFI_USB_ERR_BABBLE           0x0008
> >> +#define EFI_USB_ERR_NAK                      0x0010
> >> +#define EFI_USB_ERR_CRC                      0x0020
> >> +#define EFI_USB_ERR_TIMEOUT          0x0040
> >> +#define EFI_USB_ERR_BITSTUFF         0x0080
> >> +#define EFI_USB_ERR_SYSTEM           0x0100
> >> +
> >> +typedef struct {
> >> +     u8      request_type;
> >> +     u8      request;
> >> +     u16     value;
> >> +     u16     index;
> >> +     u16     length;
> >> +} efi_usb_device_request_t;
> >
> > A typedef?
> >
> 
> Yes, we have plenty of those already in linux/efi.h for types defined
> by the UEFI spec.

But Linux doesn't use typedefs, no need to spread horrid coding styles
of other operating systems into linux-only header files :)

> > Also, those values are little-endian, right?  And finally, they are
> > crossing the user/kernel boundry, so they should be using the __ variant
> > of the variable type, right?
> >
> 
> These are only used in code that runs in UEFI context, i.e., before
> the decompressor runs and before ExitBootServices(). So none of these
> concerns apply, AFAICT.

So why do you need to define this structure at all, if it is only used
in UEFI and not Linux?

And again, they are little endian, right?

> > And finally, why does efi.h care about device specific stuff like this?
> > Are you going to want to add all different types of efi devices here in
> > the future?  That does not seem wise from a maintaince point-of-view...
> >
> 
> No. But having the ability to talk to USB devices in the firmware
> context may be useful (given patch #2), although it would be *much*
> better if the firmware had its own driver for the Chaoskey (and we
> implemented one as well in the Tianocore project)

patch 2 doesn't use these structures, so why are you defining them?
Don't add things to the kernel that aren't used, that's just asking for
trouble.

thanks,

greg k-h

Re: [RFC PATCH 1/2] efi: import USB I/O related declarations from the UEFI spec

[Ard Biesheuvel]

On 2 September 2017 at 09:25, Greg KH <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org> wrote:
> On Sat, Sep 02, 2017 at 09:15:37AM +0100, Ard Biesheuvel wrote:
>> On 2 September 2017 at 07:41, Greg KH <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org> wrote:
>> > On Sat, Aug 19, 2017 at 04:17:39PM +0100, Ard Biesheuvel wrote:
>> >> In preparation of adding support for the Chaoskey USB stick to the
>> >> UEFI stub, import the USB I/O protocol declarations and related types
>> >> to linux/efi.h.
>> >>
>> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> >> ---
>> >>  include/linux/efi.h | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>> >>  1 file changed, 66 insertions(+)
>> >>
>> >> diff --git a/include/linux/efi.h b/include/linux/efi.h
>> >> index 12e05118657c..253749cd9b62 100644
>> >> --- a/include/linux/efi.h
>> >> +++ b/include/linux/efi.h
>> >> @@ -22,6 +22,7 @@
>> >>  #include <linux/pstore.h>
>> >>  #include <linux/range.h>
>> >>  #include <linux/reboot.h>
>> >> +#include <linux/usb/ch9.h>
>> >>  #include <linux/uuid.h>
>> >>  #include <linux/screen_info.h>
>> >>
>> >> @@ -622,6 +623,7 @@ void efi_native_runtime_setup(void);
>> >>  #define EFI_MEMORY_ATTRIBUTES_TABLE_GUID     EFI_GUID(0xdcfa911d, 0x26eb, 0x469f,  0xa2, 0x20, 0x38, 0xb7, 0xdc, 0x46, 0x12, 0x20)
>> >>  #define EFI_CONSOLE_OUT_DEVICE_GUID          EFI_GUID(0xd3b36f2c, 0xd551, 0x11d4,  0x9a, 0x46, 0x00, 0x90, 0x27, 0x3f, 0xc1, 0x4d)
>> >>  #define APPLE_PROPERTIES_PROTOCOL_GUID               EFI_GUID(0x91bd12fe, 0xf6c3, 0x44fb,  0xa5, 0xb7, 0x51, 0x22, 0xab, 0x30, 0x3a, 0xe0)
>> >> +#define EFI_USB_IO_PROTOCOL_GUID             EFI_GUID(0x2b2f68d6, 0x0cd2, 0x44cf,  0x8e, 0x8b, 0xbb, 0xa2, 0x0b, 0x1b, 0x5b, 0x75)
>> >>
>> >>  #define EFI_IMAGE_SECURITY_DATABASE_GUID     EFI_GUID(0xd719b2cb, 0x3d3a, 0x4596,  0xa3, 0xbc, 0xda, 0xd0, 0x0e, 0x67, 0x65, 0x6f)
>> >>  #define EFI_SHIM_LOCK_GUID                   EFI_GUID(0x605dab50, 0xe046, 0x4300,  0xab, 0xb6, 0x3d, 0xd8, 0x10, 0xdd, 0x8b, 0x23)
>> >> @@ -1569,4 +1571,68 @@ struct linux_efi_random_seed {
>> >>       u8      bits[];
>> >>  };
>> >>
>> >> +typedef enum {
>> >> +     EfiUsbDataIn,
>> >> +     EfiUsbDataOut,
>> >> +     EfiUsbNoData
>> >> +} efi_usb_data_direction_t;
>> >> +
>> >> +#define EFI_USB_NOERROR                      0x0000
>> >> +#define EFI_USB_ERR_NOTEXECUTE               0x0001
>> >> +#define EFI_USB_ERR_STALL            0x0002
>> >> +#define EFI_USB_ERR_BUFFER           0x0004
>> >> +#define EFI_USB_ERR_BABBLE           0x0008
>> >> +#define EFI_USB_ERR_NAK                      0x0010
>> >> +#define EFI_USB_ERR_CRC                      0x0020
>> >> +#define EFI_USB_ERR_TIMEOUT          0x0040
>> >> +#define EFI_USB_ERR_BITSTUFF         0x0080
>> >> +#define EFI_USB_ERR_SYSTEM           0x0100
>> >> +
>> >> +typedef struct {
>> >> +     u8      request_type;
>> >> +     u8      request;
>> >> +     u16     value;
>> >> +     u16     index;
>> >> +     u16     length;
>> >> +} efi_usb_device_request_t;
>> >
>> > A typedef?
>> >
>>
>> Yes, we have plenty of those already in linux/efi.h for types defined
>> by the UEFI spec.
>
> But Linux doesn't use typedefs, no need to spread horrid coding styles
> of other operating systems into linux-only header files :)
>

Well, I agree, but there is precedent in linux/efi.h to use typedefs
for this particular case, and I would prefer not to deviate from that.
I would consider a patch that converts everything to no longer use and
typedefs at all, but uniformity is more important imo.

>> > Also, those values are little-endian, right?  And finally, they are
>> > crossing the user/kernel boundry, so they should be using the __ variant
>> > of the variable type, right?
>> >
>>
>> These are only used in code that runs in UEFI context, i.e., before
>> the decompressor runs and before ExitBootServices(). So none of these
>> concerns apply, AFAICT.
>
> So why do you need to define this structure at all, if it is only used
> in UEFI and not Linux?
>
> And again, they are little endian, right?
>

They are types we need to declare in order to be able to interface
with the firmware (which is strictly little-endian). So they *are*
used in Linux, but not in the kernel proper.

>> > And finally, why does efi.h care about device specific stuff like this?
>> > Are you going to want to add all different types of efi devices here in
>> > the future?  That does not seem wise from a maintaince point-of-view...
>> >
>>
>> No. But having the ability to talk to USB devices in the firmware
>> context may be useful (given patch #2), although it would be *much*
>> better if the firmware had its own driver for the Chaoskey (and we
>> implemented one as well in the Tianocore project)
>
> patch 2 doesn't use these structures, so why are you defining them?

It does, check again

> Don't add things to the kernel that aren't used, that's just asking for
> trouble.
>
> thanks,
>
> greg k-h
> --
> To unsubscribe from this list: send the line "unsubscribe linux-efi" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html