On Mon, 28 Oct 2019 at 12:47, Dominik Brodowski
<linux@xxxxxxxxxxxxxxxxxxxx> wrote:
>
> On Mon, Oct 28, 2019 at 11:51:01AM +0100, Ard Biesheuvel wrote:
> > > It might be an issue with the size of the output.
> > >
> > > The original RDRAND based driver in EDK2 contains an apparent
> > > misconception that, due to the fact that certain SP800-90-CTR-256
> > > DRBGs require 32 bytes of raw entropy as a seed, it is only valid for
> > > the RAW algorithm to be called with an output size of 32.
> > >
> > > So in this case, it might be that 32 is treated as a magic number too,
> > > and any other size is rejected by the RAW algorithm.
> > >
> > > Not sure why the other one fails, though, but the fact that RAW and
> > > SP800-90-CTR-256 are the only supported algorithms suggests that your
> > > implementation is at least similar to the RDRAND based RngDxe
> > > implementation in EDK2.
> >
> > I've updated the RngTest-X64.efi binary with a version that invokes
> > both the RAW and the default algorithm twice with a request for 64
> > bytes of entropy, like we do in the kernel. It runs fine on my
> > Thinkpad, can you check whether it works on your Dell system as well?
>
> RngTest-X64.efi seems to run through fine.[*] Additionally, if I modify the
> kernel code to request only 32 bytes, it returns EFI_INVALID_PARAMETER as
> well. So something else seems to be missing... Just to verify: does
> efi_random_get_seed() work on your Thinkpad, if called from efi_main()? Have
> you tested that, by chance?[**]
>
I have just tried a v5.4-rc5 kernel built with your patch applied, and
it does not boot at all in EFI mode, without any output whatsoever.
Unfortunately, I don't have the bandwidth currently to dig into this,
but I should be able to help out a bit more next week.
>
> [*] As a sidenote: I say "seems" as the screen is immediately blanked
> once the RngTest-X64.efi binary has run through, so I have to capture the
> output with a camera. Maybe add a delay of some sort to the RngTest-X86.efi
> binary?
>
Are you running it from the UEFI shell?
> [**] patch with additional debug outputs (on top of Linus' tree) here:
>
>
> diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
> index d6662fdef300..4b909e5ab857 100644
> --- a/arch/x86/boot/compressed/eboot.c
> +++ b/arch/x86/boot/compressed/eboot.c
> @@ -781,6 +781,9 @@ efi_main(struct efi_config *c, struct boot_params *boot_params)
>
> /* Ask the firmware to clear memory on unclean shutdown */
> efi_enable_reset_attack_mitigation(sys_table);
> +
> + efi_random_get_seed(sys_table);
> +
> efi_retrieve_tpm2_eventlog(sys_table);
>
> setup_graphics(boot_params);
> diff --git a/drivers/char/random.c b/drivers/char/random.c
> index de434feb873a..45572b7e0548 100644
> --- a/drivers/char/random.c
> +++ b/drivers/char/random.c
> @@ -2515,6 +2515,14 @@ EXPORT_SYMBOL_GPL(add_hwgenerator_randomness);
> */
> void add_bootloader_randomness(const void *buf, unsigned int size)
> {
> + unsigned long *data = buf;
> +
> + pr_notice("random: adding %u bits of %sbootloader-provided randomness",
> + size * 8,
> + IS_ENABLED(CONFIG_RANDOM_TRUST_BOOTLOADER) ? "trusted " : "");
> +
> + pr_notice("random: is this random? %lx", *data);
> +
> if (IS_ENABLED(CONFIG_RANDOM_TRUST_BOOTLOADER))
> add_hwgenerator_randomness(buf, size, size * 8);
> else
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index 69f00f7453a3..e98bbf8e56d9 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -554,7 +554,7 @@ int __init efi_config_parse_tables(void *config_tables, int count, int sz,
> sizeof(*seed) + size);
> if (seed != NULL) {
> pr_notice("seeding entropy pool\n");
> - add_device_randomness(seed->bits, seed->size);
> + add_bootloader_randomness(seed->bits, seed->size);
> early_memunmap(seed, sizeof(*seed) + size);
> } else {
> pr_err("Could not map UEFI random seed!\n");
> diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
> index 0460c7581220..ece24c60fc2c 100644
> --- a/drivers/firmware/efi/libstub/Makefile
> +++ b/drivers/firmware/efi/libstub/Makefile
> @@ -38,7 +38,8 @@ OBJECT_FILES_NON_STANDARD := y
> # Prevents link failures: __sanitizer_cov_trace_pc() is not linked in.
> KCOV_INSTRUMENT := n
>
> -lib-y := efi-stub-helper.o gop.o secureboot.o tpm.o
> +lib-y := efi-stub-helper.o gop.o secureboot.o tpm.o \
> + random.o
>
> # include the stub's generic dependencies from lib/ when building for ARM/arm64
> arm-deps-y := fdt_rw.c fdt_ro.c fdt_wip.c fdt.c fdt_empty_tree.c fdt_sw.c
> @@ -47,7 +48,7 @@ arm-deps-$(CONFIG_ARM64) += sort.c
> $(obj)/lib-%.o: $(srctree)/lib/%.c FORCE
> $(call if_changed_rule,cc_o_c)
>
> -lib-$(CONFIG_EFI_ARMSTUB) += arm-stub.o fdt.o string.o random.o \
> +lib-$(CONFIG_EFI_ARMSTUB) += arm-stub.o fdt.o string.o \
> $(patsubst %.c,lib-%.o,$(arm-deps-y))
>
> lib-$(CONFIG_ARM) += arm32-stub.o
> diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
> index 7f1556fd867d..05739ae013c8 100644
> --- a/drivers/firmware/efi/libstub/efistub.h
> +++ b/drivers/firmware/efi/libstub/efistub.h
> @@ -63,8 +63,6 @@ efi_status_t efi_random_alloc(efi_system_table_t *sys_table_arg,
>
> 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);
> -
> void *get_efi_config_table(efi_system_table_t *sys_table, efi_guid_t guid);
>
> /* Helper macros for the usual case of using simple C variables: */
> diff --git a/drivers/firmware/efi/libstub/random.c b/drivers/firmware/efi/libstub/random.c
> index b4b1d1dcb5fd..70612cdd62c8 100644
> --- a/drivers/firmware/efi/libstub/random.c
> +++ b/drivers/firmware/efi/libstub/random.c
> @@ -152,33 +152,49 @@ efi_status_t efi_random_get_seed(efi_system_table_t *sys_table_arg)
>
> status = efi_call_early(locate_protocol, &rng_proto, NULL,
> (void **)&rng);
> - if (status != EFI_SUCCESS)
> + if (status != EFI_SUCCESS) {
> + pr_efi_err(sys_table_arg, "random: no protocol");
> return status;
> + }
>
> status = efi_call_early(allocate_pool, EFI_RUNTIME_SERVICES_DATA,
> sizeof(*seed) + EFI_RANDOM_SEED_SIZE,
> (void **)&seed);
> - if (status != EFI_SUCCESS)
> + if (status != EFI_SUCCESS){
> + pr_efi_err(sys_table_arg, "random: no pool");
> return status;
> + }
>
> status = rng->get_rng(rng, &rng_algo_raw, EFI_RANDOM_SEED_SIZE,
> seed->bits);
> - if (status == EFI_UNSUPPORTED)
> + if (status == EFI_DEVICE_ERROR)
> + pr_efi_err(sys_table_arg, "random: device error");
> + if (status == EFI_NOT_READY)
> + pr_efi_err(sys_table_arg, "random: not ready");
> + if (status == EFI_INVALID_PARAMETER)
> + pr_efi_err(sys_table_arg, "random: invalid parameter"); // that's the reason...
> + if (status == EFI_UNSUPPORTED) {
> /*
> * Use whatever algorithm we have available if the raw algorithm
> * is not implemented.
> */
> +
> status = rng->get_rng(rng, NULL, EFI_RANDOM_SEED_SIZE,
> seed->bits);
> + }
>
> - if (status != EFI_SUCCESS)
> + if (status != EFI_SUCCESS) {
> + pr_efi_err(sys_table_arg, "random: no bytes");
> goto err_freepool;
> + }
>
> seed->size = EFI_RANDOM_SEED_SIZE;
> status = efi_call_early(install_configuration_table, &rng_table_guid,
> seed);
> - if (status != EFI_SUCCESS)
> + if (status != EFI_SUCCESS) {
> + pr_efi_err(sys_table_arg, "random: install_configuration_table");
> goto err_freepool;
> + }
>
> return EFI_SUCCESS;
>
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index bd3837022307..a17cc5841668 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -1631,6 +1631,8 @@ static inline void
> efi_enable_reset_attack_mitigation(efi_system_table_t *sys_table_arg) { }
> #endif
>
> +efi_status_t efi_random_get_seed(efi_system_table_t *sys_table_arg);
> +
> void efi_retrieve_tpm2_eventlog(efi_system_table_t *sys_table);
>
> /*
