| Message ID | 20211216145209.2426137-1-ilias.apalodimas@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | efi_loader: Get rid of kaslr-seed | expand |
On Thu, 16 Dec 2021 at 15:52, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > Right now we unconditionally pass a 'kaslr-seed' property to the kernel > if the DTB we ended up in EFI includes the entry. However the kernel > EFI stub completely ignores it and only relies on EFI_RNG_PROTOCOL. > So let's get rid of it unconditionally since it would mess up the > (upcoming) DTB TPM measuring as well. > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> Acked-by: Ard Biesheuvel <ardb@kernel.org> Note that the EFI stub itself does not consume the DTB /chosen entry for its own randomness needs (i.e., the randomization of the physical placement of the kernel, which also affects low order virtual placement [bits 16-20]), and will blindly overwrite the seed with whatever the EFI_RNG_PROTOCOL returns. > --- > cmd/bootefi.c | 2 ++ > include/efi_loader.h | 2 ++ > lib/efi_loader/efi_dt_fixup.c | 22 ++++++++++++++++++++++ > 3 files changed, 26 insertions(+) > > diff --git a/cmd/bootefi.c b/cmd/bootefi.c > index d77d3b6e943d..25f9bfce9b84 100644 > --- a/cmd/bootefi.c > +++ b/cmd/bootefi.c > @@ -310,6 +310,8 @@ efi_status_t efi_install_fdt(void *fdt) > /* Create memory reservations as indicated by the device tree */ > efi_carve_out_dt_rsv(fdt); > > + efi_purge_kaslr_seed(fdt); > + > /* Install device tree as UEFI table */ > ret = efi_install_configuration_table(&efi_guid_fdt, fdt); > if (ret != EFI_SUCCESS) { > diff --git a/include/efi_loader.h b/include/efi_loader.h > index 9dd6c2033634..e560401ac54f 100644 > --- a/include/efi_loader.h > +++ b/include/efi_loader.h > @@ -519,6 +519,8 @@ efi_status_t EFIAPI efi_convert_pointer(efi_uintn_t debug_disposition, > void **address); > /* Carve out DT reserved memory ranges */ > void efi_carve_out_dt_rsv(void *fdt); > +/* Purge unused kaslr-seed */ > +void efi_purge_kaslr_seed(void *fdt); > /* Called by bootefi to make console interface available */ > efi_status_t efi_console_register(void); > /* Called by bootefi to make all disk storage accessible as EFI objects */ > diff --git a/lib/efi_loader/efi_dt_fixup.c b/lib/efi_loader/efi_dt_fixup.c > index b6fe5d2e5a34..02f7de73872e 100644 > --- a/lib/efi_loader/efi_dt_fixup.c > +++ b/lib/efi_loader/efi_dt_fixup.c > @@ -40,6 +40,28 @@ static void efi_reserve_memory(u64 addr, u64 size, bool nomap) > addr, size); > } > > +/** > + * efi_remove_kaslr_seed() - Removed unused kaslr-seed > + * > + * Kernel's EFI STUB only relies on EFI_RNG_PROTOCOL for randomization > + * and completely ignores the kaslr-seed. Weed it out from the DTB we > + * hand over, which would mess up our DTB TPM measurements as well. > + * > + * @fdt: Pointer to device tree > + */ > +void efi_purge_kaslr_seed(void *fdt) > +{ > + int nodeoff = fdt_path_offset(fdt, "/chosen"); > + int err = 0; > + > + if (nodeoff < 0) > + return; > + > + err = fdt_delprop(fdt, nodeoff, "kaslr-seed"); > + if (err < 0) > + log_err("Error deleting kaslr-seed\n"); > +} > + > /** > * efi_carve_out_dt_rsv() - Carve out DT reserved memory ranges > * > -- > 2.30.2 >
> From: Ilias Apalodimas <ilias.apalodimas@linaro.org> > Date: Thu, 16 Dec 2021 16:52:08 +0200 > > Right now we unconditionally pass a 'kaslr-seed' property to the kernel > if the DTB we ended up in EFI includes the entry. However the kernel > EFI stub completely ignores it and only relies on EFI_RNG_PROTOCOL. > So let's get rid of it unconditionally since it would mess up the > (upcoming) DTB TPM measuring as well. NAK OpenBSD uses the kaslr-seed property in the bootloader to mix in some additional entropy. (It will also use EFI_RNG_PROTOCOL if it is avilable, but most U-Boot boards don't provide that, or at least not yet). Even on Linux the EFI stub isn't the only way to load a Linux kernel. You can use a conventional EFI bootloader like grub. Cheers, Mark > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > --- > cmd/bootefi.c | 2 ++ > include/efi_loader.h | 2 ++ > lib/efi_loader/efi_dt_fixup.c | 22 ++++++++++++++++++++++ > 3 files changed, 26 insertions(+) > > diff --git a/cmd/bootefi.c b/cmd/bootefi.c > index d77d3b6e943d..25f9bfce9b84 100644 > --- a/cmd/bootefi.c > +++ b/cmd/bootefi.c > @@ -310,6 +310,8 @@ efi_status_t efi_install_fdt(void *fdt) > /* Create memory reservations as indicated by the device tree */ > efi_carve_out_dt_rsv(fdt); > > + efi_purge_kaslr_seed(fdt); > + > /* Install device tree as UEFI table */ > ret = efi_install_configuration_table(&efi_guid_fdt, fdt); > if (ret != EFI_SUCCESS) { > diff --git a/include/efi_loader.h b/include/efi_loader.h > index 9dd6c2033634..e560401ac54f 100644 > --- a/include/efi_loader.h > +++ b/include/efi_loader.h > @@ -519,6 +519,8 @@ efi_status_t EFIAPI efi_convert_pointer(efi_uintn_t debug_disposition, > void **address); > /* Carve out DT reserved memory ranges */ > void efi_carve_out_dt_rsv(void *fdt); > +/* Purge unused kaslr-seed */ > +void efi_purge_kaslr_seed(void *fdt); > /* Called by bootefi to make console interface available */ > efi_status_t efi_console_register(void); > /* Called by bootefi to make all disk storage accessible as EFI objects */ > diff --git a/lib/efi_loader/efi_dt_fixup.c b/lib/efi_loader/efi_dt_fixup.c > index b6fe5d2e5a34..02f7de73872e 100644 > --- a/lib/efi_loader/efi_dt_fixup.c > +++ b/lib/efi_loader/efi_dt_fixup.c > @@ -40,6 +40,28 @@ static void efi_reserve_memory(u64 addr, u64 size, bool nomap) > addr, size); > } > > +/** > + * efi_remove_kaslr_seed() - Removed unused kaslr-seed > + * > + * Kernel's EFI STUB only relies on EFI_RNG_PROTOCOL for randomization > + * and completely ignores the kaslr-seed. Weed it out from the DTB we > + * hand over, which would mess up our DTB TPM measurements as well. > + * > + * @fdt: Pointer to device tree > + */ > +void efi_purge_kaslr_seed(void *fdt) > +{ > + int nodeoff = fdt_path_offset(fdt, "/chosen"); > + int err = 0; > + > + if (nodeoff < 0) > + return; > + > + err = fdt_delprop(fdt, nodeoff, "kaslr-seed"); > + if (err < 0) > + log_err("Error deleting kaslr-seed\n"); > +} > + > /** > * efi_carve_out_dt_rsv() - Carve out DT reserved memory ranges > * > -- > 2.30.2 > >
On Thu, 16 Dec 2021 at 16:25, Mark Kettenis <mark.kettenis@xs4all.nl> wrote: > > > From: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > Date: Thu, 16 Dec 2021 16:52:08 +0200 > > > > Right now we unconditionally pass a 'kaslr-seed' property to the kernel > > if the DTB we ended up in EFI includes the entry. However the kernel > > EFI stub completely ignores it and only relies on EFI_RNG_PROTOCOL. > > So let's get rid of it unconditionally since it would mess up the > > (upcoming) DTB TPM measuring as well. > > NAK > > OpenBSD uses the kaslr-seed property in the bootloader to mix in some > additional entropy. (It will also use EFI_RNG_PROTOCOL if it is > avilable, but most U-Boot boards don't provide that, or at least not > yet). > What is the point of using both the DT property and the protocol if both are available? > Even on Linux the EFI stub isn't the only way to load a Linux kernel. > You can use a conventional EFI bootloader like grub. > No, you cannot, at least not on architectures other than x86. GRUB on ARM always boots via the EFI stub.
> From: Ard Biesheuvel <ardb@kernel.org> > Date: Thu, 16 Dec 2021 16:28:06 +0100 > > On Thu, 16 Dec 2021 at 16:25, Mark Kettenis <mark.kettenis@xs4all.nl> wrote: > > > > > From: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > > Date: Thu, 16 Dec 2021 16:52:08 +0200 > > > > > > Right now we unconditionally pass a 'kaslr-seed' property to the kernel > > > if the DTB we ended up in EFI includes the entry. However the kernel > > > EFI stub completely ignores it and only relies on EFI_RNG_PROTOCOL. > > > So let's get rid of it unconditionally since it would mess up the > > > (upcoming) DTB TPM measuring as well. > > > > NAK > > > > OpenBSD uses the kaslr-seed property in the bootloader to mix in some > > additional entropy. (It will also use EFI_RNG_PROTOCOL if it is > > avilable, but most U-Boot boards don't provide that, or at least not > > yet). > > > > What is the point of using both the DT property and the protocol if > both are available? Unless kaslr-seed is coming from a different entropy source, there probably isn't a point. But it doesn't hurt and it made the bootloader code simpler. It does mean there is some room for compromise though. If U-Boot would only remove kaslr-seed if it implements EFI_RNG_PROTOCOL it wouldn't be a problem. > > Even on Linux the EFI stub isn't the only way to load a Linux kernel. > > You can use a conventional EFI bootloader like grub. > > > > No, you cannot, at least not on architectures other than x86. GRUB on > ARM always boots via the EFI stub. Ok. It isn't immediately clear from the documentation that this is the case. It would still be possible to write such a bootloader, but if it isn't a thing, it isn't a thing. But not all the world is Linux.
Hi Mark, On Thu, Dec 16, 2021 at 04:48:04PM +0100, Mark Kettenis wrote: > > From: Ard Biesheuvel <ardb@kernel.org> > > Date: Thu, 16 Dec 2021 16:28:06 +0100 > > > > On Thu, 16 Dec 2021 at 16:25, Mark Kettenis <mark.kettenis@xs4all.nl> wrote: > > > > > > > From: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > > > Date: Thu, 16 Dec 2021 16:52:08 +0200 > > > > > > > > Right now we unconditionally pass a 'kaslr-seed' property to the kernel > > > > if the DTB we ended up in EFI includes the entry. However the kernel > > > > EFI stub completely ignores it and only relies on EFI_RNG_PROTOCOL. > > > > So let's get rid of it unconditionally since it would mess up the > > > > (upcoming) DTB TPM measuring as well. > > > > > > NAK I don't understand why though. I can simply send a V2 and have a Kconfig e.g CONFIG_MEASURE_DTB, which would control stripping the property -- or we could save- > strip -> measure -> re-inject (which is a horrible idea but that's the current reality). > > > > > > OpenBSD uses the kaslr-seed property in the bootloader to mix in some > > > additional entropy. (It will also use EFI_RNG_PROTOCOL if it is > > > avilable, but most U-Boot boards don't provide that, or at least not > > > yet). > > > > > > > What is the point of using both the DT property and the protocol if > > both are available? > > Unless kaslr-seed is coming from a different entropy source, there > probably isn't a point. But it doesn't hurt and it made the > bootloader code simpler. It does hurt the measurements though. The current situation is a bit weird. Ideally the firmware would provide the DTB and I would be be able to measure prior to any fixups. However that's not the reality today. So we do have to measure just before we install the config table. Which means that all entries that can change across reboots needs to be ignored. > > It does mean there is some room for compromise though. If U-Boot > would only remove kaslr-seed if it implements EFI_RNG_PROTOCOL it > wouldn't be a problem. > > > > Even on Linux the EFI stub isn't the only way to load a Linux kernel. > > > You can use a conventional EFI bootloader like grub. > > > > > > > No, you cannot, at least not on architectures other than x86. GRUB on > > ARM always boots via the EFI stub. > > Ok. It isn't immediately clear from the documentation that this is > the case. It would still be possible to write such a bootloader, but > if it isn't a thing, it isn't a thing. But not all the world is > Linux. Yea but measuring is a reality (and a spec for all it matters). So we need to find some way of fixing this. Regards /Ilias
On 12/16/21 16:48, Mark Kettenis wrote: >> From: Ard Biesheuvel <ardb@kernel.org> >> Date: Thu, 16 Dec 2021 16:28:06 +0100 >> >> On Thu, 16 Dec 2021 at 16:25, Mark Kettenis <mark.kettenis@xs4all.nl> wrote: >>> >>>> From: Ilias Apalodimas <ilias.apalodimas@linaro.org> >>>> Date: Thu, 16 Dec 2021 16:52:08 +0200 >>>> >>>> Right now we unconditionally pass a 'kaslr-seed' property to the kernel >>>> if the DTB we ended up in EFI includes the entry. However the kernel >>>> EFI stub completely ignores it and only relies on EFI_RNG_PROTOCOL. >>>> So let's get rid of it unconditionally since it would mess up the >>>> (upcoming) DTB TPM measuring as well. >>> >>> NAK >>> >>> OpenBSD uses the kaslr-seed property in the bootloader to mix in some >>> additional entropy. (It will also use EFI_RNG_PROTOCOL if it is >>> avilable, but most U-Boot boards don't provide that, or at least not >>> yet). >>> >> >> What is the point of using both the DT property and the protocol if >> both are available? > > Unless kaslr-seed is coming from a different entropy source, there > probably isn't a point. But it doesn't hurt and it made the > bootloader code simpler. > > It does mean there is some room for compromise though. If U-Boot > would only remove kaslr-seed if it implements EFI_RNG_PROTOCOL it > wouldn't be a problem. Only QEMU's ARM virt machine fills kaslr-seed in the device-tree. On QEMU the EFI_RNG_PROTOCOL is available if you call QEMU with a virtio-rng device. All physical devices will have an EFI_RNG_PROTOCOL if there is a driver available and enabled in U-Boot. There are two platforms which in the fixup phase set kaslr-seed using an SMC call but that is after the place where Ilias's patch is removing this property. The EFI_RNG_PROTOCOL is the standardized way to provide entropy on UEFI. This will also work with ACPI based systems. So it would be advisable for OpenBSD to follow this route. Best regards Heinrich > >>> Even on Linux the EFI stub isn't the only way to load a Linux kernel. >>> You can use a conventional EFI bootloader like grub. >>> >> >> No, you cannot, at least not on architectures other than x86. GRUB on >> ARM always boots via the EFI stub. > > Ok. It isn't immediately clear from the documentation that this is > the case. It would still be possible to write such a bootloader, but > if it isn't a thing, it isn't a thing. But not all the world is > Linux.
On 12/16/21 15:52, Ilias Apalodimas wrote: > Right now we unconditionally pass a 'kaslr-seed' property to the kernel > if the DTB we ended up in EFI includes the entry. However the kernel > EFI stub completely ignores it and only relies on EFI_RNG_PROTOCOL. > So let's get rid of it unconditionally since it would mess up the > (upcoming) DTB TPM measuring as well. > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > --- > cmd/bootefi.c | 2 ++ > include/efi_loader.h | 2 ++ > lib/efi_loader/efi_dt_fixup.c | 22 ++++++++++++++++++++++ > 3 files changed, 26 insertions(+) > > diff --git a/cmd/bootefi.c b/cmd/bootefi.c > index d77d3b6e943d..25f9bfce9b84 100644 > --- a/cmd/bootefi.c > +++ b/cmd/bootefi.c > @@ -310,6 +310,8 @@ efi_status_t efi_install_fdt(void *fdt) > /* Create memory reservations as indicated by the device tree */ > efi_carve_out_dt_rsv(fdt); > > + efi_purge_kaslr_seed(fdt); > + > /* Install device tree as UEFI table */ > ret = efi_install_configuration_table(&efi_guid_fdt, fdt); > if (ret != EFI_SUCCESS) { > diff --git a/include/efi_loader.h b/include/efi_loader.h > index 9dd6c2033634..e560401ac54f 100644 > --- a/include/efi_loader.h > +++ b/include/efi_loader.h > @@ -519,6 +519,8 @@ efi_status_t EFIAPI efi_convert_pointer(efi_uintn_t debug_disposition, > void **address); > /* Carve out DT reserved memory ranges */ > void efi_carve_out_dt_rsv(void *fdt); > +/* Purge unused kaslr-seed */ > +void efi_purge_kaslr_seed(void *fdt); > /* Called by bootefi to make console interface available */ > efi_status_t efi_console_register(void); > /* Called by bootefi to make all disk storage accessible as EFI objects */ > diff --git a/lib/efi_loader/efi_dt_fixup.c b/lib/efi_loader/efi_dt_fixup.c > index b6fe5d2e5a34..02f7de73872e 100644 > --- a/lib/efi_loader/efi_dt_fixup.c > +++ b/lib/efi_loader/efi_dt_fixup.c > @@ -40,6 +40,28 @@ static void efi_reserve_memory(u64 addr, u64 size, bool nomap) > addr, size); > } > > +/** > + * efi_remove_kaslr_seed() - Removed unused kaslr-seed name mismatch > + * > + * Kernel's EFI STUB only relies on EFI_RNG_PROTOCOL for randomization > + * and completely ignores the kaslr-seed. Weed it out from the DTB we > + * hand over, which would mess up our DTB TPM measurements as well. > + * > + * @fdt: Pointer to device tree > + */ > +void efi_purge_kaslr_seed(void *fdt) > +{ > + int nodeoff = fdt_path_offset(fdt, "/chosen"); > + int err = 0; > + > + if (nodeoff < 0) > + return; > + > + err = fdt_delprop(fdt, nodeoff, "kaslr-seed"); > + if (err < 0) > + log_err("Error deleting kaslr-seed\n"); If the node does not present this is not an error! Best regards Heinrich > +} > + > /** > * efi_carve_out_dt_rsv() - Carve out DT reserved memory ranges > *
Hi Heinrich, > > @@ -40,6 +40,28 @@ static void efi_reserve_memory(u64 addr, u64 size, bool nomap) > > addr, size); > > } > > > > +/** > > + * efi_remove_kaslr_seed() - Removed unused kaslr-seed > > name mismatch > > > + * > > + * Kernel's EFI STUB only relies on EFI_RNG_PROTOCOL for randomization > > + * and completely ignores the kaslr-seed. Weed it out from the DTB we > > + * hand over, which would mess up our DTB TPM measurements as well. > > + * > > + * @fdt: Pointer to device tree > > + */ > > +void efi_purge_kaslr_seed(void *fdt) > > +{ > > + int nodeoff = fdt_path_offset(fdt, "/chosen"); > > + int err = 0; > > + > > + if (nodeoff < 0) > > + return; > > + > > + err = fdt_delprop(fdt, nodeoff, "kaslr-seed"); > > + if (err < 0) > > + log_err("Error deleting kaslr-seed\n"); > > If the node does not present this is not an error! Ah true, I'll fix that Cheers /Ilias
Hi Heinrich, On Thu, Dec 16, 2021 at 04:59:02PM +0100, Heinrich Schuchardt wrote: > On 12/16/21 16:48, Mark Kettenis wrote: > > > From: Ard Biesheuvel <ardb@kernel.org> > > > Date: Thu, 16 Dec 2021 16:28:06 +0100 > > > > > > On Thu, 16 Dec 2021 at 16:25, Mark Kettenis <mark.kettenis@xs4all.nl> wrote: > > > > > > > > > From: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > > > > Date: Thu, 16 Dec 2021 16:52:08 +0200 > > > > > > > > > > Right now we unconditionally pass a 'kaslr-seed' property to the kernel > > > > > if the DTB we ended up in EFI includes the entry. However the kernel > > > > > EFI stub completely ignores it and only relies on EFI_RNG_PROTOCOL. > > > > > So let's get rid of it unconditionally since it would mess up the > > > > > (upcoming) DTB TPM measuring as well. > > > > > > > > NAK > > > > > > > > OpenBSD uses the kaslr-seed property in the bootloader to mix in some > > > > additional entropy. (It will also use EFI_RNG_PROTOCOL if it is > > > > avilable, but most U-Boot boards don't provide that, or at least not > > > > yet). > > > > > > > > > > What is the point of using both the DT property and the protocol if > > > both are available? > > > > Unless kaslr-seed is coming from a different entropy source, there > > probably isn't a point. But it doesn't hurt and it made the > > bootloader code simpler. > > > > It does mean there is some room for compromise though. If U-Boot > > would only remove kaslr-seed if it implements EFI_RNG_PROTOCOL it > > wouldn't be a problem. > I can limit the stripping if EFI_RNG_PROTOCOL is installed or a specific Kconfig option is selected and hopefully we can get rid of the Kconfig in the future. > Only QEMU's ARM virt machine fills kaslr-seed in the device-tree. > U-Boot injects it as well in some cases e,g sec_firmware_get_random() [...] Regards /Ilias
> From: Ard Biesheuvel <ardb@kernel.org> > Date: Thu, 16 Dec 2021 15:54:55 +0100 Hi Ard, Ilias, > On Thu, 16 Dec 2021 at 15:52, Ilias Apalodimas > <ilias.apalodimas@linaro.org> wrote: > > > > Right now we unconditionally pass a 'kaslr-seed' property to the kernel > > if the DTB we ended up in EFI includes the entry. However the kernel > > EFI stub completely ignores it and only relies on EFI_RNG_PROTOCOL. > > So let's get rid of it unconditionally since it would mess up the > > (upcoming) DTB TPM measuring as well. > > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > Acked-by: Ard Biesheuvel <ardb@kernel.org> > > Note that the EFI stub itself does not consume the DTB /chosen entry > for its own randomness needs (i.e., the randomization of the physical > placement of the kernel, which also affects low order virtual > placement [bits 16-20]), and will blindly overwrite the seed with > whatever the EFI_RNG_PROTOCOL returns. But it will only do that if EFI_RNG_PROTOCOL is implemented and sucessfully returns some random data. Otherwise "kaslr-seed" will survive as-is. At least that is how I read the code in drivers/firmware/efi/libstub/fdt.c:update_fdt(). And this is good. On Apple M1 systems, the Apple bootloader actually provides a block of entropy the the kernel in their version of the device tree. The m1n1 bootloader uses this entropy to populate the kaslr-seed property in the device tree it passes on. And U-Boot is used to provide an EFI implementation such that we can boot a wide variety of OSes. At this point we don't know yet whether the SoC includes an RNG that we can use to implement EFI_RNG_PROTOCOL in U-Boot. So the effect of tis change is that a Linux kernel on this platform will run without KASLR. That doesn't seem acceptable to me. > > --- > > cmd/bootefi.c | 2 ++ > > include/efi_loader.h | 2 ++ > > lib/efi_loader/efi_dt_fixup.c | 22 ++++++++++++++++++++++ > > 3 files changed, 26 insertions(+) > > > > diff --git a/cmd/bootefi.c b/cmd/bootefi.c > > index d77d3b6e943d..25f9bfce9b84 100644 > > --- a/cmd/bootefi.c > > +++ b/cmd/bootefi.c > > @@ -310,6 +310,8 @@ efi_status_t efi_install_fdt(void *fdt) > > /* Create memory reservations as indicated by the device tree */ > > efi_carve_out_dt_rsv(fdt); > > > > + efi_purge_kaslr_seed(fdt); > > + > > /* Install device tree as UEFI table */ > > ret = efi_install_configuration_table(&efi_guid_fdt, fdt); > > if (ret != EFI_SUCCESS) { > > diff --git a/include/efi_loader.h b/include/efi_loader.h > > index 9dd6c2033634..e560401ac54f 100644 > > --- a/include/efi_loader.h > > +++ b/include/efi_loader.h > > @@ -519,6 +519,8 @@ efi_status_t EFIAPI efi_convert_pointer(efi_uintn_t debug_disposition, > > void **address); > > /* Carve out DT reserved memory ranges */ > > void efi_carve_out_dt_rsv(void *fdt); > > +/* Purge unused kaslr-seed */ > > +void efi_purge_kaslr_seed(void *fdt); > > /* Called by bootefi to make console interface available */ > > efi_status_t efi_console_register(void); > > /* Called by bootefi to make all disk storage accessible as EFI objects */ > > diff --git a/lib/efi_loader/efi_dt_fixup.c b/lib/efi_loader/efi_dt_fixup.c > > index b6fe5d2e5a34..02f7de73872e 100644 > > --- a/lib/efi_loader/efi_dt_fixup.c > > +++ b/lib/efi_loader/efi_dt_fixup.c > > @@ -40,6 +40,28 @@ static void efi_reserve_memory(u64 addr, u64 size, bool nomap) > > addr, size); > > } > > > > +/** > > + * efi_remove_kaslr_seed() - Removed unused kaslr-seed > > + * > > + * Kernel's EFI STUB only relies on EFI_RNG_PROTOCOL for randomization > > + * and completely ignores the kaslr-seed. Weed it out from the DTB we > > + * hand over, which would mess up our DTB TPM measurements as well. > > + * > > + * @fdt: Pointer to device tree > > + */ > > +void efi_purge_kaslr_seed(void *fdt) > > +{ > > + int nodeoff = fdt_path_offset(fdt, "/chosen"); > > + int err = 0; > > + > > + if (nodeoff < 0) > > + return; > > + > > + err = fdt_delprop(fdt, nodeoff, "kaslr-seed"); > > + if (err < 0) > > + log_err("Error deleting kaslr-seed\n"); > > +} > > + > > /** > > * efi_carve_out_dt_rsv() - Carve out DT reserved memory ranges > > * > > -- > > 2.30.2 > > >
On Thu, 16 Dec 2021 at 17:56, Mark Kettenis <mark.kettenis@xs4all.nl> wrote: > > > From: Ard Biesheuvel <ardb@kernel.org> > > Date: Thu, 16 Dec 2021 15:54:55 +0100 > > Hi Ard, Ilias, > > > On Thu, 16 Dec 2021 at 15:52, Ilias Apalodimas > > <ilias.apalodimas@linaro.org> wrote: > > > > > > Right now we unconditionally pass a 'kaslr-seed' property to the kernel > > > if the DTB we ended up in EFI includes the entry. However the kernel > > > EFI stub completely ignores it and only relies on EFI_RNG_PROTOCOL. > > > So let's get rid of it unconditionally since it would mess up the > > > (upcoming) DTB TPM measuring as well. > > > > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > > > Acked-by: Ard Biesheuvel <ardb@kernel.org> > > > > Note that the EFI stub itself does not consume the DTB /chosen entry > > for its own randomness needs (i.e., the randomization of the physical > > placement of the kernel, which also affects low order virtual > > placement [bits 16-20]), and will blindly overwrite the seed with > > whatever the EFI_RNG_PROTOCOL returns. > > But it will only do that if EFI_RNG_PROTOCOL is implemented and > sucessfully returns some random data. Otherwise "kaslr-seed" will > survive as-is. At least that is how I read the code in > drivers/firmware/efi/libstub/fdt.c:update_fdt(). > > And this is good. On Apple M1 systems, the Apple bootloader actually > provides a block of entropy the the kernel in their version of the > device tree. The m1n1 bootloader uses this entropy to populate the > kaslr-seed property in the device tree it passes on. And U-Boot is > used to provide an EFI implementation such that we can boot a wide > variety of OSes. At this point we don't know yet whether the SoC > includes an RNG that we can use to implement EFI_RNG_PROTOCOL in > U-Boot. > Wouldn't it be better to use this block of entropy to seed a DRBG and subsequently use that as a source of random numbers? > So the effect of tis change is that a Linux kernel on this platform > will run without KASLR. That doesn't seem acceptable to me. > I agree that this kind of regression should be avoided. But the reality is that /chosen/kaslr-seed is Linux internal ABI (EFI stub<->kernel) that somehow got promoted to boot protocol, in a way that doesn't even work correctly with the EFI stub itself, since nobody ever proposed a way to *consume* this kaslr-seed in a way that permits the EFI stub itself to perform load address randomization when EFI_RNG_PROTOCOL is absent. Note that randomization of the physical placement is important not only for physical KASLR but also for virtual KASLR in Linux. (The virtual placement modulo 2 MiB is decided by the physical placement directly) So in my opinion, this is a blatant layering violation, and EFI boot should be fixed, by implementing EFI_RNG_PROTOCOL in all cases where u-boot apparently has a source of entropy, as it is able to populate the kaslr-seed property. For non-EFI boot, the situation is obviously different, and it's perfectly fine to use /chosen/kaslr-seed directly for the the parts of KASLR that can still be used in this case. > > > --- > > > cmd/bootefi.c | 2 ++ > > > include/efi_loader.h | 2 ++ > > > lib/efi_loader/efi_dt_fixup.c | 22 ++++++++++++++++++++++ > > > 3 files changed, 26 insertions(+) > > > > > > diff --git a/cmd/bootefi.c b/cmd/bootefi.c > > > index d77d3b6e943d..25f9bfce9b84 100644 > > > --- a/cmd/bootefi.c > > > +++ b/cmd/bootefi.c > > > @@ -310,6 +310,8 @@ efi_status_t efi_install_fdt(void *fdt) > > > /* Create memory reservations as indicated by the device tree */ > > > efi_carve_out_dt_rsv(fdt); > > > > > > + efi_purge_kaslr_seed(fdt); > > > + > > > /* Install device tree as UEFI table */ > > > ret = efi_install_configuration_table(&efi_guid_fdt, fdt); > > > if (ret != EFI_SUCCESS) { > > > diff --git a/include/efi_loader.h b/include/efi_loader.h > > > index 9dd6c2033634..e560401ac54f 100644 > > > --- a/include/efi_loader.h > > > +++ b/include/efi_loader.h > > > @@ -519,6 +519,8 @@ efi_status_t EFIAPI efi_convert_pointer(efi_uintn_t debug_disposition, > > > void **address); > > > /* Carve out DT reserved memory ranges */ > > > void efi_carve_out_dt_rsv(void *fdt); > > > +/* Purge unused kaslr-seed */ > > > +void efi_purge_kaslr_seed(void *fdt); > > > /* Called by bootefi to make console interface available */ > > > efi_status_t efi_console_register(void); > > > /* Called by bootefi to make all disk storage accessible as EFI objects */ > > > diff --git a/lib/efi_loader/efi_dt_fixup.c b/lib/efi_loader/efi_dt_fixup.c > > > index b6fe5d2e5a34..02f7de73872e 100644 > > > --- a/lib/efi_loader/efi_dt_fixup.c > > > +++ b/lib/efi_loader/efi_dt_fixup.c > > > @@ -40,6 +40,28 @@ static void efi_reserve_memory(u64 addr, u64 size, bool nomap) > > > addr, size); > > > } > > > > > > +/** > > > + * efi_remove_kaslr_seed() - Removed unused kaslr-seed > > > + * > > > + * Kernel's EFI STUB only relies on EFI_RNG_PROTOCOL for randomization > > > + * and completely ignores the kaslr-seed. Weed it out from the DTB we > > > + * hand over, which would mess up our DTB TPM measurements as well. > > > + * > > > + * @fdt: Pointer to device tree > > > + */ > > > +void efi_purge_kaslr_seed(void *fdt) > > > +{ > > > + int nodeoff = fdt_path_offset(fdt, "/chosen"); > > > + int err = 0; > > > + > > > + if (nodeoff < 0) > > > + return; > > > + > > > + err = fdt_delprop(fdt, nodeoff, "kaslr-seed"); > > > + if (err < 0) > > > + log_err("Error deleting kaslr-seed\n"); > > > +} > > > + > > > /** > > > * efi_carve_out_dt_rsv() - Carve out DT reserved memory ranges > > > * > > > -- > > > 2.30.2 > > > > >
> From: Ard Biesheuvel <ardb@kernel.org> > Date: Thu, 16 Dec 2021 18:12:02 +0100 > > On Thu, 16 Dec 2021 at 17:56, Mark Kettenis <mark.kettenis@xs4all.nl> wrote: > > > > > From: Ard Biesheuvel <ardb@kernel.org> > > > Date: Thu, 16 Dec 2021 15:54:55 +0100 > > > > Hi Ard, Ilias, > > > > > On Thu, 16 Dec 2021 at 15:52, Ilias Apalodimas > > > <ilias.apalodimas@linaro.org> wrote: > > > > > > > > Right now we unconditionally pass a 'kaslr-seed' property to the kernel > > > > if the DTB we ended up in EFI includes the entry. However the kernel > > > > EFI stub completely ignores it and only relies on EFI_RNG_PROTOCOL. > > > > So let's get rid of it unconditionally since it would mess up the > > > > (upcoming) DTB TPM measuring as well. > > > > > > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > > > > > Acked-by: Ard Biesheuvel <ardb@kernel.org> > > > > > > Note that the EFI stub itself does not consume the DTB /chosen entry > > > for its own randomness needs (i.e., the randomization of the physical > > > placement of the kernel, which also affects low order virtual > > > placement [bits 16-20]), and will blindly overwrite the seed with > > > whatever the EFI_RNG_PROTOCOL returns. > > > > But it will only do that if EFI_RNG_PROTOCOL is implemented and > > sucessfully returns some random data. Otherwise "kaslr-seed" will > > survive as-is. At least that is how I read the code in > > drivers/firmware/efi/libstub/fdt.c:update_fdt(). > > > > And this is good. On Apple M1 systems, the Apple bootloader actually > > provides a block of entropy the the kernel in their version of the > > device tree. The m1n1 bootloader uses this entropy to populate the > > kaslr-seed property in the device tree it passes on. And U-Boot is > > used to provide an EFI implementation such that we can boot a wide > > variety of OSes. At this point we don't know yet whether the SoC > > includes an RNG that we can use to implement EFI_RNG_PROTOCOL in > > U-Boot. > > > > Wouldn't it be better to use this block of entropy to seed a DRBG and > subsequently use that as a source of random numbers? Hmm, I didn't consider that as an option. We actually get 512 bits of entropy from m1n1, which should be good enough to seed a DRBG isn't it? Not really my area of expertise though. So I'll need some help here. > > So the effect of tis change is that a Linux kernel on this platform > > will run without KASLR. That doesn't seem acceptable to me. > > > > I agree that this kind of regression should be avoided. But the > reality is that /chosen/kaslr-seed is Linux internal ABI (EFI > stub<->kernel) that somehow got promoted to boot protocol, in a way > that doesn't even work correctly with the EFI stub itself, since > nobody ever proposed a way to *consume* this kaslr-seed in a way that > permits the EFI stub itself to perform load address randomization when > EFI_RNG_PROTOCOL is absent. Note that randomization of the physical > placement is important not only for physical KASLR but also for > virtual KASLR in Linux. (The virtual placement modulo 2 MiB is decided > by the physical placement directly) > > So in my opinion, this is a blatant layering violation, and EFI boot > should be fixed, by implementing EFI_RNG_PROTOCOL in all cases where > u-boot apparently has a source of entropy, as it is able to populate > the kaslr-seed property. > > For non-EFI boot, the situation is obviously different, and it's > perfectly fine to use /chosen/kaslr-seed directly for the the parts of > KASLR that can still be used in this case. > > > > > > > --- > > > > cmd/bootefi.c | 2 ++ > > > > include/efi_loader.h | 2 ++ > > > > lib/efi_loader/efi_dt_fixup.c | 22 ++++++++++++++++++++++ > > > > 3 files changed, 26 insertions(+) > > > > > > > > diff --git a/cmd/bootefi.c b/cmd/bootefi.c > > > > index d77d3b6e943d..25f9bfce9b84 100644 > > > > --- a/cmd/bootefi.c > > > > +++ b/cmd/bootefi.c > > > > @@ -310,6 +310,8 @@ efi_status_t efi_install_fdt(void *fdt) > > > > /* Create memory reservations as indicated by the device tree */ > > > > efi_carve_out_dt_rsv(fdt); > > > > > > > > + efi_purge_kaslr_seed(fdt); > > > > + > > > > /* Install device tree as UEFI table */ > > > > ret = efi_install_configuration_table(&efi_guid_fdt, fdt); > > > > if (ret != EFI_SUCCESS) { > > > > diff --git a/include/efi_loader.h b/include/efi_loader.h > > > > index 9dd6c2033634..e560401ac54f 100644 > > > > --- a/include/efi_loader.h > > > > +++ b/include/efi_loader.h > > > > @@ -519,6 +519,8 @@ efi_status_t EFIAPI efi_convert_pointer(efi_uintn_t debug_disposition, > > > > void **address); > > > > /* Carve out DT reserved memory ranges */ > > > > void efi_carve_out_dt_rsv(void *fdt); > > > > +/* Purge unused kaslr-seed */ > > > > +void efi_purge_kaslr_seed(void *fdt); > > > > /* Called by bootefi to make console interface available */ > > > > efi_status_t efi_console_register(void); > > > > /* Called by bootefi to make all disk storage accessible as EFI objects */ > > > > diff --git a/lib/efi_loader/efi_dt_fixup.c b/lib/efi_loader/efi_dt_fixup.c > > > > index b6fe5d2e5a34..02f7de73872e 100644 > > > > --- a/lib/efi_loader/efi_dt_fixup.c > > > > +++ b/lib/efi_loader/efi_dt_fixup.c > > > > @@ -40,6 +40,28 @@ static void efi_reserve_memory(u64 addr, u64 size, bool nomap) > > > > addr, size); > > > > } > > > > > > > > +/** > > > > + * efi_remove_kaslr_seed() - Removed unused kaslr-seed > > > > + * > > > > + * Kernel's EFI STUB only relies on EFI_RNG_PROTOCOL for randomization > > > > + * and completely ignores the kaslr-seed. Weed it out from the DTB we > > > > + * hand over, which would mess up our DTB TPM measurements as well. > > > > + * > > > > + * @fdt: Pointer to device tree > > > > + */ > > > > +void efi_purge_kaslr_seed(void *fdt) > > > > +{ > > > > + int nodeoff = fdt_path_offset(fdt, "/chosen"); > > > > + int err = 0; > > > > + > > > > + if (nodeoff < 0) > > > > + return; > > > > + > > > > + err = fdt_delprop(fdt, nodeoff, "kaslr-seed"); > > > > + if (err < 0) > > > > + log_err("Error deleting kaslr-seed\n"); > > > > +} > > > > + > > > > /** > > > > * efi_carve_out_dt_rsv() - Carve out DT reserved memory ranges > > > > * > > > > -- > > > > 2.30.2 > > > > > > > >
On Thu, 16 Dec 2021 at 18:55, Mark Kettenis <mark.kettenis@xs4all.nl> wrote: > > > From: Ard Biesheuvel <ardb@kernel.org> > > Date: Thu, 16 Dec 2021 18:12:02 +0100 > > > > On Thu, 16 Dec 2021 at 17:56, Mark Kettenis <mark.kettenis@xs4all.nl> wrote: > > > > > > > From: Ard Biesheuvel <ardb@kernel.org> > > > > Date: Thu, 16 Dec 2021 15:54:55 +0100 > > > > > > Hi Ard, Ilias, > > > > > > > On Thu, 16 Dec 2021 at 15:52, Ilias Apalodimas > > > > <ilias.apalodimas@linaro.org> wrote: > > > > > > > > > > Right now we unconditionally pass a 'kaslr-seed' property to the kernel > > > > > if the DTB we ended up in EFI includes the entry. However the kernel > > > > > EFI stub completely ignores it and only relies on EFI_RNG_PROTOCOL. > > > > > So let's get rid of it unconditionally since it would mess up the > > > > > (upcoming) DTB TPM measuring as well. > > > > > > > > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > > > > > > > Acked-by: Ard Biesheuvel <ardb@kernel.org> > > > > > > > > Note that the EFI stub itself does not consume the DTB /chosen entry > > > > for its own randomness needs (i.e., the randomization of the physical > > > > placement of the kernel, which also affects low order virtual > > > > placement [bits 16-20]), and will blindly overwrite the seed with > > > > whatever the EFI_RNG_PROTOCOL returns. > > > > > > But it will only do that if EFI_RNG_PROTOCOL is implemented and > > > sucessfully returns some random data. Otherwise "kaslr-seed" will > > > survive as-is. At least that is how I read the code in > > > drivers/firmware/efi/libstub/fdt.c:update_fdt(). > > > > > > And this is good. On Apple M1 systems, the Apple bootloader actually > > > provides a block of entropy the the kernel in their version of the > > > device tree. The m1n1 bootloader uses this entropy to populate the > > > kaslr-seed property in the device tree it passes on. And U-Boot is > > > used to provide an EFI implementation such that we can boot a wide > > > variety of OSes. At this point we don't know yet whether the SoC > > > includes an RNG that we can use to implement EFI_RNG_PROTOCOL in > > > U-Boot. > > > > > > > Wouldn't it be better to use this block of entropy to seed a DRBG and > > subsequently use that as a source of random numbers? > > Hmm, I didn't consider that as an option. We actually get 512 bits of > entropy from m1n1, which should be good enough to seed a DRBG isn't > it? Not really my area of expertise though. So I'll need some help > here. > Yes, all the DRBGs defined by NIST SP800-90A take a seed in the order of 300 - 500 bits IIRC. On an arm64 system that implements the crypto extensions, stringing together a couple of AES instructions to implement the AES-CTR of flavor of DRBG should be rather straight-forward, and tiny in terms of code size. Of course, reusing an existing implementation would be even better but I don't know from the top of my head if anything suitable exists under an appropriate license that we can just drop in.
Hi, On Thu, 16 Dec 2021 at 21:00, Ard Biesheuvel <ardb@kernel.org> wrote: > > On Thu, 16 Dec 2021 at 18:55, Mark Kettenis <mark.kettenis@xs4all.nl> wrote: > > > > > From: Ard Biesheuvel <ardb@kernel.org> > > > Date: Thu, 16 Dec 2021 18:12:02 +0100 > > > > > > On Thu, 16 Dec 2021 at 17:56, Mark Kettenis <mark.kettenis@xs4all.nl> wrote: > > > > > > > > > From: Ard Biesheuvel <ardb@kernel.org> > > > > > Date: Thu, 16 Dec 2021 15:54:55 +0100 > > > > > > > > Hi Ard, Ilias, > > > > > > > > > On Thu, 16 Dec 2021 at 15:52, Ilias Apalodimas > > > > > <ilias.apalodimas@linaro.org> wrote: > > > > > > > > > > > > Right now we unconditionally pass a 'kaslr-seed' property to the kernel > > > > > > if the DTB we ended up in EFI includes the entry. However the kernel > > > > > > EFI stub completely ignores it and only relies on EFI_RNG_PROTOCOL. > > > > > > So let's get rid of it unconditionally since it would mess up the > > > > > > (upcoming) DTB TPM measuring as well. > > > > > > > > > > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > > > > > > > > > Acked-by: Ard Biesheuvel <ardb@kernel.org> > > > > > > > > > > Note that the EFI stub itself does not consume the DTB /chosen entry > > > > > for its own randomness needs (i.e., the randomization of the physical > > > > > placement of the kernel, which also affects low order virtual > > > > > placement [bits 16-20]), and will blindly overwrite the seed with > > > > > whatever the EFI_RNG_PROTOCOL returns. > > > > > > > > But it will only do that if EFI_RNG_PROTOCOL is implemented and > > > > sucessfully returns some random data. Otherwise "kaslr-seed" will > > > > survive as-is. At least that is how I read the code in > > > > drivers/firmware/efi/libstub/fdt.c:update_fdt(). > > > > > > > > And this is good. On Apple M1 systems, the Apple bootloader actually > > > > provides a block of entropy the the kernel in their version of the > > > > device tree. The m1n1 bootloader uses this entropy to populate the > > > > kaslr-seed property in the device tree it passes on. And U-Boot is > > > > used to provide an EFI implementation such that we can boot a wide > > > > variety of OSes. At this point we don't know yet whether the SoC > > > > includes an RNG that we can use to implement EFI_RNG_PROTOCOL in > > > > U-Boot. > > > > > > > > > > Wouldn't it be better to use this block of entropy to seed a DRBG and > > > subsequently use that as a source of random numbers? > > > > Hmm, I didn't consider that as an option. We actually get 512 bits of > > entropy from m1n1, which should be good enough to seed a DRBG isn't > > it? Not really my area of expertise though. So I'll need some help > > here. > > > > Yes, all the DRBGs defined by NIST SP800-90A take a seed in the order > of 300 - 500 bits IIRC. > > On an arm64 system that implements the crypto extensions, stringing > together a couple of AES instructions to implement the AES-CTR of > flavor of DRBG should be rather straight-forward, and tiny in terms of > code size. > > Of course, reusing an existing implementation would be even better but > I don't know from the top of my head if anything suitable exists under > an appropriate license that we can just drop in. Right here's an idea. The main problem I had here was measuring the DTB. But measuring means we have a TPM and if we have a TPM we have an RNG. So what we could do, is support EFI_RNG_PROTOCOL whenever a TPM is there. For those boards I can unconditionally weed out the kaslr-seed and everyone will be happy. It won't solve the problem of ASLR when booting via EFI and a kaslr-seed persists, but we can always fix that later. Cheers /Ilias
diff --git a/cmd/bootefi.c b/cmd/bootefi.c index d77d3b6e943d..25f9bfce9b84 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -310,6 +310,8 @@ efi_status_t efi_install_fdt(void *fdt) /* Create memory reservations as indicated by the device tree */ efi_carve_out_dt_rsv(fdt); + efi_purge_kaslr_seed(fdt); + /* Install device tree as UEFI table */ ret = efi_install_configuration_table(&efi_guid_fdt, fdt); if (ret != EFI_SUCCESS) { diff --git a/include/efi_loader.h b/include/efi_loader.h index 9dd6c2033634..e560401ac54f 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -519,6 +519,8 @@ efi_status_t EFIAPI efi_convert_pointer(efi_uintn_t debug_disposition, void **address); /* Carve out DT reserved memory ranges */ void efi_carve_out_dt_rsv(void *fdt); +/* Purge unused kaslr-seed */ +void efi_purge_kaslr_seed(void *fdt); /* Called by bootefi to make console interface available */ efi_status_t efi_console_register(void); /* Called by bootefi to make all disk storage accessible as EFI objects */ diff --git a/lib/efi_loader/efi_dt_fixup.c b/lib/efi_loader/efi_dt_fixup.c index b6fe5d2e5a34..02f7de73872e 100644 --- a/lib/efi_loader/efi_dt_fixup.c +++ b/lib/efi_loader/efi_dt_fixup.c @@ -40,6 +40,28 @@ static void efi_reserve_memory(u64 addr, u64 size, bool nomap) addr, size); } +/** + * efi_remove_kaslr_seed() - Removed unused kaslr-seed + * + * Kernel's EFI STUB only relies on EFI_RNG_PROTOCOL for randomization + * and completely ignores the kaslr-seed. Weed it out from the DTB we + * hand over, which would mess up our DTB TPM measurements as well. + * + * @fdt: Pointer to device tree + */ +void efi_purge_kaslr_seed(void *fdt) +{ + int nodeoff = fdt_path_offset(fdt, "/chosen"); + int err = 0; + + if (nodeoff < 0) + return; + + err = fdt_delprop(fdt, nodeoff, "kaslr-seed"); + if (err < 0) + log_err("Error deleting kaslr-seed\n"); +} + /** * efi_carve_out_dt_rsv() - Carve out DT reserved memory ranges *
Right now we unconditionally pass a 'kaslr-seed' property to the kernel if the DTB we ended up in EFI includes the entry. However the kernel EFI stub completely ignores it and only relies on EFI_RNG_PROTOCOL. So let's get rid of it unconditionally since it would mess up the (upcoming) DTB TPM measuring as well. Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> --- cmd/bootefi.c | 2 ++ include/efi_loader.h | 2 ++ lib/efi_loader/efi_dt_fixup.c | 22 ++++++++++++++++++++++ 3 files changed, 26 insertions(+)