Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220904165321.1140894-1-Jason@zx2c4.com>
In-Reply-To: <20220904165321.1140894-1-Jason@zx2c4.com>
From:   Ard Biesheuvel <ardb@kernel.org>
Date:   Mon, 5 Sep 2022 11:54:52 +0200
Message-ID: <CAMj1kXHC9Wfr74761gPcG=N8OC2P76FqSb8FVGWM7x1p-4hQKg@mail.gmail.com>
Subject: Re: [PATCH] efi: x86: Wipe setup_data on pure EFI boot
To:     "Jason A. Donenfeld" <Jason@zx2c4.com>
Cc:     linux-efi@vger.kernel.org, linux-kernel@vger.kernel.org,
        x86@kernel.org, bp@alien8.de
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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.

Also, care to update Documentation/x86/boot.rst to document the new behavior?