| Message ID | 20200926085850.16342-1-ardb@kernel.org |
|---|---|
| State | Accepted |
| Commit | d32de9130f6c79533508e2c7879f18997bfbe2a0 |
| Headers | show |
| Series | efi/arm64: libstub: Deal gracefully with EFI_RNG_PROTOCOL failure | expand |
On 9/26/20 10:58 AM, Ard Biesheuvel wrote: > Currently, on arm64, we abort on any failure from efi_get_random_bytes() > other than EFI_NOT_FOUND when it comes to setting the physical seed for > KASLR, but ignore such failures when obtaining the seed for virtual > KASLR or for early seeding of the kernel's entropy pool via the config > table. This is inconsistent, and may lead to unexpected boot failures. > > So let's permit any failure for the physical seed, and simply report > the error code if it does not equal EFI_NOT_FOUND. > > Reported-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> Hello Ard, thank you for providing the patch. Unfortunately this seems not to be enough for booting with an EFI_RNG_PROTOCOL.GetRng() returning EFI_UNSUPPORTED. This is the output I received on v5.9-rc6, defconfig with your patch applied. I can retry with a branch from https://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git/ if you indicate which one to use. EFI stub: Booting Linux Kernel... EFI stub: ERROR: efi_get_random_bytes() failed (0x8000000000000003), KASLR will be disabled EFI stub: Using DTB from configuration table EFI stub: Exiting boot services and installing virtual address map... EFI stub: ERROR: Unable to construct new device tree. EFI stub: ERROR: Failed to update FDT and exit boot services This matches the problem originally reported by Scott. Best regards Heinrich > --- > drivers/firmware/efi/libstub/arm64-stub.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/firmware/efi/libstub/arm64-stub.c b/drivers/firmware/efi/libstub/arm64-stub.c > index e5bfac79e5ac..21692a986beb 100644 > --- a/drivers/firmware/efi/libstub/arm64-stub.c > +++ b/drivers/firmware/efi/libstub/arm64-stub.c > @@ -62,10 +62,10 @@ efi_status_t handle_kernel_image(unsigned long *image_addr, > status = efi_get_random_bytes(sizeof(phys_seed), > (u8 *)&phys_seed); > if (status == EFI_NOT_FOUND) { > - efi_info("EFI_RNG_PROTOCOL unavailable, no randomness supplied\n"); > + efi_info("EFI_RNG_PROTOCOL unavailable, KASLR will be disabled\n"); > } else if (status != EFI_SUCCESS) { > - efi_err("efi_get_random_bytes() failed\n"); > - return status; > + efi_err("efi_get_random_bytes() failed (0x%lx), KASLR will be disabled\n", > + status); > } > } else { > efi_info("KASLR disabled on kernel command line\n"); >
On Sun, 27 Sep 2020 at 10:18, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > On 9/26/20 10:58 AM, Ard Biesheuvel wrote: > > Currently, on arm64, we abort on any failure from efi_get_random_bytes() > > other than EFI_NOT_FOUND when it comes to setting the physical seed for > > KASLR, but ignore such failures when obtaining the seed for virtual > > KASLR or for early seeding of the kernel's entropy pool via the config > > table. This is inconsistent, and may lead to unexpected boot failures. > > > > So let's permit any failure for the physical seed, and simply report > > the error code if it does not equal EFI_NOT_FOUND. > > > > Reported-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > > Hello Ard, > > thank you for providing the patch. Unfortunately this seems not to be > enough for booting with an EFI_RNG_PROTOCOL.GetRng() returning > EFI_UNSUPPORTED. > > This is the output I received on v5.9-rc6, defconfig with your patch > applied. I can retry with a branch from > https://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git/ if you > indicate which one to use. > > EFI stub: Booting Linux Kernel... > EFI stub: ERROR: efi_get_random_bytes() failed (0x8000000000000003), > KASLR will be disabled > EFI stub: Using DTB from configuration table > > EFI stub: Exiting boot services and installing virtual address map... > > EFI stub: ERROR: Unable to construct new device tree. > EFI stub: ERROR: Failed to update FDT and exit boot services > > This matches the problem originally reported by Scott. > OK, so we need something like the below as well: --- a/drivers/firmware/efi/libstub/fdt.c +++ b/drivers/firmware/efi/libstub/fdt.c @@ -145,8 +145,6 @@ static efi_status_t update_fdt(void *orig_fdt, unsigned long orig_fdt_size, status = fdt_setprop_var(fdt, node, "kaslr-seed", fdt_val64); if (status) goto fdt_set_fail; - } else if (efi_status != EFI_NOT_FOUND) { - return efi_status; } } Could you please check whether that fixes the issue?
On 9/27/20 11:13 AM, Ard Biesheuvel wrote: > On Sun, 27 Sep 2020 at 10:18, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: >> >> On 9/26/20 10:58 AM, Ard Biesheuvel wrote: >>> Currently, on arm64, we abort on any failure from efi_get_random_bytes() >>> other than EFI_NOT_FOUND when it comes to setting the physical seed for >>> KASLR, but ignore such failures when obtaining the seed for virtual >>> KASLR or for early seeding of the kernel's entropy pool via the config >>> table. This is inconsistent, and may lead to unexpected boot failures. >>> >>> So let's permit any failure for the physical seed, and simply report >>> the error code if it does not equal EFI_NOT_FOUND. >>> >>> Reported-by: Heinrich Schuchardt <xypron.glpk@gmx.de> >>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org> >> >> Hello Ard, >> >> thank you for providing the patch. Unfortunately this seems not to be >> enough for booting with an EFI_RNG_PROTOCOL.GetRng() returning >> EFI_UNSUPPORTED. >> >> This is the output I received on v5.9-rc6, defconfig with your patch >> applied. I can retry with a branch from >> https://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git/ if you >> indicate which one to use. >> >> EFI stub: Booting Linux Kernel... >> EFI stub: ERROR: efi_get_random_bytes() failed (0x8000000000000003), >> KASLR will be disabled >> EFI stub: Using DTB from configuration table >> >> EFI stub: Exiting boot services and installing virtual address map... >> >> EFI stub: ERROR: Unable to construct new device tree. >> EFI stub: ERROR: Failed to update FDT and exit boot services >> >> This matches the problem originally reported by Scott. >> > > OK, so we need something like the below as well: > > --- a/drivers/firmware/efi/libstub/fdt.c > +++ b/drivers/firmware/efi/libstub/fdt.c > @@ -145,8 +145,6 @@ static efi_status_t update_fdt(void *orig_fdt, > unsigned long orig_fdt_size, > status = fdt_setprop_var(fdt, node, > "kaslr-seed", fdt_val64); > if (status) > goto fdt_set_fail; > - } else if (efi_status != EFI_NOT_FOUND) { > - return efi_status; > } > } > > > Could you please check whether that fixes the issue? > The new change allows booting. I could not observe the new message from the patch on my ARM64 system. The only related messages I found are: [ +0.000000] efi: EFI v2.80 by Das U-Boot [ +0.000000] efi: RTPROP=0x7aef9040 SMBIOS=0x7aef5000 MEMRESERVE=0x566df040 [ +0.000000] random: get_random_bytes called from start_kernel+0x314/0x4e8 with crng_init=0 [ +0.003506] KASLR disabled due to lack of seed Best regards Heinrich
On Sun, 27 Sep 2020 at 16:08, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > On 9/27/20 11:13 AM, Ard Biesheuvel wrote: > > On Sun, 27 Sep 2020 at 10:18, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > >> > >> On 9/26/20 10:58 AM, Ard Biesheuvel wrote: > >>> Currently, on arm64, we abort on any failure from efi_get_random_bytes() > >>> other than EFI_NOT_FOUND when it comes to setting the physical seed for > >>> KASLR, but ignore such failures when obtaining the seed for virtual > >>> KASLR or for early seeding of the kernel's entropy pool via the config > >>> table. This is inconsistent, and may lead to unexpected boot failures. > >>> > >>> So let's permit any failure for the physical seed, and simply report > >>> the error code if it does not equal EFI_NOT_FOUND. > >>> > >>> Reported-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > >>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > >> > >> Hello Ard, > >> > >> thank you for providing the patch. Unfortunately this seems not to be > >> enough for booting with an EFI_RNG_PROTOCOL.GetRng() returning > >> EFI_UNSUPPORTED. > >> > >> This is the output I received on v5.9-rc6, defconfig with your patch > >> applied. I can retry with a branch from > >> https://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git/ if you > >> indicate which one to use. > >> > >> EFI stub: Booting Linux Kernel... > >> EFI stub: ERROR: efi_get_random_bytes() failed (0x8000000000000003), > >> KASLR will be disabled > >> EFI stub: Using DTB from configuration table > >> > >> EFI stub: Exiting boot services and installing virtual address map... > >> > >> EFI stub: ERROR: Unable to construct new device tree. > >> EFI stub: ERROR: Failed to update FDT and exit boot services > >> > >> This matches the problem originally reported by Scott. > >> > > > > OK, so we need something like the below as well: > > > > --- a/drivers/firmware/efi/libstub/fdt.c > > +++ b/drivers/firmware/efi/libstub/fdt.c > > @@ -145,8 +145,6 @@ static efi_status_t update_fdt(void *orig_fdt, > > unsigned long orig_fdt_size, > > status = fdt_setprop_var(fdt, node, > > "kaslr-seed", fdt_val64); > > if (status) > > goto fdt_set_fail; > > - } else if (efi_status != EFI_NOT_FOUND) { > > - return efi_status; > > } > > } > > > > > > Could you please check whether that fixes the issue? > > > > The new change allows booting. > > I could not observe the new message from the patch on my ARM64 system. > The only related messages I found are: > > [ +0.000000] efi: EFI v2.80 by Das U-Boot > [ +0.000000] efi: RTPROP=0x7aef9040 SMBIOS=0x7aef5000 MEMRESERVE=0x566df040 > [ +0.000000] random: get_random_bytes called from > start_kernel+0x314/0x4e8 with crng_init=0 > [ +0.003506] KASLR disabled due to lack of seed > Thanks. The EFI diagnostic messages are usually written directly to the serial console - they are not captured by dmesg.
diff --git a/drivers/firmware/efi/libstub/arm64-stub.c b/drivers/firmware/efi/libstub/arm64-stub.c index e5bfac79e5ac..21692a986beb 100644 --- a/drivers/firmware/efi/libstub/arm64-stub.c +++ b/drivers/firmware/efi/libstub/arm64-stub.c @@ -62,10 +62,10 @@ efi_status_t handle_kernel_image(unsigned long *image_addr, status = efi_get_random_bytes(sizeof(phys_seed), (u8 *)&phys_seed); if (status == EFI_NOT_FOUND) { - efi_info("EFI_RNG_PROTOCOL unavailable, no randomness supplied\n"); + efi_info("EFI_RNG_PROTOCOL unavailable, KASLR will be disabled\n"); } else if (status != EFI_SUCCESS) { - efi_err("efi_get_random_bytes() failed\n"); - return status; + efi_err("efi_get_random_bytes() failed (0x%lx), KASLR will be disabled\n", + status); } } else { efi_info("KASLR disabled on kernel command line\n");
Currently, on arm64, we abort on any failure from efi_get_random_bytes() other than EFI_NOT_FOUND when it comes to setting the physical seed for KASLR, but ignore such failures when obtaining the seed for virtual KASLR or for early seeding of the kernel's entropy pool via the config table. This is inconsistent, and may lead to unexpected boot failures. So let's permit any failure for the physical seed, and simply report the error code if it does not equal EFI_NOT_FOUND. Reported-by: Heinrich Schuchardt <xypron.glpk@gmx.de> Signed-off-by: Ard Biesheuvel <ardb@kernel.org> --- drivers/firmware/efi/libstub/arm64-stub.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)