All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] efi: x86: Wipe setup_data on pure EFI boot
@ 2022-09-04 16:53 Jason A. Donenfeld
  2022-09-05  9:54 ` Ard Biesheuvel
  2022-09-22  7:34 ` [PATCH] " Ard Biesheuvel
  0 siblings, 2 replies; 8+ messages in thread
From: Jason A. Donenfeld @ 2022-09-04 16:53 UTC (permalink / raw)
  To: linux-efi, linux-kernel, x86, ardb, bp; +Cc: Jason A . Donenfeld

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 */
 
-- 
2.37.3


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] efi: x86: Wipe setup_data on pure EFI boot
  2022-09-04 16:53 [PATCH] efi: x86: Wipe setup_data on pure EFI boot Jason A. Donenfeld
@ 2022-09-05  9:54 ` Ard Biesheuvel
  2022-09-05  9:56   ` Jason A. Donenfeld
  2022-09-05  9:57   ` Jason A. Donenfeld
  2022-09-22  7:34 ` [PATCH] " Ard Biesheuvel
  1 sibling, 2 replies; 8+ messages in thread
From: Ard Biesheuvel @ 2022-09-05  9:54 UTC (permalink / raw)
  To: Jason A. Donenfeld; +Cc: linux-efi, linux-kernel, x86, bp

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?

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] efi: x86: Wipe setup_data on pure EFI boot
  2022-09-05  9:54 ` Ard Biesheuvel
@ 2022-09-05  9:56   ` Jason A. Donenfeld
  2022-09-05  9:59     ` Ard Biesheuvel
  2022-09-05  9:57   ` Jason A. Donenfeld
  1 sibling, 1 reply; 8+ messages in thread
From: Jason A. Donenfeld @ 2022-09-05  9:56 UTC (permalink / raw)
  To: Ard Biesheuvel; +Cc: linux-efi, LKML, X86 ML, Borislav Petkov

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.

Jason

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] efi: x86: Wipe setup_data on pure EFI boot
  2022-09-05  9:54 ` Ard Biesheuvel
  2022-09-05  9:56   ` Jason A. Donenfeld
@ 2022-09-05  9:57   ` Jason A. Donenfeld
  2022-09-06 10:41     ` [PATCH v2] " Jason A. Donenfeld
  1 sibling, 1 reply; 8+ messages in thread
From: Jason A. Donenfeld @ 2022-09-05  9:57 UTC (permalink / raw)
  To: Ard Biesheuvel; +Cc: linux-efi, LKML, X86 ML, Borislav Petkov

On Mon, Sep 5, 2022 at 11:55 AM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> Also, care to update Documentation/x86/boot.rst to document the new behavior?

Sure, I'll send a v2 with that.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] efi: x86: Wipe setup_data on pure EFI boot
  2022-09-05  9:56   ` Jason A. Donenfeld
@ 2022-09-05  9:59     ` Ard Biesheuvel
  0 siblings, 0 replies; 8+ messages in thread
From: Ard Biesheuvel @ 2022-09-05  9:59 UTC (permalink / raw)
  To: Jason A. Donenfeld; +Cc: linux-efi, LKML, X86 ML, Borislav Petkov

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.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v2] efi: x86: Wipe setup_data on pure EFI boot
  2022-09-05  9:57   ` Jason A. Donenfeld
@ 2022-09-06 10:41     ` Jason A. Donenfeld
  2022-09-06 12:21       ` Jason A. Donenfeld
  0 siblings, 1 reply; 8+ messages in thread
From: Jason A. Donenfeld @ 2022-09-06 10:41 UTC (permalink / raw)
  To: ardb, linux-efi, linux-kernel, x86, bp
  Cc: Jason A. Donenfeld, Borislav Petkov

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>
Cc: Borislav Petkov <bp@suse.de>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 Documentation/x86/boot.rst              | 2 ++
 arch/x86/boot/header.S                  | 2 +-
 drivers/firmware/efi/libstub/x86-stub.c | 7 +++++++
 3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/Documentation/x86/boot.rst b/Documentation/x86/boot.rst
index 894a19897005..9181f905937d 100644
--- a/Documentation/x86/boot.rst
+++ b/Documentation/x86/boot.rst
@@ -75,6 +75,8 @@ Protocol 2.14	BURNT BY INCORRECT COMMIT
 		DO NOT USE!!! ASSUME SAME AS 2.13.
 
 Protocol 2.15	(Kernel 5.5) Added the kernel_info and kernel_info.setup_type_max.
+
+Protocol 2.16   (Kernel 6.0) EFI stub loader ignores setup_data pointer.
 =============	============================================================
 
 .. note::
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 */
 
-- 
2.37.3


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] efi: x86: Wipe setup_data on pure EFI boot
  2022-09-06 10:41     ` [PATCH v2] " Jason A. Donenfeld
@ 2022-09-06 12:21       ` Jason A. Donenfeld
  0 siblings, 0 replies; 8+ messages in thread
From: Jason A. Donenfeld @ 2022-09-06 12:21 UTC (permalink / raw)
  To: X86 ML, Borislav Petkov, Borislav Petkov; +Cc: linux-efi, Ard Biesheuvel, LKML

Hi Boris and other x86ers,

On Tue, Sep 6, 2022 at 12:41 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> When booting the x86 kernel via EFI using the LoadImage/StartImage boot
> services [as opposed to the deprecated EFI handover protocol], the setup

Just FYI, while this is an EFI change for Ard, it also bumps the x86
boot protocol version, so it needs your ack or input on the matter.

Jason

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] efi: x86: Wipe setup_data on pure EFI boot
  2022-09-04 16:53 [PATCH] efi: x86: Wipe setup_data on pure EFI boot Jason A. Donenfeld
  2022-09-05  9:54 ` Ard Biesheuvel
@ 2022-09-22  7:34 ` Ard Biesheuvel
  1 sibling, 0 replies; 8+ messages in thread
From: Ard Biesheuvel @ 2022-09-22  7:34 UTC (permalink / raw)
  To: Jason A. Donenfeld; +Cc: linux-efi, linux-kernel, x86, bp

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>

Now that we've fixed this on the QEMU end without the need for a boot
protocol version bump [0], I am going to merge just the x86-stub.c
change as a fix for EFI.

Thanks,
Ard.

[0] https://lore.kernel.org/all/166383158063.107920.1563965268305325639.b4-ty@redhat.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 */
>
> --
> 2.37.3
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2022-09-22  7:34 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-04 16:53 [PATCH] efi: x86: Wipe setup_data on pure EFI boot Jason A. Donenfeld
2022-09-05  9:54 ` Ard Biesheuvel
2022-09-05  9:56   ` Jason A. Donenfeld
2022-09-05  9:59     ` Ard Biesheuvel
2022-09-05  9:57   ` Jason A. Donenfeld
2022-09-06 10:41     ` [PATCH v2] " Jason A. Donenfeld
2022-09-06 12:21       ` Jason A. Donenfeld
2022-09-22  7:34 ` [PATCH] " Ard Biesheuvel

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.