Re: [PATCH] efi: x86: Wipe setup_data on pure EFI boot

Ard Biesheuvel
2022-09-05T09:59:59Z

On Mon, 5 Sept 2022 at 11:57, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> On Mon, Sep 5, 2022 at 11:55 AM Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Sun, 4 Sept 2022 at 18:53, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> > >
> > > From: Ard Biesheuvel <ardb@kernel.org>
> > >
> > > When booting the x86 kernel via EFI using the LoadImage/StartImage boot
> > > services [as opposed to the deprecated EFI handover protocol], the setup
> > > header is taken from the image directly, and given that EFI's LoadImage
> > > has no Linux/x86 specific knowledge regarding struct bootparams or
> > > struct setup_header, any absolute addresses in the setup header must
> > > originate from the file and not from a prior loading stage.
> > >
> > > Since we cannot generally predict where LoadImage() decides to load an
> > > image (*), such absolute addresses must be treated as suspect: even if a
> > > prior boot stage intended to make them point somewhere inside the
> > > [signed] image, there is no way to validate that, and if they point at
> > > an arbitrary location in memory, the setup_data nodes will not be
> > > covered by any signatures or TPM measurements either, and could be made
> > > to contain an arbitrary sequence of SETUP_xxx nodes, which could
> > > interfere quite badly with the early x86 boot sequence.
> > >
> > > (*) Note that, while LoadImage() does take a buffer/size tuple in
> > > addition to a device path, which can be used to provide the image
> > > contents directly, it will re-allocate such images, as the memory
> > > footprint of an image is generally larger than the PE/COFF file
> > > representation.
> > >
> > > Next, in order to allow hypervisors to still use setup_data in scenarios
> > > where it may be useful, bump the x86 boot protocol version, so that
> > > hypervisors, e.g. QEMU in the linked patch, can do the right thing
> > > automatically depending on whether it is safe.
> > >
> > > Link: https://lore.kernel.org/qemu-devel/20220904165058.1140503-1-Jason@zx2c4.com/
> > > Coauthored-by: Ard Biesheuvel <ardb@kernel.org>
> > > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> > > ---
> > > arch/x86/boot/header.S | 2 +-
> > > drivers/firmware/efi/libstub/x86-stub.c | 7 +++++++
> > > 2 files changed, 8 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/arch/x86/boot/header.S b/arch/x86/boot/header.S
> > > index f912d7770130..e4e2d6e33924 100644
> > > --- a/arch/x86/boot/header.S
> > > +++ b/arch/x86/boot/header.S
> > > @@ -307,7 +307,7 @@ _start:
> > > # Part 2 of the header, from the old setup.S
> > >
> > > .ascii "HdrS" # header signature
> > > - .word 0x020f # header version number (>= 0x0105)
> > > + .word 0x0210 # header version number (>= 0x0105)
> > > # or else old loadlin-1.5 will fail)
> > > .globl realmode_swtch
> > > realmode_swtch: .word 0, 0 # default_switch, SETUPSEG
> > > diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
> > > index 05ae8bcc9d67..9780f32a9f24 100644
> > > --- a/drivers/firmware/efi/libstub/x86-stub.c
> > > +++ b/drivers/firmware/efi/libstub/x86-stub.c
> > > @@ -517,6 +517,13 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle,
> > > hdr->ramdisk_image = 0;
> > > hdr->ramdisk_size = 0;
> > >
> > > + /*
> > > + * Disregard any setup data that was provided by the bootloader:
> > > + * setup_data could be pointing anywhere, and we have no way of
> > > + * authenticating or validating the payload.
> > > + */
> > > + hdr->setup_data = 0;
> > > +
> > > efi_stub_entry(handle, sys_table_arg, boot_params);
> > > /* not reached */
> > >
> >
> > if the x86 folks are ok with this, I would like to send this to
> > cc:stable, but I imagine retroactively changing the header version
> > number might be something they would prefer to avoid. In that case,
> > better to split these up.
>
> Just FYI, the rng seed thing is new in 6.0 anyway. That still leaves
> the more obscure dtb one, and whatever else is used, but at least
> doing this for only 6.0+ would take care of the rng seed one.
>

Sure, but there is also the -dtb thing which can already be used to
crash the EFI stub. So even if this doesn't fix an issue occurring in
the wild, I think it is cleaner to clear setup_data on the pure EFI
entry path.