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. Also, care to update Documentation/x86/boot.rst to document the new behavior?
