Message ID | 20220608153216.1480073-1-ardb@kernel.org |
---|---|
State | New |
Headers | show |
Series | efi: random: wait for CRNG to become ready before refreshing the seed | expand |
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
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.
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");
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(-)