linux-kernel - Re: [PATCH] efi: x86: Wipe setup

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAMj1kXHPypCO5MNvFcw795LpZXPDRLjOdUJ4iRWxX7km7BF0ig@mail.gmail.com>
Date:   Mon, 5 Sep 2022 11:59:39 +0200
From:   Ard Biesheuvel <ardb@...nel.org>
To:     "Jason A. Donenfeld" <Jason@...c4.com>
Cc:     linux-efi <linux-efi@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>, X86 ML <x86@...nel.org>,
        Borislav Petkov <bp@...en8.de>
Subject: Re: [PATCH] efi: x86: Wipe setup_data on pure EFI boot

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