diff mbox series

efi: x86: Make the deprecated EFI handover protocol optional

Message ID 20221007172918.3131811-1-ardb@kernel.org
State New
Headers show
Series efi: x86: Make the deprecated EFI handover protocol optional | expand

Commit Message

Ard Biesheuvel Oct. 7, 2022, 5:29 p.m. UTC
The EFI handover protocol permits a bootloader to invoke the kernel as a
EFI PE/COFF application, while passing a bootparams struct as a third
argument to the entrypoint function call.

This has no basis in the UEFI specification, and there are better ways
to pass additional data to a UEFI application (UEFI configuration
tables, UEFI variables, UEFI protocols) than going around the
StartImage() boot service and jumping to a fixed offset in the loaded
image, just to call a different function that takes a third parameter.

The reason for handling struct bootparams in the bootloader was that the
EFI stub could only load initrd images from the EFI system partition,
and so passing it via struct bootparams was needed for loaders like
GRUB, which pass the initrd in memory, and may load it from anywhere,
including from the network. Another motivation was EFI mixed mode, which
could not use the initrd loader in the EFI stub at all due to 32/64 bit
incompatibilities (which will be fixed shortly [0]), and could not
invoke the ordinary PE/COFF entry point either, for the same reasons.

Given that loaders such as GRUB already carried the bootparams handling
in order to implement non-EFI boot, retaining that code and just passing
bootparams to the EFI stub was a reasonable choice (although defining an
alternate entrypoint could have been avoided.) However, the GRUB side
changes never made it upstream, and are only shipped by some of the
distros in their downstream versions.

In the meantime, EFI support has been added to other Linux architecture
ports, as well as to U-boot and systemd, including arch-agnostic methods
for passing initrd images in memory [1], and for doing mixed mode boot
[2], none of them requiring anything like the EFI handover protocol. So
given that only out-of-tree distro GRUB relies on this, let's permit it
to be omitted from the build, in preparation for retiring it completely
at a later date. (Note that systemd-boot does have an implementation as
well, but only uses it as a fallback for booting images that do not
implement the LoadFile2 based initrd loading method, i.e., v5.8 or older)

[0] https://lore.kernel.org/all/20220927085842.2860715-1-ardb@kernel.org/
[1] ec93fc371f01 ("efi/libstub: Add support for loading the initrd ...")
[2] 97aa276579b2 ("efi/x86: Add true mixed mode entry point into ...")

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/x86/Kconfig                   | 12 ++++++++++++
 arch/x86/boot/compressed/head_64.S |  4 +++-
 arch/x86/boot/header.S             |  2 +-
 arch/x86/boot/tools/build.c        |  2 ++
 4 files changed, 18 insertions(+), 2 deletions(-)

Comments

Borislav Petkov Oct. 8, 2022, 2:51 p.m. UTC | #1
On Fri, Oct 07, 2022 at 07:29:18PM +0200, Ard Biesheuvel wrote:
> Given that loaders such as GRUB already carried the bootparams handling
> in order to implement non-EFI boot, retaining that code and just passing
> bootparams to the EFI stub was a reasonable choice (although defining an
> alternate entrypoint could have been avoided.) However, the GRUB side
> changes never made it upstream, and are only shipped by some of the
	  ^^^^^^^^^^^^^^^^^^^^^^^

> distros in their downstream versions.

Gotta love that bit particularly. :-(

> In the meantime, EFI support has been added to other Linux architecture
> ports, as well as to U-boot and systemd, including arch-agnostic methods
> for passing initrd images in memory [1], and for doing mixed mode boot
> [2], none of them requiring anything like the EFI handover protocol. So
> given that only out-of-tree distro GRUB relies on this, let's permit it
> to be omitted from the build, in preparation for retiring it completely
> at a later date. (Note that systemd-boot does have an implementation as
> well, but only uses it as a fallback for booting images that do not
> implement the LoadFile2 based initrd loading method, i.e., v5.8 or older)
> 
> [0] https://lore.kernel.org/all/20220927085842.2860715-1-ardb@kernel.org/
> [1] ec93fc371f01 ("efi/libstub: Add support for loading the initrd ...")
> [2] 97aa276579b2 ("efi/x86: Add true mixed mode entry point into ...")
> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  arch/x86/Kconfig                   | 12 ++++++++++++
>  arch/x86/boot/compressed/head_64.S |  4 +++-
>  arch/x86/boot/header.S             |  2 +-
>  arch/x86/boot/tools/build.c        |  2 ++
>  4 files changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index f9920f1341c8..0c8fcb090232 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -1964,6 +1964,18 @@ config EFI_STUB
>  
>  	  See Documentation/admin-guide/efi-stub.rst for more information.
>  
> +config EFI_HANDOVER_PROTOCOL
> +	bool "EFI handover protocol (DEPRECATED)"
> +	depends on EFI_STUB
> +	default y

I'd say "default n" here.

> +	help
> +	  Whether to include support for the deprecated EFI handover protocol,

"Select this in order to include..."

> +	  which defines alternative entry points into the EFI stub. This is a
> +	  practice that has no basis in the UEFI specification, and requires
> +	  a priori knowledge on the part of the bootloader about Linux/x86
> +	  specific ways of passing the command line and initrd, and where in
> +	  memory those assets may be loaded.

	"If in doubt, say N. This option and accompanying code will disappear
	in some future kernel as the corresponding GRUB support is not even
	present in upstream GRUB but only in some distros' versions."

> +
>  config EFI_MIXED
>  	bool "EFI mixed-mode support"
>  	depends on EFI_STUB && X86_64
> diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
> index 6ba2c2142c33..7bcc50c6cdcc 100644
> --- a/arch/x86/boot/compressed/head_64.S
> +++ b/arch/x86/boot/compressed/head_64.S
> @@ -286,7 +286,7 @@ SYM_FUNC_START(startup_32)
>  	lret
>  SYM_FUNC_END(startup_32)
>  
> -#ifdef CONFIG_EFI_MIXED
> +#if defined(CONFIG_EFI_MIXED) && defined(CONFIG_EFI_HANDOVER_PROTOCOL)

...

Can we use IS_ENABLED() in all that instead, in order to improve
readability?

In any case, looks good. I'm thinking I'll take it into tip after -rc1
and see who cries and why...

Thx.
Ard Biesheuvel Oct. 8, 2022, 3:41 p.m. UTC | #2
On Sat, 8 Oct 2022 at 16:51, Borislav Petkov <bp@alien8.de> wrote:
>
> On Fri, Oct 07, 2022 at 07:29:18PM +0200, Ard Biesheuvel wrote:
> > Given that loaders such as GRUB already carried the bootparams handling
> > in order to implement non-EFI boot, retaining that code and just passing
> > bootparams to the EFI stub was a reasonable choice (although defining an
> > alternate entrypoint could have been avoided.) However, the GRUB side
> > changes never made it upstream, and are only shipped by some of the
>           ^^^^^^^^^^^^^^^^^^^^^^^
>
> > distros in their downstream versions.
>
> Gotta love that bit particularly. :-(
>

Yeah most distros have ~100 ore more patches against GRUB, but this
isn't actually their fault. GRUB maintainership was defunct for a
number of years, which is why we were stuck on GRUB version 2.02-beta3
for such a long time. But in recent years, things have been getting
better, and there is an agreement with the current maintainer not to
merge the EFI handover protocol, and merge the new EFI protocol based
initrd loading method instead, which works on all architectures
instead of only on x86.

> > In the meantime, EFI support has been added to other Linux architecture
> > ports, as well as to U-boot and systemd, including arch-agnostic methods
> > for passing initrd images in memory [1], and for doing mixed mode boot
> > [2], none of them requiring anything like the EFI handover protocol. So
> > given that only out-of-tree distro GRUB relies on this, let's permit it
> > to be omitted from the build, in preparation for retiring it completely
> > at a later date. (Note that systemd-boot does have an implementation as
> > well, but only uses it as a fallback for booting images that do not
> > implement the LoadFile2 based initrd loading method, i.e., v5.8 or older)
> >
> > [0] https://lore.kernel.org/all/20220927085842.2860715-1-ardb@kernel.org/
> > [1] ec93fc371f01 ("efi/libstub: Add support for loading the initrd ...")
> > [2] 97aa276579b2 ("efi/x86: Add true mixed mode entry point into ...")
> >
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> >  arch/x86/Kconfig                   | 12 ++++++++++++
> >  arch/x86/boot/compressed/head_64.S |  4 +++-
> >  arch/x86/boot/header.S             |  2 +-
> >  arch/x86/boot/tools/build.c        |  2 ++
> >  4 files changed, 18 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> > index f9920f1341c8..0c8fcb090232 100644
> > --- a/arch/x86/Kconfig
> > +++ b/arch/x86/Kconfig
> > @@ -1964,6 +1964,18 @@ config EFI_STUB
> >
> >         See Documentation/admin-guide/efi-stub.rst for more information.
> >
> > +config EFI_HANDOVER_PROTOCOL
> > +     bool "EFI handover protocol (DEPRECATED)"
> > +     depends on EFI_STUB
> > +     default y
>
> I'd say "default n" here.
>

I'd rather not - see below.

> > +     help
> > +       Whether to include support for the deprecated EFI handover protocol,
>
> "Select this in order to include..."
>

OK

> > +       which defines alternative entry points into the EFI stub. This is a
> > +       practice that has no basis in the UEFI specification, and requires
> > +       a priori knowledge on the part of the bootloader about Linux/x86
> > +       specific ways of passing the command line and initrd, and where in
> > +       memory those assets may be loaded.
>
>         "If in doubt, say N. This option and accompanying code will disappear
>         in some future kernel as the corresponding GRUB support is not even
>         present in upstream GRUB but only in some distros' versions."
>

I'll adopt some of that thanks.

> > +
> >  config EFI_MIXED
> >       bool "EFI mixed-mode support"
> >       depends on EFI_STUB && X86_64
> > diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
> > index 6ba2c2142c33..7bcc50c6cdcc 100644
> > --- a/arch/x86/boot/compressed/head_64.S
> > +++ b/arch/x86/boot/compressed/head_64.S
> > @@ -286,7 +286,7 @@ SYM_FUNC_START(startup_32)
> >       lret
> >  SYM_FUNC_END(startup_32)
> >
> > -#ifdef CONFIG_EFI_MIXED
> > +#if defined(CONFIG_EFI_MIXED) && defined(CONFIG_EFI_HANDOVER_PROTOCOL)
>
> ...
>
> Can we use IS_ENABLED() in all that instead, in order to improve
> readability?
>

Never tried that in .S files but I guess it should just work.

> In any case, looks good. I'm thinking I'll take it into tip after -rc1
> and see who cries and why...
>

I'd venture a guess that this will break the boot even your own x86
boxes, given that almost nobody uses plain upstream GRUB..

I can work with the distros directly to start disabling this in their
downstream configs once their GRUB builds are up to date with the new
changes, so we can phase this out in a controlled manner. But
disabling tthis right now by default is going to affect everyone who
builds their own kernels and runs them on a distro Linux install.
Borislav Petkov Oct. 8, 2022, 3:51 p.m. UTC | #3
On Sat, Oct 08, 2022 at 05:41:40PM +0200, Ard Biesheuvel wrote:
> Yeah most distros have ~100 ore more patches against GRUB, but this
> isn't actually their fault. GRUB maintainership was defunct for a
> number of years, which is why we were stuck on GRUB version 2.02-beta3
> for such a long time. But in recent years, things have been getting
> better, and there is an agreement with the current maintainer not to
> merge the EFI handover protocol, and merge the new EFI protocol based
> initrd loading method instead, which works on all architectures
> instead of only on x86.

Aha, ok.

> Never tried that in .S files but I guess it should just work.

If not, at least in the .c files.

> I'd venture a guess that this will break the boot even your own x86
> boxes, given that almost nobody uses plain upstream GRUB..
> 
> I can work with the distros directly to start disabling this in their
> downstream configs once their GRUB builds are up to date with the new
> changes, so we can phase this out in a controlled manner.

Hm, that might turn out to be a multi-year effort considering how the
enterprise distros' kernels are moving. Yeah, yeah, they have good
reasons and so on.

> But disabling tthis right now by default is going to affect everyone
> who builds their own kernels and runs them on a distro Linux install.

Ok, I can try it on my SUSE and Debian partitions and see what happens.

Thx.
Ard Biesheuvel Oct. 10, 2022, 8:59 a.m. UTC | #4
On Sat, 8 Oct 2022 at 17:51, Borislav Petkov <bp@alien8.de> wrote:
>
> On Sat, Oct 08, 2022 at 05:41:40PM +0200, Ard Biesheuvel wrote:
> > Yeah most distros have ~100 ore more patches against GRUB, but this
> > isn't actually their fault. GRUB maintainership was defunct for a
> > number of years, which is why we were stuck on GRUB version 2.02-beta3
> > for such a long time. But in recent years, things have been getting
> > better, and there is an agreement with the current maintainer not to
> > merge the EFI handover protocol, and merge the new EFI protocol based
> > initrd loading method instead, which works on all architectures
> > instead of only on x86.
>
> Aha, ok.
>
> > Never tried that in .S files but I guess it should just work.
>
> If not, at least in the .c files.
>
> > I'd venture a guess that this will break the boot even your own x86
> > boxes, given that almost nobody uses plain upstream GRUB..
> >
> > I can work with the distros directly to start disabling this in their
> > downstream configs once their GRUB builds are up to date with the new
> > changes, so we can phase this out in a controlled manner.
>
> Hm, that might turn out to be a multi-year effort considering how the
> enterprise distros' kernels are moving. Yeah, yeah, they have good
> reasons and so on.
>

Yes, this is going to take time. But we simply cannot get rid of it
today, so the choice we have is between doing nothing at all, or
taking the next step in phasing out this stuff.
Borislav Petkov Oct. 10, 2022, 9:21 a.m. UTC | #5
On Mon, Oct 10, 2022 at 10:59:24AM +0200, Ard Biesheuvel wrote:
> Yes, this is going to take time. But we simply cannot get rid of it
> today, so the choice we have is between doing nothing at all, or
> taking the next step in phasing out this stuff.

Yes, next step ofc. This is simply the next thing we're deprecating.

Thx.
diff mbox series

Patch

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index f9920f1341c8..0c8fcb090232 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -1964,6 +1964,18 @@  config EFI_STUB
 
 	  See Documentation/admin-guide/efi-stub.rst for more information.
 
+config EFI_HANDOVER_PROTOCOL
+	bool "EFI handover protocol (DEPRECATED)"
+	depends on EFI_STUB
+	default y
+	help
+	  Whether to include support for the deprecated EFI handover protocol,
+	  which defines alternative entry points into the EFI stub. This is a
+	  practice that has no basis in the UEFI specification, and requires
+	  a priori knowledge on the part of the bootloader about Linux/x86
+	  specific ways of passing the command line and initrd, and where in
+	  memory those assets may be loaded.
+
 config EFI_MIXED
 	bool "EFI mixed-mode support"
 	depends on EFI_STUB && X86_64
diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
index 6ba2c2142c33..7bcc50c6cdcc 100644
--- a/arch/x86/boot/compressed/head_64.S
+++ b/arch/x86/boot/compressed/head_64.S
@@ -286,7 +286,7 @@  SYM_FUNC_START(startup_32)
 	lret
 SYM_FUNC_END(startup_32)
 
-#ifdef CONFIG_EFI_MIXED
+#if defined(CONFIG_EFI_MIXED) && defined(CONFIG_EFI_HANDOVER_PROTOCOL)
 	.org 0x190
 SYM_FUNC_START(efi32_stub_entry)
 	add	$0x4, %esp		/* Discard return address */
@@ -516,7 +516,9 @@  trampoline_return:
 SYM_CODE_END(startup_64)
 
 #ifdef CONFIG_EFI_STUB
+#ifdef CONFIG_EFI_HANDOVER_PROTOCOL
 	.org 0x390
+#endif
 SYM_FUNC_START(efi64_stub_entry)
 	and	$~0xf, %rsp			/* realign the stack */
 	movq	%rdx, %rbx			/* save boot_params pointer */
diff --git a/arch/x86/boot/header.S b/arch/x86/boot/header.S
index f912d7770130..d31982509654 100644
--- a/arch/x86/boot/header.S
+++ b/arch/x86/boot/header.S
@@ -406,7 +406,7 @@  xloadflags:
 # define XLF1 0
 #endif
 
-#ifdef CONFIG_EFI_STUB
+#ifdef CONFIG_EFI_HANDOVER_PROTOCOL
 # ifdef CONFIG_EFI_MIXED
 #  define XLF23 (XLF_EFI_HANDOVER_32|XLF_EFI_HANDOVER_64)
 # else
diff --git a/arch/x86/boot/tools/build.c b/arch/x86/boot/tools/build.c
index a3725ad46c5a..bd247692b701 100644
--- a/arch/x86/boot/tools/build.c
+++ b/arch/x86/boot/tools/build.c
@@ -290,6 +290,7 @@  static void efi_stub_entry_update(void)
 {
 	unsigned long addr = efi32_stub_entry;
 
+#ifdef CONFIG_EFI_HANDOVER_PROTOCOL
 #ifdef CONFIG_X86_64
 	/* Yes, this is really how we defined it :( */
 	addr = efi64_stub_entry - 0x200;
@@ -298,6 +299,7 @@  static void efi_stub_entry_update(void)
 #ifdef CONFIG_EFI_MIXED
 	if (efi32_stub_entry != addr)
 		die("32-bit and 64-bit EFI entry points do not match\n");
+#endif
 #endif
 	put_unaligned_le32(addr, &buf[0x264]);
 }