[PATCH] efi_loader: Get rid of kaslr-seed

[Mark Kettenis]

> From: Ard Biesheuvel <ardb at kernel.org>
> Date: Thu, 16 Dec 2021 18:12:02 +0100
> 
> On Thu, 16 Dec 2021 at 17:56, Mark Kettenis <mark.kettenis at xs4all.nl> wrote:
> >
> > > From: Ard Biesheuvel <ardb at 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 at 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 at linaro.org>
> > >
> > > Acked-by: Ard Biesheuvel <ardb at 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
> > > >
> > >
>