On Mon, 5 Sept 2022 at 11:57, Jason A. Donenfeld <Jason@xxxxxxxxx> wrote: > > On Mon, Sep 5, 2022 at 11:55 AM Ard Biesheuvel <ardb@xxxxxxxxxx> wrote: > > > > On Sun, 4 Sept 2022 at 18:53, Jason A. Donenfeld <Jason@xxxxxxxxx> wrote: > > > > > > From: Ard Biesheuvel <ardb@xxxxxxxxxx> > > > > > > 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@xxxxxxxxx/ > > > Coauthored-by: Ard Biesheuvel <ardb@xxxxxxxxxx> > > > Signed-off-by: Jason A. Donenfeld <Jason@xxxxxxxxx> > > > --- > > > 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.
