[PATCH 2/3] efi: qemu: arm64: Add efi_rng_protocol implementation for the platform
Heinrich Schuchardt
xypron.glpk at gmx.de
Tue Dec 24 18:05:32 CET 2019
On 12/24/19 4:54 PM, Sughosh Ganu wrote:
> Add support for the EFI_RNG_PROTOCOL routines for the qemu arm64
> platform. EFI_RNG_PROTOCOL is an uefi boottime service which is
> invoked by the efi stub in the kernel for getting random seed for
> kaslr.
>
> The routines are platform specific, and use the virtio-rng device on
> the platform to get random data.
>
> The feature can be enabled through the following config
> CONFIG_EFI_RNG_PROTOCOL
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu at linaro.org>
> ---
> board/emulation/qemu-arm/qemu-arm.c | 50 +++++++++++++++++++++++++
> include/efi_rng.h | 34 +++++++++++++++++
> lib/efi_loader/Kconfig | 8 ++++
> lib/efi_loader/Makefile | 1 +
> lib/efi_loader/efi_rng.c | 74 +++++++++++++++++++++++++++++++++++++
> 5 files changed, 167 insertions(+)
> create mode 100644 include/efi_rng.h
> create mode 100644 lib/efi_loader/efi_rng.c
>
> diff --git a/board/emulation/qemu-arm/qemu-arm.c b/board/emulation/qemu-arm/qemu-arm.c
> index e1f4709..3176421 100644
> --- a/board/emulation/qemu-arm/qemu-arm.c
> +++ b/board/emulation/qemu-arm/qemu-arm.c
> @@ -91,3 +91,53 @@ void *board_fdt_blob_setup(void)
> /* QEMU loads a generated DTB for us at the start of RAM. */
> return (void *)CONFIG_SYS_SDRAM_BASE;
> }
> +
> +#if defined(CONFIG_EFI_RNG_PROTOCOL)
> +#include <efi_loader.h>
> +#include <efi_rng.h>
> +
> +#include <dm/device-internal.h>
> +
> +#define VIRTIO_RNG_PCI_DEVICE "virtio-pci.l#0"
> +
> +void platform_rng_getinfo(efi_rng_algorithm *rng_algo)
Thanks for working on the implementation of the EFI_RNG_PROTOCOL.
Please, put an underscore after each word: platform_rng_get_info
> +{
> + const efi_guid_t rng_raw_guid = EFI_RNG_ALGORITHM_RAW;
> +
> + guidcpy(rng_algo, &rng_raw_guid);
This function should be in efi_rng.c if it is needed at all.
> +}
> +
> +efi_status_t platform_get_rng_device(struct udevice **dev)
> +{
Here you are creating platform specific code. The idea of the driver
model in U-Boot is to separate duties.
So the implementation of the EFI_RNG_PROTOCOL should be platform
agnostic and rely simply on looping of the devices of UCLASS_RNG.
Please, move function platform_get_rng_device into the RNG uclass.
> + int ret;
> + efi_status_t status = EFI_DEVICE_ERROR;
> + struct udevice *bus, *devp;
> +
> + for (uclass_first_device(UCLASS_VIRTIO, &bus); bus;
> + uclass_next_device(&bus)) {
> + for (device_find_first_child(bus, &devp); devp; device_find_next_child(&devp)) {
> + if (device_get_uclass_id(devp) == UCLASS_RNG) {
> + *dev = devp;
> + status = EFI_SUCCESS;
> + break;
> + }
> + }
> + }
> +
> + if (status != EFI_SUCCESS) {
> + debug("No rng device found\n");
> + return EFI_DEVICE_ERROR;
> + }
> +
> + if (*dev) {
> + ret = device_probe(*dev);
> + if (ret)
> + return EFI_DEVICE_ERROR;
> + } else {
> + debug("Couldn't get child device\n");
> + return EFI_DEVICE_ERROR;
> + }
> +
> + return EFI_SUCCESS;
> +}
> +#endif /* CONFIG_EFI_RNG_PROTOCOL */
> diff --git a/include/efi_rng.h b/include/efi_rng.h
> new file mode 100644
> index 0000000..df749dd
> --- /dev/null
> +++ b/include/efi_rng.h
> @@ -0,0 +1,34 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (c) 2019, Linaro Limited
> + */
> +
> +#if !defined _EFI_RNG_H_
> +#define _EFI_RNG_H_
> +
> +#include <efi.h>
> +#include <efi_api.h>
> +
> +#define EFI_RNG_PROTOCOL_GUID \
> + EFI_GUID(0x3152bca5, 0xeade, 0x433d, 0x86, 0x2e, \
> + 0xc0, 0x1c, 0xdc, 0x29, 0x1f, 0x44)
> +
> +#define EFI_RNG_ALGORITHM_RAW \
> + EFI_GUID(0xe43176d7, 0xb6e8, 0x4827, 0xb7, 0x84, \
> + 0x7f, 0xfd, 0xc4, 0xb6, 0x85, 0x61)
Please, add a comment line for each of the GUIDs.
> +
> +typedef efi_guid_t efi_rng_algorithm;
We want to avoid typedefs in U-Boot.
> +
> +struct efi_rng_protocol {
> + efi_status_t (EFIAPI *getinfo)(struct efi_rng_protocol *this,
get_info
> + efi_uintn_t *rng_algo_size,
Please, use the parameter names from the UEFI 2.8 specification:
rng_algorithm_list_size
> + efi_rng_algorithm *rng_algo);
rng_algorithm_list
> + efi_status_t (EFIAPI *getrng)(struct efi_rng_protocol *this,
get_rng
> + efi_rng_algorithm *rng_algo,
efi_guid_t *rng_algorithm
> + efi_uintn_t rng_len, uint8_t *rng_data);
rng_value_length, rng_value
> +};
> +
> +void platform_rng_getinfo(efi_rng_algorithm *rng_algo);
> +efi_status_t platform_get_rng_device(struct udevice **dev);
> +
> +#endif /* _EFI_RNG_H_ */
> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> index 21ef440..7437442 100644
> --- a/lib/efi_loader/Kconfig
> +++ b/lib/efi_loader/Kconfig
> @@ -120,4 +120,12 @@ config EFI_GRUB_ARM32_WORKAROUND
> GRUB prior to version 2.04 requires U-Boot to disable caches. This
> workaround currently is also needed on systems with caches that
> cannot be managed via CP15.
> +
> +config EFI_RNG_PROTOCOL
> + bool "EFI_RNG_PROTOCOL support"
> + depends on DM_RNG && TARGET_QEMU_ARM_64BIT
DM_RNG should be enough.
Just return EFI_UNSUPPORTED from EFI_RNG_PROTOCOL if no device of the
RNG uclass is found.
> + help
> + "Support for EFI_RNG_PROTOCOL implementation. Uses the rng
> + device on the platform"
> +
> endif
> diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
> index 7db4060..04dc864 100644
> --- a/lib/efi_loader/Makefile
> +++ b/lib/efi_loader/Makefile
> @@ -42,3 +42,4 @@ obj-$(CONFIG_PARTITIONS) += efi_disk.o
> obj-$(CONFIG_NET) += efi_net.o
> obj-$(CONFIG_GENERATE_ACPI_TABLE) += efi_acpi.o
> obj-$(CONFIG_GENERATE_SMBIOS_TABLE) += efi_smbios.o
> +obj-$(CONFIG_EFI_RNG_PROTOCOL) += efi_rng.o
> diff --git a/lib/efi_loader/efi_rng.c b/lib/efi_loader/efi_rng.c
> new file mode 100644
> index 0000000..8456592
> --- /dev/null
> +++ b/lib/efi_loader/efi_rng.c
> @@ -0,0 +1,74 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (c) 2019, Linaro Limited
> + */
> +
> +#include <common.h>
> +#include <dm.h>
> +#include <efi_loader.h>
> +#include <efi_rng.h>
> +#include <rng.h>
> +
> +DECLARE_GLOBAL_DATA_PTR;
Please, add platform_rng_get_info() in this file.
> +
> +static efi_status_t EFIAPI rng_getinfo(struct efi_rng_protocol *this,
> + efi_uintn_t *rng_algo_size,
> + efi_rng_algorithm *rng_algo)
> +{
Please, call EFI_ENTRY here.
> + if (!this || !rng_algo_size || !rng_algo)
Please, allow RNGAlgorithmList == NULL here. The caller will typically
call this function twice. The first call will inform the caller about
the size of the buffer needed. The caller then allocates the buffer and
calls the function again.
> + return EFI_INVALID_PARAMETER;
> +
> + if (*rng_algo_size < sizeof(*rng_algo)) {
> + *rng_algo_size = sizeof(*rng_algo);
> + return EFI_BUFFER_TOO_SMALL;
> + }
> +
> + *rng_algo_size = sizeof(*rng_algo);
> + platform_rng_getinfo(rng_algo);
Having a platform_rng_get_info() function does not make much sense if we
hard code the the size of the algorithm list above and the allowed
algorithm below. So you can directly put your memcpy() here.
If no UCLASS_RNG device is found, please, return EFI_UNSUPPORTED.
Please, call EFI_EXIT here.
> +
> + return EFI_SUCCESS;
> +}
> +
> +static efi_status_t EFIAPI getrng(struct efi_rng_protocol *protocol,
> + efi_rng_algorithm *rng_algo,
> + efi_uintn_t rng_len, uint8_t *rng_data)
> +{
> + int ret;
> + struct udevice *dev;
> + const efi_guid_t rng_raw_guid = EFI_RNG_ALGORITHM_RAW;
> +
> + /*
> + * Booting into the linux kernel mangles
> + * x18, which holds the gd.
> + * When the efi stub in the kernel invokes
> + * the GetRng routine, gd needs to be
> + * restored back, without which bad things
> + * happen
> + */
> + efi_restore_gd();
Please, use EFI_ENTRY() instead.
> +
> + if (!protocol || !rng_data || !rng_len)
> + return EFI_INVALID_PARAMETER;
> +
> + if (rng_algo) {
> + if (guidcmp(rng_algo, &rng_raw_guid))
> + return EFI_UNSUPPORTED;
> + }
> +
> + ret = platform_get_rng_device(&dev);
> + if (ret != EFI_SUCCESS)
> + return EFI_DEVICE_ERROR;
I think this should be EFI_UNSUPPORTED.
> +
> + ret = dm_rng_read(dev, rng_data, rng_len);
> + if (ret < 0) {
> + debug("Rng device read failed\n");
> + return EFI_DEVICE_ERROR;
> + }
> +
You have to call EFI_EXIT(EFI_SUCCESS) here.
Best regards
Heinrich
> + return EFI_SUCCESS;
> +}
> +
> +const struct efi_rng_protocol efi_rng_protocol_ops = {
> + .getinfo = rng_getinfo,
> + .getrng = getrng,
> +};
>
More information about the U-Boot
mailing list