[PATCH] efi: random: wait for CRNG to become ready before refreshing the seed

[PATCH] efi: random: wait for CRNG to become ready before refreshing the seed

[Ard Biesheuvel]

The EFI stub executes only once after boot, and kexec'd kernels reuse
the firmware context created on the first boot. This is intentional: we
preserve as much of the original firmware provided context as we can,
and pass it on unmodified, making kexec mostly idempotent.

However, there is one piece of firmware context that we should not
reuse, which is the EFI random seed, especially in cases where the
kexec'ed kernel trusts the bootloader, and we declare the CRNG ready as
soon as the firmware seed is mixed in. So in kexec capable kernels, we
refresh the EFI random seed before passing it on.

Currently, we refresh the seed without taking into account whether or
not the RNG subsystem is fully initialized, which means we may end up
passing on a seed that is weaker than desired. To avoid this, switch to
get_random_bytes_wait(), which will wait for the CRNG init to complete.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 drivers/firmware/efi/efi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 860534bcfdac..7da49c783c01 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -1035,7 +1035,7 @@ static int update_efi_random_seed(struct notifier_block *nb,
 				MEMREMAP_WB);
 		if (seed != NULL) {
 			seed->size = size;
-			get_random_bytes(seed->bits, seed->size);
+			get_random_bytes_wait(seed->bits, seed->size);
 			memunmap(seed);
 		} else {
 			pr_err("Could not map UEFI random seed!\n");
-- 
2.30.2

Re: [PATCH] efi: random: wait for CRNG to become ready before refreshing the seed

[Jason A. Donenfeld]

Hi Ard,

I'm trying to break the analysis down a bit to figure out what should be
done here. Can you tell me if any of these points are incorrect?

Case 0) EFI fails to provide any randomness at all.
        Result: nothing happens on kexec; add_bootloader_randomness() is
	used by nobody.

Case 1) EFI provides randomness. Parent kernel is trust_bootloader=y.
        Child kernel is trust_bootloader=y.
	Result: parent makes new seed for child, and everything works fine.

Case 2) EFI provides randomness. Parent kernel is trust_bootloader=y.
        Child kernel is trust_bootloader=n.
	Result: parent makes new seed for child, which child doesn't
	credit, but everything still works fine.

Case 3) EFI provides randomness. Parent kernel is trust_bootloader=n.
        Child kernel is trust_bootloader=n.
	Result: parent makes new seed for child using a technically
	uninitialized RNG that is still of EFI-quality, which child
	doesn't credit, so everything still works fine.

Case 4) EFI provides randomness. Parent kernel is trust_bootloader=n.
        Child kernel is trust_bootloader=y.
	Result: parent makes new seed for child using a technically
	uninitialized RNG that is still of EFI-quality, which child then
	credits. On the surface, this seems bad. But actually, the
	randomness here is still at least as good as what EFI provided,
	which is what trust_bootloader=y means anyway. So I think this
	case is actually fine.

Since all cases are fine, I don't think this patch is actually needed.
The interesting thing about this exercise, though, is that the thing
that enables this conclusion is the base case 0, where we notice that
kexec doesn't pass a seed if EFI isn't passing any randomness in the
first place. Were that not so, then case 4 would be dangerous.

The question I have is whether the same holds for how device tree is
doing things. There, they check rng_is_initialized(), which means the
worst is avoided, I suppose, but doing so precludes the nice properties
that EFI has for cases 3 and 4. Is there a way to do things better
there, or not really?

Jason

Re: [PATCH] efi: random: wait for CRNG to become ready before refreshing the seed

[Ard Biesheuvel]

On Thu, 9 Jun 2022 at 11:03, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> Hi Ard,
>
> I'm trying to break the analysis down a bit to figure out what should be
> done here. Can you tell me if any of these points are incorrect?
>
> Case 0) EFI fails to provide any randomness at all.
>         Result: nothing happens on kexec; add_bootloader_randomness() is
>         used by nobody.
>
> Case 1) EFI provides randomness. Parent kernel is trust_bootloader=y.
>         Child kernel is trust_bootloader=y.
>         Result: parent makes new seed for child, and everything works fine.
>
> Case 2) EFI provides randomness. Parent kernel is trust_bootloader=y.
>         Child kernel is trust_bootloader=n.
>         Result: parent makes new seed for child, which child doesn't
>         credit, but everything still works fine.
>
> Case 3) EFI provides randomness. Parent kernel is trust_bootloader=n.
>         Child kernel is trust_bootloader=n.
>         Result: parent makes new seed for child using a technically
>         uninitialized RNG that is still of EFI-quality, which child
>         doesn't credit, so everything still works fine.
>
> Case 4) EFI provides randomness. Parent kernel is trust_bootloader=n.
>         Child kernel is trust_bootloader=y.
>         Result: parent makes new seed for child using a technically
>         uninitialized RNG that is still of EFI-quality, which child then
>         credits. On the surface, this seems bad. But actually, the
>         randomness here is still at least as good as what EFI provided,
>         which is what trust_bootloader=y means anyway. So I think this
>         case is actually fine.
>
> Since all cases are fine, I don't think this patch is actually needed.
> The interesting thing about this exercise, though, is that the thing
> that enables this conclusion is the base case 0, where we notice that
> kexec doesn't pass a seed if EFI isn't passing any randomness in the
> first place. Were that not so, then case 4 would be dangerous.
>

Indeed.

> The question I have is whether the same holds for how device tree is
> doing things. There, they check rng_is_initialized(), which means the
> worst is avoided, I suppose, but doing so precludes the nice properties
> that EFI has for cases 3 and 4. Is there a way to do things better
> there, or not really?
>

I suppose we could zero the existing rng-seed property instead of
dropping it entirely, and only repopulate it if it existed in the
first place.

The only problem here is that the kernel itself is not in charge of
instantiating the original rng-seed property - it is whatever the DT
contained and DTs are often kept in files. But this just boils down to
whether or not the DT seed can be trusted to begin with.