[PATCH] x86/boot: efistub: Assign global boot_params variable

[PATCH] x86/boot: efistub: Assign global boot_params variable

[Ard Biesheuvel]

From: Ard Biesheuvel <ardb@kernel.org>

Now that the x86 EFI stub calls into some APIs exposed by the
decompressor (e.g., kaslr_get_random_long()), it is necessary to ensure
that the global boot_params variable is set correctly before doing so.

Note that the decompressor and the kernel proper carry conflicting
declarations for the global variable 'boot_params' so refer to it via an
alias to work around this.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 drivers/firmware/efi/libstub/x86-stub.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
index 3bfc59637dd1..51407207ded5 100644
--- a/drivers/firmware/efi/libstub/x86-stub.c
+++ b/drivers/firmware/efi/libstub/x86-stub.c
@@ -812,12 +812,15 @@ void __noreturn efi_stub_entry(efi_handle_t handle,
 			       efi_system_table_t *sys_table_arg,
 			       struct boot_params *boot_params)
 {
+	extern struct boot_params *bpp asm("boot_params");
 	efi_guid_t guid = EFI_MEMORY_ATTRIBUTE_PROTOCOL_GUID;
 	struct setup_header *hdr = &boot_params->hdr;
 	const struct linux_efi_initrd *initrd = NULL;
 	unsigned long kernel_entry;
 	efi_status_t status;
 
+	bpp = boot_params;
+
 	efi_system_table = sys_table_arg;
 	/* Check if we were booted by the EFI firmware */
 	if (efi_system_table->hdr.signature != EFI_SYSTEM_TABLE_SIGNATURE)
-- 
2.42.0.655.g421f12c284-goog

Re: [PATCH] x86/boot: efistub: Assign global boot_params variable

[Ard Biesheuvel]

On Mon, 16 Oct 2023 at 18:46, Ard Biesheuvel <ardb@google.com> wrote:
>
> From: Ard Biesheuvel <ardb@kernel.org>
>
> Now that the x86 EFI stub calls into some APIs exposed by the
> decompressor (e.g., kaslr_get_random_long()), it is necessary to ensure
> that the global boot_params variable is set correctly before doing so.
>
> Note that the decompressor and the kernel proper carry conflicting
> declarations for the global variable 'boot_params' so refer to it via an
> alias to work around this.
>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  drivers/firmware/efi/libstub/x86-stub.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
> index 3bfc59637dd1..51407207ded5 100644
> --- a/drivers/firmware/efi/libstub/x86-stub.c
> +++ b/drivers/firmware/efi/libstub/x86-stub.c
> @@ -812,12 +812,15 @@ void __noreturn efi_stub_entry(efi_handle_t handle,
>                                efi_system_table_t *sys_table_arg,
>                                struct boot_params *boot_params)
>  {
> +       extern struct boot_params *bpp asm("boot_params");

I am getting hatemail from the bots about this extern declaration, so
I will move that into x86-stub.h.

Unless anyone minds, I'll queue this up and send it to Linus myself
via the EFI fixes branch.


>         efi_guid_t guid = EFI_MEMORY_ATTRIBUTE_PROTOCOL_GUID;
>         struct setup_header *hdr = &boot_params->hdr;
>         const struct linux_efi_initrd *initrd = NULL;
>         unsigned long kernel_entry;
>         efi_status_t status;
>
> +       bpp = boot_params;
> +
>         efi_system_table = sys_table_arg;
>         /* Check if we were booted by the EFI firmware */
>         if (efi_system_table->hdr.signature != EFI_SYSTEM_TABLE_SIGNATURE)
> --
> 2.42.0.655.g421f12c284-goog
>

Re: [PATCH] x86/boot: efistub: Assign global boot_params variable

[Ingo Molnar]

* Ard Biesheuvel <ardb@kernel.org> wrote:

> On Mon, 16 Oct 2023 at 18:46, Ard Biesheuvel <ardb@google.com> wrote:
> >
> > From: Ard Biesheuvel <ardb@kernel.org>
> >
> > Now that the x86 EFI stub calls into some APIs exposed by the
> > decompressor (e.g., kaslr_get_random_long()), it is necessary to ensure
> > that the global boot_params variable is set correctly before doing so.
> >
> > Note that the decompressor and the kernel proper carry conflicting
> > declarations for the global variable 'boot_params' so refer to it via an
> > alias to work around this.
> >
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> >  drivers/firmware/efi/libstub/x86-stub.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
> > index 3bfc59637dd1..51407207ded5 100644
> > --- a/drivers/firmware/efi/libstub/x86-stub.c
> > +++ b/drivers/firmware/efi/libstub/x86-stub.c
> > @@ -812,12 +812,15 @@ void __noreturn efi_stub_entry(efi_handle_t handle,
> >                                efi_system_table_t *sys_table_arg,
> >                                struct boot_params *boot_params)
> >  {
> > +       extern struct boot_params *bpp asm("boot_params");
> 
> I am getting hatemail from the bots about this extern declaration, so
> I will move that into x86-stub.h.

Hm, it would be better if we moved all of x86-stub.h into 
arch/x86/include/asm/ or so. We don't really encourage "deep" driver-level 
exports like that, and there's little reason why this couldn't be in the 
x86 headers, right?

> Unless anyone minds, I'll queue this up and send it to Linus myself
> via the EFI fixes branch.

Other than the above:

  Acked-by: Ingo Molnar <mingo@kernel.org>

Thanks,

	Ingo

Re: [PATCH] x86/boot: efistub: Assign global boot_params variable

[Ard Biesheuvel]

On Tue, 17 Oct 2023 at 13:34, Ingo Molnar <mingo@kernel.org> wrote:
>
>
> * Ard Biesheuvel <ardb@kernel.org> wrote:
>
> > On Mon, 16 Oct 2023 at 18:46, Ard Biesheuvel <ardb@google.com> wrote:
> > >
> > > From: Ard Biesheuvel <ardb@kernel.org>
> > >
> > > Now that the x86 EFI stub calls into some APIs exposed by the
> > > decompressor (e.g., kaslr_get_random_long()), it is necessary to ensure
> > > that the global boot_params variable is set correctly before doing so.
> > >
> > > Note that the decompressor and the kernel proper carry conflicting
> > > declarations for the global variable 'boot_params' so refer to it via an
> > > alias to work around this.
> > >
> > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > > ---
> > >  drivers/firmware/efi/libstub/x86-stub.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
> > > index 3bfc59637dd1..51407207ded5 100644
> > > --- a/drivers/firmware/efi/libstub/x86-stub.c
> > > +++ b/drivers/firmware/efi/libstub/x86-stub.c
> > > @@ -812,12 +812,15 @@ void __noreturn efi_stub_entry(efi_handle_t handle,
> > >                                efi_system_table_t *sys_table_arg,
> > >                                struct boot_params *boot_params)
> > >  {
> > > +       extern struct boot_params *bpp asm("boot_params");
> >
> > I am getting hatemail from the bots about this extern declaration, so
> > I will move that into x86-stub.h.
>
> Hm, it would be better if we moved all of x86-stub.h into
> arch/x86/include/asm/ or so. We don't really encourage "deep" driver-level
> exports like that, and there's little reason why this couldn't be in the
> x86 headers, right?
>

Yes, there is.

arch/x86/boot/compressed/misc.h:extern struct boot_params *boot_params;
arch/x86/include/asm/setup.h:extern struct boot_params boot_params;

Ideally, we should get rid of the first one.

Happy to send a patch that replaces all conflicting uses in the
decompressor, so we can just access the global pointer without these
tricks. But I'd like to get the fix in for v6.6 in any case.


> > Unless anyone minds, I'll queue this up and send it to Linus myself
> > via the EFI fixes branch.
>
> Other than the above:
>
>   Acked-by: Ingo Molnar <mingo@kernel.org>
>

Re: [PATCH] x86/boot: efistub: Assign global boot_params variable

[Ingo Molnar]

* Ard Biesheuvel <ardb@kernel.org> wrote:

> On Tue, 17 Oct 2023 at 13:34, Ingo Molnar <mingo@kernel.org> wrote:
> >
> >
> > * Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > > On Mon, 16 Oct 2023 at 18:46, Ard Biesheuvel <ardb@google.com> wrote:
> > > >
> > > > From: Ard Biesheuvel <ardb@kernel.org>
> > > >
> > > > Now that the x86 EFI stub calls into some APIs exposed by the
> > > > decompressor (e.g., kaslr_get_random_long()), it is necessary to ensure
> > > > that the global boot_params variable is set correctly before doing so.
> > > >
> > > > Note that the decompressor and the kernel proper carry conflicting
> > > > declarations for the global variable 'boot_params' so refer to it via an
> > > > alias to work around this.
> > > >
> > > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > > > ---
> > > >  drivers/firmware/efi/libstub/x86-stub.c | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
> > > > index 3bfc59637dd1..51407207ded5 100644
> > > > --- a/drivers/firmware/efi/libstub/x86-stub.c
> > > > +++ b/drivers/firmware/efi/libstub/x86-stub.c
> > > > @@ -812,12 +812,15 @@ void __noreturn efi_stub_entry(efi_handle_t handle,
> > > >                                efi_system_table_t *sys_table_arg,
> > > >                                struct boot_params *boot_params)
> > > >  {
> > > > +       extern struct boot_params *bpp asm("boot_params");
> > >
> > > I am getting hatemail from the bots about this extern declaration, so
> > > I will move that into x86-stub.h.
> >
> > Hm, it would be better if we moved all of x86-stub.h into
> > arch/x86/include/asm/ or so. We don't really encourage "deep" driver-level
> > exports like that, and there's little reason why this couldn't be in the
> > x86 headers, right?
> >
> 
> Yes, there is.
> 
> arch/x86/boot/compressed/misc.h:extern struct boot_params *boot_params;
> arch/x86/include/asm/setup.h:extern struct boot_params boot_params;
> 
> Ideally, we should get rid of the first one.
> 
> Happy to send a patch that replaces all conflicting uses in the
> decompressor, so we can just access the global pointer without these
> tricks.

Yeah, that would be lovely, thanks.

> But I'd like to get the fix in for v6.6 in any case.

Sure:

  Acked-by: Ingo Molnar <mingo@kernel.org>

Thanks,

	Ingo