All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC PATCH] arch/x86: efistub: Invoke EFI_RNG_PROTOCOL to seed the UEFI RNG table
@ 2019-10-05 11:37 Dominik Brodowski
  2019-10-09 13:18 ` Ard Biesheuvel
  2019-10-28  7:20 ` efistub: EFI_RNG_PROTOCOL on Dell Inc. Latitude 7390 Dominik Brodowski
  0 siblings, 2 replies; 11+ messages in thread
From: Dominik Brodowski @ 2019-10-05 11:37 UTC (permalink / raw)
  To: Ard Biesheuvel, Hsin-Yi Wang, Stephen Boyd, Rob Herring,
	Theodore Ts'o, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	H. Peter Anvin, x86, torvalds
  Cc: linux-kernel, linux-efi

Implement the same mechanism for x86 efistub as introduced by commit
568bc4e87033 ("efi/arm*/libstub: Invoke EFI_RNG_PROTOCOL to seed the
UEFI RNG table") for efi/arm*/libstub, and best described here and there
as:

    Invoke the EFI_RNG_PROTOCOL protocol in the context of the stub and
    install the Linux-specific RNG seed UEFI config table. This will be
    picked up by the EFI routines in the core kernel to seed the kernel
    entropy pool.

Signed-off-by: Dominik Brodowski <linux@dominikbrodowski.net>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Hsin-Yi Wang <hsinyi@chromium.org>
Cc: Stephen Boyd <swboyd@chromium.org>
Cc: Rob Herring <robh@kernel.org>
Cc: Theodore Ts'o <tytso@mit.edu>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: <x86@kernel.org>

---

As far as I can see, we do not yet make use of the UEFI RNG on x86 at all,
but only on arm. Note that this works only when Linux is booted as an EFI
stub, and that the EFI-provided randomness is not credited as entropy
unless RANDOM_TRUST_BOOTLOADER is set _and_ another patch (on its way
upstream; thanks Ard!) is applied, see
	https://lore.kernel.org/lkml/20190928101428.GA222453@light.dominikbrodowski.net/

Further note that this patch is untested, as the firmware on my old x86
laptop only has UEFI v2.31. If you want to test it, you may wish to apply
	https://lore.kernel.org/lkml/20191005113632.GA74715@light.dominikbrodowski.net/T/#u
first to get a clear indication in dmesg.

Thanks,
	Dominik


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/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/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);
 
 /*

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [RFC PATCH] arch/x86: efistub: Invoke EFI_RNG_PROTOCOL to seed the UEFI RNG table
  2019-10-05 11:37 [RFC PATCH] arch/x86: efistub: Invoke EFI_RNG_PROTOCOL to seed the UEFI RNG table Dominik Brodowski
@ 2019-10-09 13:18 ` Ard Biesheuvel
  2019-10-28  7:20 ` efistub: EFI_RNG_PROTOCOL on Dell Inc. Latitude 7390 Dominik Brodowski
  1 sibling, 0 replies; 11+ messages in thread
From: Ard Biesheuvel @ 2019-10-09 13:18 UTC (permalink / raw)
  To: Dominik Brodowski
  Cc: Hsin-Yi Wang, Stephen Boyd, Rob Herring, Theodore Ts'o,
	Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin,
	the arch/x86 maintainers, Linus Torvalds,
	Linux Kernel Mailing List, linux-efi

On Sat, 5 Oct 2019 at 13:38, Dominik Brodowski
<linux@dominikbrodowski.net> wrote:
>
> Implement the same mechanism for x86 efistub as introduced by commit
> 568bc4e87033 ("efi/arm*/libstub: Invoke EFI_RNG_PROTOCOL to seed the
> UEFI RNG table") for efi/arm*/libstub, and best described here and there
> as:
>
>     Invoke the EFI_RNG_PROTOCOL protocol in the context of the stub and
>     install the Linux-specific RNG seed UEFI config table. This will be
>     picked up by the EFI routines in the core kernel to seed the kernel
>     entropy pool.
>
> Signed-off-by: Dominik Brodowski <linux@dominikbrodowski.net>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Hsin-Yi Wang <hsinyi@chromium.org>
> Cc: Stephen Boyd <swboyd@chromium.org>
> Cc: Rob Herring <robh@kernel.org>
> Cc: Theodore Ts'o <tytso@mit.edu>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: <x86@kernel.org>
>
> ---
>
> As far as I can see, we do not yet make use of the UEFI RNG on x86 at all,
> but only on arm.

If you have a ChaosKey, you may be able to use this driver

http://people.linaro.org/~ard.biesheuvel/ChaosKey/ChaosKeyDxe.efi

to get UEFI to expose the EFI_RNG_PROTOCOL. (Use efibootmgr -r to
install it as Driver#### or load it from the UEFI shell using 'load
xxx.efi')

> Note that this works only when Linux is booted as an EFI
> stub, and that the EFI-provided randomness is not credited as entropy
> unless RANDOM_TRUST_BOOTLOADER is set _and_ another patch (on its way
> upstream; thanks Ard!) is applied, see
>         https://lore.kernel.org/lkml/20190928101428.GA222453@light.dominikbrodowski.net/
>
> Further note that this patch is untested, as the firmware on my old x86
> laptop only has UEFI v2.31. If you want to test it, you may wish to apply
>         https://lore.kernel.org/lkml/20191005113632.GA74715@light.dominikbrodowski.net/T/#u
> first to get a clear indication in dmesg.
>


>
> 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/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/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);
>
>  /*

^ permalink raw reply	[flat|nested] 11+ messages in thread

* efistub: EFI_RNG_PROTOCOL on Dell Inc. Latitude 7390
  2019-10-05 11:37 [RFC PATCH] arch/x86: efistub: Invoke EFI_RNG_PROTOCOL to seed the UEFI RNG table Dominik Brodowski
  2019-10-09 13:18 ` Ard Biesheuvel
@ 2019-10-28  7:20 ` Dominik Brodowski
  2019-10-28  7:46   ` Ard Biesheuvel
  2019-10-31 13:33   ` Mario.Limonciello
  1 sibling, 2 replies; 11+ messages in thread
From: Dominik Brodowski @ 2019-10-28  7:20 UTC (permalink / raw)
  To: Ard Biesheuvel, mario.limonciello; +Cc: linux-efi

Ard, Mario,

in theory, invoking EFI_RNG_PROTOCOL on a Dell Inc. Latitude 7390/09386V,
BIOS 1.9.2 04/03/2019, should work fine as the system provides EFI v2.60.
Using my patch from a few weeks ago[1], efi_random_get_seed() is called from
arch/x86/boot/compressed/eboot.c::efi_main(). In efi_random_get_seed(), the
calls to

	status = efi_call_early(locate_protocol, &rng_proto, NULL,
				(void **)&rng);

and

	status = efi_call_early(allocate_pool, EFI_RUNTIME_SERVICES_DATA,
				sizeof(*seed) + EFI_RANDOM_SEED_SIZE,
				(void **)&seed);

succeed. However,

	status = rng->get_rng(rng, &rng_algo_raw, EFI_RANDOM_SEED_SIZE,
			      seed->bits);

returns EFI_INVALID_PARAMETER, though I can't see why one of these
parameters is invalid. When trying to use the default rng algorithm (by
modifying the test to "(status != EFI_SUCCESS)"),

		status = rng->get_rng(rng, NULL, EFI_RANDOM_SEED_SIZE,
				      seed->bits);

the call doesn't seem to return.

Any ideas?

Best,
	Dominik


[1] https://lore.kernel.org/lkml/20191005113753.GA77634@light.dominikbrodowski.net/

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: efistub: EFI_RNG_PROTOCOL on Dell Inc. Latitude 7390
  2019-10-28  7:20 ` efistub: EFI_RNG_PROTOCOL on Dell Inc. Latitude 7390 Dominik Brodowski
@ 2019-10-28  7:46   ` Ard Biesheuvel
  2019-10-28  8:40     ` Dominik Brodowski
  2019-10-31 13:33   ` Mario.Limonciello
  1 sibling, 1 reply; 11+ messages in thread
From: Ard Biesheuvel @ 2019-10-28  7:46 UTC (permalink / raw)
  To: Dominik Brodowski; +Cc: Mario Limonciello, linux-efi

Hello Dominik,

On Mon, 28 Oct 2019 at 08:30, Dominik Brodowski
<linux@dominikbrodowski.net> wrote:
>
> Ard, Mario,
>
> in theory, invoking EFI_RNG_PROTOCOL on a Dell Inc. Latitude 7390/09386V,
> BIOS 1.9.2 04/03/2019, should work fine as the system provides EFI v2.60.
> Using my patch from a few weeks ago[1], efi_random_get_seed() is called from
> arch/x86/boot/compressed/eboot.c::efi_main(). In efi_random_get_seed(), the
> calls to
>
>         status = efi_call_early(locate_protocol, &rng_proto, NULL,
>                                 (void **)&rng);
>
> and
>
>         status = efi_call_early(allocate_pool, EFI_RUNTIME_SERVICES_DATA,
>                                 sizeof(*seed) + EFI_RANDOM_SEED_SIZE,
>                                 (void **)&seed);
>
> succeed. However,
>
>         status = rng->get_rng(rng, &rng_algo_raw, EFI_RANDOM_SEED_SIZE,
>                               seed->bits);
>
> returns EFI_INVALID_PARAMETER, though I can't see why one of these
> parameters is invalid.

The UEFI spec defines the conditions under which this function may
return EFI_INVALID_PARAMETER as

"""
RNGValue is null or RNGValueLength is zero.

"""


> When trying to use the default rng algorithm (by
> modifying the test to "(status != EFI_SUCCESS)"),
>
>                 status = rng->get_rng(rng, NULL, EFI_RANDOM_SEED_SIZE,
>                                       seed->bits);
>
> the call doesn't seem to return.
>
> Any ideas?
>

Try running this from the UEFI shell:

http://people.linaro.org/~ard.biesheuvel/RngTest-X64.efi

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: efistub: EFI_RNG_PROTOCOL on Dell Inc. Latitude 7390
  2019-10-28  7:46   ` Ard Biesheuvel
@ 2019-10-28  8:40     ` Dominik Brodowski
  2019-10-28  8:56       ` Ard Biesheuvel
  0 siblings, 1 reply; 11+ messages in thread
From: Dominik Brodowski @ 2019-10-28  8:40 UTC (permalink / raw)
  To: Ard Biesheuvel; +Cc: Mario Limonciello, linux-efi

Hello Ard,

On Mon, Oct 28, 2019 at 08:46:28AM +0100, Ard Biesheuvel wrote:
> Hello Dominik,
> 
> On Mon, 28 Oct 2019 at 08:30, Dominik Brodowski
> <linux@dominikbrodowski.net> wrote:
> >
> > Ard, Mario,
> >
> > in theory, invoking EFI_RNG_PROTOCOL on a Dell Inc. Latitude 7390/09386V,
> > BIOS 1.9.2 04/03/2019, should work fine as the system provides EFI v2.60.
> > Using my patch from a few weeks ago[1], efi_random_get_seed() is called from
> > arch/x86/boot/compressed/eboot.c::efi_main(). In efi_random_get_seed(), the
> > calls to
> >
> >         status = efi_call_early(locate_protocol, &rng_proto, NULL,
> >                                 (void **)&rng);
> >
> > and
> >
> >         status = efi_call_early(allocate_pool, EFI_RUNTIME_SERVICES_DATA,
> >                                 sizeof(*seed) + EFI_RANDOM_SEED_SIZE,
> >                                 (void **)&seed);
> >
> > succeed. However,
> >
> >         status = rng->get_rng(rng, &rng_algo_raw, EFI_RANDOM_SEED_SIZE,
> >                               seed->bits);
> >
> > returns EFI_INVALID_PARAMETER, though I can't see why one of these
> > parameters is invalid.
> 
> The UEFI spec defines the conditions under which this function may
> return EFI_INVALID_PARAMETER as
> 
> """
> RNGValue is null or RNGValueLength is zero.
> 
> """
> 
> 
> > When trying to use the default rng algorithm (by
> > modifying the test to "(status != EFI_SUCCESS)"),
> >
> >                 status = rng->get_rng(rng, NULL, EFI_RANDOM_SEED_SIZE,
> >                                       seed->bits);
> >
> > the call doesn't seem to return.
> >
> > Any ideas?
> >
> 
> Try running this from the UEFI shell:
> 
> http://people.linaro.org/~ard.biesheuvel/RngTest-X64.efi

Interestingly, this succeeds -- with the default algorithm, SP800-90-CTR-256
and RAW. So I am more confused than before on why the call to ->get_rng()
fails in efi_random_get_seed().

Best,
	Dominik

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: efistub: EFI_RNG_PROTOCOL on Dell Inc. Latitude 7390
  2019-10-28  8:40     ` Dominik Brodowski
@ 2019-10-28  8:56       ` Ard Biesheuvel
  2019-10-28 10:51         ` Ard Biesheuvel
  2019-10-31 15:30         ` Mario.Limonciello
  0 siblings, 2 replies; 11+ messages in thread
From: Ard Biesheuvel @ 2019-10-28  8:56 UTC (permalink / raw)
  To: Dominik Brodowski; +Cc: Mario Limonciello, linux-efi

On Mon, 28 Oct 2019 at 09:42, Dominik Brodowski
<linux@dominikbrodowski.net> wrote:
>
> Hello Ard,
>
> On Mon, Oct 28, 2019 at 08:46:28AM +0100, Ard Biesheuvel wrote:
> > Hello Dominik,
> >
> > On Mon, 28 Oct 2019 at 08:30, Dominik Brodowski
> > <linux@dominikbrodowski.net> wrote:
> > >
> > > Ard, Mario,
> > >
> > > in theory, invoking EFI_RNG_PROTOCOL on a Dell Inc. Latitude 7390/09386V,
> > > BIOS 1.9.2 04/03/2019, should work fine as the system provides EFI v2.60.
> > > Using my patch from a few weeks ago[1], efi_random_get_seed() is called from
> > > arch/x86/boot/compressed/eboot.c::efi_main(). In efi_random_get_seed(), the
> > > calls to
> > >
> > >         status = efi_call_early(locate_protocol, &rng_proto, NULL,
> > >                                 (void **)&rng);
> > >
> > > and
> > >
> > >         status = efi_call_early(allocate_pool, EFI_RUNTIME_SERVICES_DATA,
> > >                                 sizeof(*seed) + EFI_RANDOM_SEED_SIZE,
> > >                                 (void **)&seed);
> > >
> > > succeed. However,
> > >
> > >         status = rng->get_rng(rng, &rng_algo_raw, EFI_RANDOM_SEED_SIZE,
> > >                               seed->bits);
> > >
> > > returns EFI_INVALID_PARAMETER, though I can't see why one of these
> > > parameters is invalid.
> >
> > The UEFI spec defines the conditions under which this function may
> > return EFI_INVALID_PARAMETER as
> >
> > """
> > RNGValue is null or RNGValueLength is zero.
> >
> > """
> >
> >
> > > When trying to use the default rng algorithm (by
> > > modifying the test to "(status != EFI_SUCCESS)"),
> > >
> > >                 status = rng->get_rng(rng, NULL, EFI_RANDOM_SEED_SIZE,
> > >                                       seed->bits);
> > >
> > > the call doesn't seem to return.
> > >
> > > Any ideas?
> > >
> >
> > Try running this from the UEFI shell:
> >
> > http://people.linaro.org/~ard.biesheuvel/RngTest-X64.efi
>
> Interestingly, this succeeds -- with the default algorithm, SP800-90-CTR-256
> and RAW. So I am more confused than before on why the call to ->get_rng()
> fails in efi_random_get_seed().
>

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.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: efistub: EFI_RNG_PROTOCOL on Dell Inc. Latitude 7390
  2019-10-28  8:56       ` Ard Biesheuvel
@ 2019-10-28 10:51         ` Ard Biesheuvel
  2019-10-28 11:47           ` Dominik Brodowski
  2019-10-31 15:30         ` Mario.Limonciello
  1 sibling, 1 reply; 11+ messages in thread
From: Ard Biesheuvel @ 2019-10-28 10:51 UTC (permalink / raw)
  To: Dominik Brodowski; +Cc: Mario Limonciello, linux-efi

On Mon, 28 Oct 2019 at 09:56, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
> On Mon, 28 Oct 2019 at 09:42, Dominik Brodowski
> <linux@dominikbrodowski.net> wrote:
> >
> > Hello Ard,
> >
> > On Mon, Oct 28, 2019 at 08:46:28AM +0100, Ard Biesheuvel wrote:
> > > Hello Dominik,
> > >
> > > On Mon, 28 Oct 2019 at 08:30, Dominik Brodowski
> > > <linux@dominikbrodowski.net> wrote:
> > > >
> > > > Ard, Mario,
> > > >
> > > > in theory, invoking EFI_RNG_PROTOCOL on a Dell Inc. Latitude 7390/09386V,
> > > > BIOS 1.9.2 04/03/2019, should work fine as the system provides EFI v2.60.
> > > > Using my patch from a few weeks ago[1], efi_random_get_seed() is called from
> > > > arch/x86/boot/compressed/eboot.c::efi_main(). In efi_random_get_seed(), the
> > > > calls to
> > > >
> > > >         status = efi_call_early(locate_protocol, &rng_proto, NULL,
> > > >                                 (void **)&rng);
> > > >
> > > > and
> > > >
> > > >         status = efi_call_early(allocate_pool, EFI_RUNTIME_SERVICES_DATA,
> > > >                                 sizeof(*seed) + EFI_RANDOM_SEED_SIZE,
> > > >                                 (void **)&seed);
> > > >
> > > > succeed. However,
> > > >
> > > >         status = rng->get_rng(rng, &rng_algo_raw, EFI_RANDOM_SEED_SIZE,
> > > >                               seed->bits);
> > > >
> > > > returns EFI_INVALID_PARAMETER, though I can't see why one of these
> > > > parameters is invalid.
> > >
> > > The UEFI spec defines the conditions under which this function may
> > > return EFI_INVALID_PARAMETER as
> > >
> > > """
> > > RNGValue is null or RNGValueLength is zero.
> > >
> > > """
> > >
> > >
> > > > When trying to use the default rng algorithm (by
> > > > modifying the test to "(status != EFI_SUCCESS)"),
> > > >
> > > >                 status = rng->get_rng(rng, NULL, EFI_RANDOM_SEED_SIZE,
> > > >                                       seed->bits);
> > > >
> > > > the call doesn't seem to return.
> > > >
> > > > Any ideas?
> > > >
> > >
> > > Try running this from the UEFI shell:
> > >
> > > http://people.linaro.org/~ard.biesheuvel/RngTest-X64.efi
> >
> > Interestingly, this succeeds -- with the default algorithm, SP800-90-CTR-256
> > and RAW. So I am more confused than before on why the call to ->get_rng()
> > fails in efi_random_get_seed().
> >
>
> 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?

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: efistub: EFI_RNG_PROTOCOL on Dell Inc. Latitude 7390
  2019-10-28 10:51         ` Ard Biesheuvel
@ 2019-10-28 11:47           ` Dominik Brodowski
  2019-10-28 12:02             ` Ard Biesheuvel
  0 siblings, 1 reply; 11+ messages in thread
From: Dominik Brodowski @ 2019-10-28 11:47 UTC (permalink / raw)
  To: Ard Biesheuvel; +Cc: Mario Limonciello, linux-efi

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?[**]

Thanks,
	Dominik


[*] 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?

[**] 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);
 
 /*

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: efistub: EFI_RNG_PROTOCOL on Dell Inc. Latitude 7390
  2019-10-28 11:47           ` Dominik Brodowski
@ 2019-10-28 12:02             ` Ard Biesheuvel
  0 siblings, 0 replies; 11+ messages in thread
From: Ard Biesheuvel @ 2019-10-28 12:02 UTC (permalink / raw)
  To: Dominik Brodowski; +Cc: Mario Limonciello, linux-efi

On Mon, 28 Oct 2019 at 12:47, Dominik Brodowski
<linux@dominikbrodowski.net> 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);
>
>  /*

^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: efistub: EFI_RNG_PROTOCOL on Dell Inc. Latitude 7390
  2019-10-28  7:20 ` efistub: EFI_RNG_PROTOCOL on Dell Inc. Latitude 7390 Dominik Brodowski
  2019-10-28  7:46   ` Ard Biesheuvel
@ 2019-10-31 13:33   ` Mario.Limonciello
  1 sibling, 0 replies; 11+ messages in thread
From: Mario.Limonciello @ 2019-10-31 13:33 UTC (permalink / raw)
  To: linux, ard.biesheuvel; +Cc: linux-efi

Dominik,

> -----Original Message-----
> From: Dominik Brodowski <linux@dominikbrodowski.net>
> Sent: Monday, October 28, 2019 2:21 AM
> To: Ard Biesheuvel; Limonciello, Mario
> Cc: linux-efi@vger.kernel.org
> Subject: efistub: EFI_RNG_PROTOCOL on Dell Inc. Latitude 7390
> 
> 
> [EXTERNAL EMAIL]
> 
> Ard, Mario,

I had some discussion to one of our BIOS guys and wanted to point a few things out.

> 
> in theory, invoking EFI_RNG_PROTOCOL on a Dell Inc. Latitude 7390/09386V,
> BIOS 1.9.2 04/03/2019, should work fine as the system provides EFI v2.60.
> Using my patch from a few weeks ago[1], efi_random_get_seed() is called
> from arch/x86/boot/compressed/eboot.c::efi_main(). In
> efi_random_get_seed(), the calls to

Just providing UEFI 2.6 doesn't guarantee an implementation of particular protocols.
the protocols are still "optional" but the spec prescribes how they should behave if
implemented.
 
> 
> 	status = efi_call_early(locate_protocol, &rng_proto, NULL,
> 				(void **)&rng);
> 
> and
> 
> 	status = efi_call_early(allocate_pool, EFI_RUNTIME_SERVICES_DATA,
> 				sizeof(*seed) + EFI_RANDOM_SEED_SIZE,
> 				(void **)&seed);
> 
> succeed. However,
> 
> 	status = rng->get_rng(rng, &rng_algo_raw,
> EFI_RANDOM_SEED_SIZE,
> 			      seed->bits);
> 
> returns EFI_INVALID_PARAMETER, though I can't see why one of these
> parameters is invalid. When trying to use the default rng algorithm (by
> modifying the test to "(status != EFI_SUCCESS)"),
> 
> 		status = rng->get_rng(rng, NULL, EFI_RANDOM_SEED_SIZE,
> 				      seed->bits);
> 
> the call doesn't seem to return.
> 
> Any ideas?

You should call RngGetInfo() to find the supported protocols instead of making
assumptions on which algorithms are supported.

On the particular platform you mentioned above, these are the only two algorithms
supported (which you of course also mentioned in a follow up).

o	  EFI_RNG_ALGORITHM_SP800_90_CTR_256_GUID
o	  EFI_RNG_ALGORITHM_RAW
> 

> 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 would agree it's unlikely to have changed in any significant way.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: efistub: EFI_RNG_PROTOCOL on Dell Inc. Latitude 7390
  2019-10-28  8:56       ` Ard Biesheuvel
  2019-10-28 10:51         ` Ard Biesheuvel
@ 2019-10-31 15:30         ` Mario.Limonciello
  1 sibling, 0 replies; 11+ messages in thread
From: Mario.Limonciello @ 2019-10-31 15:30 UTC (permalink / raw)
  To: ard.biesheuvel, linux; +Cc: linux-efi

> -----Original Message-----
> From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Sent: Monday, October 28, 2019 3:57 AM
> To: Dominik Brodowski
> Cc: Limonciello, Mario; linux-efi
> Subject: Re: efistub: EFI_RNG_PROTOCOL on Dell Inc. Latitude 7390
> 
> 
> [EXTERNAL EMAIL]
> 
> On Mon, 28 Oct 2019 at 09:42, Dominik Brodowski
> <linux@dominikbrodowski.net> wrote:
> >
> > Hello Ard,
> >
> > On Mon, Oct 28, 2019 at 08:46:28AM +0100, Ard Biesheuvel wrote:
> > > Hello Dominik,
> > >
> > > On Mon, 28 Oct 2019 at 08:30, Dominik Brodowski
> > > <linux@dominikbrodowski.net> wrote:
> > > >
> > > > Ard, Mario,
> > > >
> > > > in theory, invoking EFI_RNG_PROTOCOL on a Dell Inc. Latitude 7390/09386V,
> > > > BIOS 1.9.2 04/03/2019, should work fine as the system provides EFI v2.60.
> > > > Using my patch from a few weeks ago[1], efi_random_get_seed() is called
> from
> > > > arch/x86/boot/compressed/eboot.c::efi_main(). In
> efi_random_get_seed(), the
> > > > calls to
> > > >
> > > >         status = efi_call_early(locate_protocol, &rng_proto, NULL,
> > > >                                 (void **)&rng);
> > > >
> > > > and
> > > >
> > > >         status = efi_call_early(allocate_pool, EFI_RUNTIME_SERVICES_DATA,
> > > >                                 sizeof(*seed) + EFI_RANDOM_SEED_SIZE,
> > > >                                 (void **)&seed);
> > > >
> > > > succeed. However,
> > > >
> > > >         status = rng->get_rng(rng, &rng_algo_raw, EFI_RANDOM_SEED_SIZE,
> > > >                               seed->bits);
> > > >
> > > > returns EFI_INVALID_PARAMETER, though I can't see why one of these
> > > > parameters is invalid.
> > >
> > > The UEFI spec defines the conditions under which this function may
> > > return EFI_INVALID_PARAMETER as
> > >
> > > """
> > > RNGValue is null or RNGValueLength is zero.
> > >
> > > """
> > >
> > >
> > > > When trying to use the default rng algorithm (by
> > > > modifying the test to "(status != EFI_SUCCESS)"),
> > > >
> > > >                 status = rng->get_rng(rng, NULL, EFI_RANDOM_SEED_SIZE,
> > > >                                       seed->bits);
> > > >
> > > > the call doesn't seem to return.
> > > >
> > > > Any ideas?
> > > >
> > >
> > > Try running this from the UEFI shell:
> > >
> > > http://people.linaro.org/~ard.biesheuvel/RngTest-X64.efi
> >
> > Interestingly, this succeeds -- with the default algorithm, SP800-90-CTR-256
> > and RAW. So I am more confused than before on why the call to ->get_rng()
> > fails in efi_random_get_seed().
> >
> 
> 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 confirmed with our BIOS team for this platform that the implementation of
https://github.com/tianocore/edk2/blob/master/SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.c
matches EDK2 implementation as of commit https://github.com/tianocore/edk2/commit/289b714b77008aa4200c0be25c4b4e25df04955a

Thanks,


^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2019-10-31 15:30 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-05 11:37 [RFC PATCH] arch/x86: efistub: Invoke EFI_RNG_PROTOCOL to seed the UEFI RNG table Dominik Brodowski
2019-10-09 13:18 ` Ard Biesheuvel
2019-10-28  7:20 ` efistub: EFI_RNG_PROTOCOL on Dell Inc. Latitude 7390 Dominik Brodowski
2019-10-28  7:46   ` Ard Biesheuvel
2019-10-28  8:40     ` Dominik Brodowski
2019-10-28  8:56       ` Ard Biesheuvel
2019-10-28 10:51         ` Ard Biesheuvel
2019-10-28 11:47           ` Dominik Brodowski
2019-10-28 12:02             ` Ard Biesheuvel
2019-10-31 15:30         ` Mario.Limonciello
2019-10-31 13:33   ` Mario.Limonciello

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.