[PATCH 2/3] efi: qemu: arm64: Add efi_rng_protocol implementation for the platform
Heinrich Schuchardt
xypron.glpk at gmx.de
Wed Dec 25 09:15:49 CET 2019
On 12/25/19 7:21 AM, Sughosh Ganu wrote:
> hi Heinrich,
> Thanks for the review.
>
> On Tue, 24 Dec 2019 at 22:35, Heinrich Schuchardt <xypron.glpk at gmx.de
> <mailto:xypron.glpk at gmx.de>> wrote:
>
> 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
> <mailto: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
>
>
> Ok.
>
>
> > +{
> > + 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.
>
>
> Is the rng algorithm supported not platform specific. I believe
> different platforms might use different algorithms for providing the
> random seed.
Sure you may later want develop code implementing one of the other RNG
algorithms and than use a Kconfig setting to enable building the code
and calling it from the EFI_RNG_PROTOCOL driver. But this code will not
depend on whether you are using a specific board. A good place to put
the algorithms would be in lib/.
Have a look at EDK II's
SecurityPkg/RandomNumberGenerator/RngDxe/RdRand.c. That code does not
depend on anything from https://github.com/tianocore/edk2-platforms -
EDK II's analogue of our board/ and arch/ directories.
As platform_rng_getinfo() will only depend on Kconfig settings you can
put it into lib/efi_loader/efi_rng.c.
Best regards
Heinrich
>
>
> > +}
> > +
> > +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.
>
>
> The core implementation of efi_rng_protocol routines is indeed platform
> agnostic. I have just separated the method of getting the rng
> device(struct udevice), since different platforms might need different
> ways of getting the rng device. For example, on the qemu arm64 platform,
> the rng device is a child of a virtio device on the pci bus, while the
> rng module on the stm32mp1 platform is a direct memory mapped
> peripheral. So the mechanism used to fetch the rng udevice would be
> platform dependent. Hence the platform_get_rng_device function. In fact,
> I see this kind of method adopted in a lot many other boards, one
> example being board/CZ.NIC/turris_omnia/turris_omnia.c(get_atsha204a_dev).
>
>
> 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.
>
>
> Ok. Will do.
>
>
> > +
> > +typedef efi_guid_t efi_rng_algorithm;
>
> We want to avoid typedefs in U-Boot.
>
>
> Ok. Will replace efi_rng_algorithm with efi_guid_t
>
>
> > +
> > +struct efi_rng_protocol {
> > + efi_status_t (EFIAPI *getinfo)(struct efi_rng_protocol *this,
>
> get_info
>
>
> Ok.
>
>
> > + 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
>
>
> Ok. Will make the above suggested changes.
>
> > +};
> > +
> > +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.
>
>
> Ok.
>
>
> > + 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.
>
>
> Like I had mentioned above, could there not be a case where different
> platforms might implement different algorithms for providing the random
> seed. The uefi spec gives a list of few of the algorithms that might be
> used.
>
>
> > +
> > +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.
>
>
> Ok. Will do.
>
>
> > + 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.
>
>
> Ok. Will change. In fact, in case rng_algo is NULL, the rng_algo_size
> can be set to reflect the size of the buffer needed, and the call then
> return with EFI_BUFFER_TOO_SMALL. I guess that is what the spec requires.
>
>
> > + 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.
>
>
> Ok. I think I now understand as to why were you asking me to move
> platform_rng_get_info function into the efi_rng.c file. Instead, i will
> move the setting of the rng_algo_size to the platform code.
>
>
> If no UCLASS_RNG device is found, please, return EFI_UNSUPPORTED.
>
> Please, call EFI_EXIT here.
>
>
> Ok.
>
>
> > +
> > + 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.
>
>
> Ok. Will change.
>
>
> > +
> > + 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.
>
>
> Ok. Will change.
>
>
> > +
> > + 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.
>
>
> Ok. Will change.
>
> -sughosh
More information about the U-Boot
mailing list