[GIT PULL 0/9] EFI fixes for v5.7-rc

[GIT PULL 0/9] EFI fixes for v5.7-rc

[Ard Biesheuvel]

The following changes since commit 594e576d4b93b8cda3247542366b47e1b2ddc4dc:

  efi/libstub/arm: Fix spurious message that an initrd was loaded (2020-03-29 12:08:18 +0200)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git tags/efi-urgent

for you to fetch changes up to 55a3cad6df4bff67280c4722ceb2a5ff4375eff9:

  efi/x86: Don't remap text<->rodata gap read-only for mixed mode (2020-04-09 14:50:21 +0200)

----------------------------------------------------------------
EFI fixes for v5.7-rcX:
- a couple of mixed mode fixes for the changes made in v5.6
- a couple of fixes from Arvind to deal with buggy firmware that does not
  load the PE/COFF kernel image based on the description in the PE/COFF
  header, but simply copies it to memory
- add a clarification to the x86 boot protocol documentation regarding the
  previous issue
- reduce the stack footprint of the new file I/O code, to suppress a
  compiler warning
- use the right sprintf() flavor in the CPER code to prevent a potential
  out of bounds write
- prevent a symbol reference from going out of range in the ARM startup code
- cosmetic fix for an issue reported by Coverity

----------------------------------------------------------------
Ard Biesheuvel (4):
      efi/arm: Deal with ADR going out of range in efi_enter_kernel()
      Documentation: efi/x86: clarify EFI handover protocol and its requirements
      efi/libstub/file: merge filename buffers to reduce stack usage
      efi/x86: Don't remap text<->rodata gap read-only for mixed mode

Arvind Sankar (2):
      efi/x86: Move efi stub globals from .bss to .data
      efi/x86: Always relocate the kernel for EFI handover entry

Colin Ian King (1):
      efi/libstub/x86: remove redundant assignment to pointer hdr

Gary Lin (1):
      efi/x86: Fix the deletion of variables in mixed mode

Takashi Iwai (1):
      efi/cper: Use scnprintf() for avoiding potential buffer overflow

 Documentation/x86/boot.rst              | 21 ++++++++++++++++++---
 arch/arm/boot/compressed/head.S         |  3 ++-
 arch/x86/platform/efi/efi_64.c          | 16 ++++++++++++----
 drivers/firmware/efi/cper.c             |  2 +-
 drivers/firmware/efi/libstub/efistub.h  |  2 +-
 drivers/firmware/efi/libstub/file.c     | 27 ++++++++++++++-------------
 drivers/firmware/efi/libstub/x86-stub.c | 18 +++++++++++-------
 7 files changed, 59 insertions(+), 30 deletions(-)

[PATCH 1/9] efi/cper: Use scnprintf() for avoiding potential buffer overflow

[Ard Biesheuvel]

From: Takashi Iwai <tiwai@suse.de>

Since snprintf() returns the would-be-output size instead of the
actual output size, the succeeding calls may go beyond the given
buffer limit.  Fix it by replacing with scnprintf().

Signed-off-by: Takashi Iwai <tiwai@suse.de>
Link: https://lore.kernel.org/r/20200311072145.5001-1-tiwai@suse.de
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 drivers/firmware/efi/cper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
index b1af0de2e100..9d2512913d25 100644
--- a/drivers/firmware/efi/cper.c
+++ b/drivers/firmware/efi/cper.c
@@ -101,7 +101,7 @@ void cper_print_bits(const char *pfx, unsigned int bits,
 		if (!len)
 			len = snprintf(buf, sizeof(buf), "%s%s", pfx, str);
 		else
-			len += snprintf(buf+len, sizeof(buf)-len, ", %s", str);
+			len += scnprintf(buf+len, sizeof(buf)-len, ", %s", str);
 	}
 	if (len)
 		printk("%s\n", buf);
-- 
2.17.1

[PATCH 2/9] efi/libstub/x86: remove redundant assignment to pointer hdr

[Ard Biesheuvel]

From: Colin Ian King <colin.king@canonical.com>

The pointer hdr is being assigned a value that is never read and
it is being updated later with a new value. The assignment is
redundant and can be removed.

Addresses-Coverity: ("Unused value")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
Link: https://lore.kernel.org/r/20200402102537.503103-1-colin.king@canonical.com
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 drivers/firmware/efi/libstub/x86-stub.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
index 8d3a707789de..e02ea51273ff 100644
--- a/drivers/firmware/efi/libstub/x86-stub.c
+++ b/drivers/firmware/efi/libstub/x86-stub.c
@@ -392,8 +392,6 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle,
 	image_base = efi_table_attr(image, image_base);
 	image_offset = (void *)startup_32 - image_base;
 
-	hdr = &((struct boot_params *)image_base)->hdr;
-
 	status = efi_allocate_pages(0x4000, (unsigned long *)&boot_params, ULONG_MAX);
 	if (status != EFI_SUCCESS) {
 		efi_printk("Failed to allocate lowmem for boot params\n");
-- 
2.17.1

[PATCH 3/9] efi/x86: Move efi stub globals from .bss to .data

[Ard Biesheuvel]

From: Arvind Sankar <nivedita@alum.mit.edu>

Commit

  3ee372ccce4d ("x86/boot/compressed/64: Remove .bss/.pgtable from bzImage")

removed the .bss section from the bzImage.

However, while a PE loader is required to zero-initialize the .bss
section before calling the PE entry point, the EFI handover protocol
does not currently document any requirement that .bss be initialized by
the bootloader prior to calling the handover entry.

When systemd-boot is used to boot a unified kernel image [1], the image
is constructed by embedding the bzImage as a .linux section in a PE
executable that contains a small stub loader from systemd together with
additional sections and potentially an initrd. As the .bss section
within the bzImage is no longer explicitly present as part of the file,
it is not initialized before calling the EFI handover entry.
Furthermore, as the size of the embedded .linux section is only the size
of the bzImage file itself, the .bss section's memory may not even have
been allocated.

In particular, this can result in efi_disable_pci_dma being true even
when it was not specified via the command line or configuration option,
which in turn causes crashes while booting on some systems.

To avoid issues, place all EFI stub global variables into the .data
section instead of .bss. As of this writing, only boolean flags for a
few command line arguments and the sys_table pointer were in .bss and
will now move into the .data section.

[1] https://systemd.io/BOOT_LOADER_SPECIFICATION/#type-2-efi-unified-kernel-images

Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
Reported-by: Sergey Shatunov <me@prok.pw>
Fixes: 3ee372ccce4d ("x86/boot/compressed/64: Remove .bss/.pgtable from bzImage")
Link: https://lore.kernel.org/r/20200406180614.429454-1-nivedita@alum.mit.edu
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 drivers/firmware/efi/libstub/efistub.h  | 2 +-
 drivers/firmware/efi/libstub/x86-stub.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
index cc90a748bcf0..67d26949fd26 100644
--- a/drivers/firmware/efi/libstub/efistub.h
+++ b/drivers/firmware/efi/libstub/efistub.h
@@ -25,7 +25,7 @@
 #define EFI_ALLOC_ALIGN		EFI_PAGE_SIZE
 #endif
 
-#ifdef CONFIG_ARM
+#if defined(CONFIG_ARM) || defined(CONFIG_X86)
 #define __efistub_global	__section(.data)
 #else
 #define __efistub_global
diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
index e02ea51273ff..867a57e28980 100644
--- a/drivers/firmware/efi/libstub/x86-stub.c
+++ b/drivers/firmware/efi/libstub/x86-stub.c
@@ -20,7 +20,7 @@
 /* Maximum physical address for 64-bit kernel with 4-level paging */
 #define MAXMEM_X86_64_4LEVEL (1ull << 46)
 
-static efi_system_table_t *sys_table;
+static efi_system_table_t *sys_table __efistub_global;
 extern const bool efi_is64;
 extern u32 image_offset;
 
-- 
2.17.1

Re: [PATCH 3/9] efi/x86: Move efi stub globals from .bss to .data

[Brian Gerst]

On Thu, Apr 9, 2020 at 9:07 AM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> From: Arvind Sankar <nivedita@alum.mit.edu>
>
> Commit
>
>   3ee372ccce4d ("x86/boot/compressed/64: Remove .bss/.pgtable from bzImage")
>
> removed the .bss section from the bzImage.
>
> However, while a PE loader is required to zero-initialize the .bss
> section before calling the PE entry point, the EFI handover protocol
> does not currently document any requirement that .bss be initialized by
> the bootloader prior to calling the handover entry.
>
> When systemd-boot is used to boot a unified kernel image [1], the image
> is constructed by embedding the bzImage as a .linux section in a PE
> executable that contains a small stub loader from systemd together with
> additional sections and potentially an initrd. As the .bss section
> within the bzImage is no longer explicitly present as part of the file,
> it is not initialized before calling the EFI handover entry.
> Furthermore, as the size of the embedded .linux section is only the size
> of the bzImage file itself, the .bss section's memory may not even have
> been allocated.
>
> In particular, this can result in efi_disable_pci_dma being true even
> when it was not specified via the command line or configuration option,
> which in turn causes crashes while booting on some systems.
>
> To avoid issues, place all EFI stub global variables into the .data
> section instead of .bss. As of this writing, only boolean flags for a
> few command line arguments and the sys_table pointer were in .bss and
> will now move into the .data section.
>
> [1] https://systemd.io/BOOT_LOADER_SPECIFICATION/#type-2-efi-unified-kernel-images
>
> Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
> Reported-by: Sergey Shatunov <me@prok.pw>
> Fixes: 3ee372ccce4d ("x86/boot/compressed/64: Remove .bss/.pgtable from bzImage")
> Link: https://lore.kernel.org/r/20200406180614.429454-1-nivedita@alum.mit.edu
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  drivers/firmware/efi/libstub/efistub.h  | 2 +-
>  drivers/firmware/efi/libstub/x86-stub.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
> index cc90a748bcf0..67d26949fd26 100644
> --- a/drivers/firmware/efi/libstub/efistub.h
> +++ b/drivers/firmware/efi/libstub/efistub.h
> @@ -25,7 +25,7 @@
>  #define EFI_ALLOC_ALIGN                EFI_PAGE_SIZE
>  #endif
>
> -#ifdef CONFIG_ARM
> +#if defined(CONFIG_ARM) || defined(CONFIG_X86)
>  #define __efistub_global       __section(.data)
>  #else
>  #define __efistub_global
> diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
> index e02ea51273ff..867a57e28980 100644
> --- a/drivers/firmware/efi/libstub/x86-stub.c
> +++ b/drivers/firmware/efi/libstub/x86-stub.c
> @@ -20,7 +20,7 @@
>  /* Maximum physical address for 64-bit kernel with 4-level paging */
>  #define MAXMEM_X86_64_4LEVEL (1ull << 46)
>
> -static efi_system_table_t *sys_table;
> +static efi_system_table_t *sys_table __efistub_global;
>  extern const bool efi_is64;
>  extern u32 image_offset;
>
> --
> 2.17.1
>

Can we use the -fno-zero-initialized-in-bss compiler flag instead of
explicitly marking global variables?

--
Brian Gerst

Re: [PATCH 3/9] efi/x86: Move efi stub globals from .bss to .data

[Brian Gerst]

On Thu, Apr 9, 2020 at 4:05 PM Brian Gerst <brgerst@gmail.com> wrote:
>
> On Thu, Apr 9, 2020 at 9:07 AM Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > From: Arvind Sankar <nivedita@alum.mit.edu>
> >
> > Commit
> >
> >   3ee372ccce4d ("x86/boot/compressed/64: Remove .bss/.pgtable from bzImage")
> >
> > removed the .bss section from the bzImage.
> >
> > However, while a PE loader is required to zero-initialize the .bss
> > section before calling the PE entry point, the EFI handover protocol
> > does not currently document any requirement that .bss be initialized by
> > the bootloader prior to calling the handover entry.
> >
> > When systemd-boot is used to boot a unified kernel image [1], the image
> > is constructed by embedding the bzImage as a .linux section in a PE
> > executable that contains a small stub loader from systemd together with
> > additional sections and potentially an initrd. As the .bss section
> > within the bzImage is no longer explicitly present as part of the file,
> > it is not initialized before calling the EFI handover entry.
> > Furthermore, as the size of the embedded .linux section is only the size
> > of the bzImage file itself, the .bss section's memory may not even have
> > been allocated.
> >
> > In particular, this can result in efi_disable_pci_dma being true even
> > when it was not specified via the command line or configuration option,
> > which in turn causes crashes while booting on some systems.
> >
> > To avoid issues, place all EFI stub global variables into the .data
> > section instead of .bss. As of this writing, only boolean flags for a
> > few command line arguments and the sys_table pointer were in .bss and
> > will now move into the .data section.
> >
> > [1] https://systemd.io/BOOT_LOADER_SPECIFICATION/#type-2-efi-unified-kernel-images
> >
> > Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
> > Reported-by: Sergey Shatunov <me@prok.pw>
> > Fixes: 3ee372ccce4d ("x86/boot/compressed/64: Remove .bss/.pgtable from bzImage")
> > Link: https://lore.kernel.org/r/20200406180614.429454-1-nivedita@alum.mit.edu
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> >  drivers/firmware/efi/libstub/efistub.h  | 2 +-
> >  drivers/firmware/efi/libstub/x86-stub.c | 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
> > index cc90a748bcf0..67d26949fd26 100644
> > --- a/drivers/firmware/efi/libstub/efistub.h
> > +++ b/drivers/firmware/efi/libstub/efistub.h
> > @@ -25,7 +25,7 @@
> >  #define EFI_ALLOC_ALIGN                EFI_PAGE_SIZE
> >  #endif
> >
> > -#ifdef CONFIG_ARM
> > +#if defined(CONFIG_ARM) || defined(CONFIG_X86)
> >  #define __efistub_global       __section(.data)
> >  #else
> >  #define __efistub_global
> > diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
> > index e02ea51273ff..867a57e28980 100644
> > --- a/drivers/firmware/efi/libstub/x86-stub.c
> > +++ b/drivers/firmware/efi/libstub/x86-stub.c
> > @@ -20,7 +20,7 @@
> >  /* Maximum physical address for 64-bit kernel with 4-level paging */
> >  #define MAXMEM_X86_64_4LEVEL (1ull << 46)
> >
> > -static efi_system_table_t *sys_table;
> > +static efi_system_table_t *sys_table __efistub_global;
> >  extern const bool efi_is64;
> >  extern u32 image_offset;
> >
> > --
> > 2.17.1
> >
>
> Can we use the -fno-zero-initialized-in-bss compiler flag instead of
> explicitly marking global variables?

Scratch that.  Apparently it only works when a variable is explicitly
initialized to zero.

--
Brian Gerst

Re: [PATCH 3/9] efi/x86: Move efi stub globals from .bss to .data

[Arvind Sankar]

On Thu, Apr 09, 2020 at 04:53:07PM -0400, Brian Gerst wrote:
> > Can we use the -fno-zero-initialized-in-bss compiler flag instead of
> > explicitly marking global variables?
> 
> Scratch that.  Apparently it only works when a variable is explicitly
> initialized to zero.
> 
> --
> Brian Gerst

Right, there doesn't seem to be a compiler option to turn off the use of
.bss altogether.

Re: [PATCH 3/9] efi/x86: Move efi stub globals from .bss to .data

[Ard Biesheuvel]

On Thu, 9 Apr 2020 at 23:08, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> On Thu, Apr 09, 2020 at 04:53:07PM -0400, Brian Gerst wrote:
> > > Can we use the -fno-zero-initialized-in-bss compiler flag instead of
> > > explicitly marking global variables?
> >
> > Scratch that.  Apparently it only works when a variable is explicitly
> > initialized to zero.
> >
> > --
> > Brian Gerst
>
> Right, there doesn't seem to be a compiler option to turn off the use of
> .bss altogether.

Yeah. I'll try to come up with a way to consolidate this a bit across
architectures (which is a bit easier now that all of the EFI stub C
code lives in the same place). It is probably easiest to use a section
renaming trick similar to the one I added for ARM (as Arvind suggested
as well, IIRC), and get rid of the per-symbol annotations altogether.

Re: [PATCH 3/9] efi/x86: Move efi stub globals from .bss to .data

[Arvind Sankar]

On Fri, Apr 10, 2020 at 10:20:42AM +0200, Ard Biesheuvel wrote:
> On Thu, 9 Apr 2020 at 23:08, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> >
> > On Thu, Apr 09, 2020 at 04:53:07PM -0400, Brian Gerst wrote:
> > > > Can we use the -fno-zero-initialized-in-bss compiler flag instead of
> > > > explicitly marking global variables?
> > >
> > > Scratch that.  Apparently it only works when a variable is explicitly
> > > initialized to zero.
> > >
> > > --
> > > Brian Gerst
> >
> > Right, there doesn't seem to be a compiler option to turn off the use of
> > .bss altogether.
> 
> Yeah. I'll try to come up with a way to consolidate this a bit across
> architectures (which is a bit easier now that all of the EFI stub C
> code lives in the same place). It is probably easiest to use a section
> renaming trick similar to the one I added for ARM (as Arvind suggested
> as well, IIRC), and get rid of the per-symbol annotations altogether.

Does that work for 32-bit ARM, or does it need to be .data to tell the
compiler to avoid generating GOT references? If that's fine, we don't
actually need to rename sections -- linker script magic is enough. For
eg, the below pulls the EFI stub bss into .data for x86 without the need
for the annotations.

diff --git a/arch/x86/boot/compressed/vmlinux.lds.S b/arch/x86/boot/compressed/vmlinux.lds.S
index 508cfa6828c5..e324819c95bc 100644
--- a/arch/x86/boot/compressed/vmlinux.lds.S
+++ b/arch/x86/boot/compressed/vmlinux.lds.S
@@ -52,6 +52,7 @@ SECTIONS
 		_data = . ;
 		*(.data)
 		*(.data.*)
+		drivers/firmware/efi/libstub/lib.a:(.bss .bss.*)
 		_edata = . ;
 	}
 	. = ALIGN(L1_CACHE_BYTES);

Re: [PATCH 3/9] efi/x86: Move efi stub globals from .bss to .data

[Ard Biesheuvel]

On Fri, 10 Apr 2020 at 17:16, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> On Fri, Apr 10, 2020 at 10:20:42AM +0200, Ard Biesheuvel wrote:
> > On Thu, 9 Apr 2020 at 23:08, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> > >
> > > On Thu, Apr 09, 2020 at 04:53:07PM -0400, Brian Gerst wrote:
> > > > > Can we use the -fno-zero-initialized-in-bss compiler flag instead of
> > > > > explicitly marking global variables?
> > > >
> > > > Scratch that.  Apparently it only works when a variable is explicitly
> > > > initialized to zero.
> > > >
> > > > --
> > > > Brian Gerst
> > >
> > > Right, there doesn't seem to be a compiler option to turn off the use of
> > > .bss altogether.
> >
> > Yeah. I'll try to come up with a way to consolidate this a bit across
> > architectures (which is a bit easier now that all of the EFI stub C
> > code lives in the same place). It is probably easiest to use a section
> > renaming trick similar to the one I added for ARM (as Arvind suggested
> > as well, IIRC), and get rid of the per-symbol annotations altogether.
>
> Does that work for 32-bit ARM, or does it need to be .data to tell the
> compiler to avoid generating GOT references? If that's fine, we don't
> actually need to rename sections -- linker script magic is enough. For
> eg, the below pulls the EFI stub bss into .data for x86 without the need
> for the annotations.
>
> diff --git a/arch/x86/boot/compressed/vmlinux.lds.S b/arch/x86/boot/compressed/vmlinux.lds.S
> index 508cfa6828c5..e324819c95bc 100644
> --- a/arch/x86/boot/compressed/vmlinux.lds.S
> +++ b/arch/x86/boot/compressed/vmlinux.lds.S
> @@ -52,6 +52,7 @@ SECTIONS
>                 _data = . ;
>                 *(.data)
>                 *(.data.*)
> +               drivers/firmware/efi/libstub/lib.a:(.bss .bss.*)
>                 _edata = . ;
>         }
>         . = ALIGN(L1_CACHE_BYTES);

No, we can add this to ARM as well, and get rid of the
__efistub_global annotations entirely.

We'll still need .data.efistub for the .data pieces, but that is a
separate issue.

Re: [PATCH 3/9] efi/x86: Move efi stub globals from .bss to .data

[Arvind Sankar]

On Fri, Apr 10, 2020 at 06:03:38PM +0200, Ard Biesheuvel wrote:
> On Fri, 10 Apr 2020 at 17:16, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> >
> > On Fri, Apr 10, 2020 at 10:20:42AM +0200, Ard Biesheuvel wrote:
> > > On Thu, 9 Apr 2020 at 23:08, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> > > >
> > > > On Thu, Apr 09, 2020 at 04:53:07PM -0400, Brian Gerst wrote:
> > > > > > Can we use the -fno-zero-initialized-in-bss compiler flag instead of
> > > > > > explicitly marking global variables?
> > > > >
> > > > > Scratch that.  Apparently it only works when a variable is explicitly
> > > > > initialized to zero.
> > > > >
> > > > > --
> > > > > Brian Gerst
> > > >
> > > > Right, there doesn't seem to be a compiler option to turn off the use of
> > > > .bss altogether.
> > >
> > > Yeah. I'll try to come up with a way to consolidate this a bit across
> > > architectures (which is a bit easier now that all of the EFI stub C
> > > code lives in the same place). It is probably easiest to use a section
> > > renaming trick similar to the one I added for ARM (as Arvind suggested
> > > as well, IIRC), and get rid of the per-symbol annotations altogether.
> >
> > Does that work for 32-bit ARM, or does it need to be .data to tell the
> > compiler to avoid generating GOT references? If that's fine, we don't
> > actually need to rename sections -- linker script magic is enough. For
> > eg, the below pulls the EFI stub bss into .data for x86 without the need
> > for the annotations.
> >
> > diff --git a/arch/x86/boot/compressed/vmlinux.lds.S b/arch/x86/boot/compressed/vmlinux.lds.S
> > index 508cfa6828c5..e324819c95bc 100644
> > --- a/arch/x86/boot/compressed/vmlinux.lds.S
> > +++ b/arch/x86/boot/compressed/vmlinux.lds.S
> > @@ -52,6 +52,7 @@ SECTIONS
> >                 _data = . ;
> >                 *(.data)
> >                 *(.data.*)
> > +               drivers/firmware/efi/libstub/lib.a:(.bss .bss.*)
> >                 _edata = . ;
> >         }
> >         . = ALIGN(L1_CACHE_BYTES);
> 
> No, we can add this to ARM as well, and get rid of the
> __efistub_global annotations entirely.

Cool.

> 
> We'll still need .data.efistub for the .data pieces, but that is a
> separate issue.

You can avoid that by using an archive specification like above. i.e.
adding
	drivers/firmware/efi/libstub/lib.a:(.data .data.*)
to the .init.data output section will pull in just the .data input
sections from the EFI stub into the .init.data section.

Re: [PATCH 3/9] efi/x86: Move efi stub globals from .bss to .data

[Ard Biesheuvel]

On Fri, 10 Apr 2020 at 20:01, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> On Fri, Apr 10, 2020 at 06:03:38PM +0200, Ard Biesheuvel wrote:
> > On Fri, 10 Apr 2020 at 17:16, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> > >
> > > On Fri, Apr 10, 2020 at 10:20:42AM +0200, Ard Biesheuvel wrote:
> > > > On Thu, 9 Apr 2020 at 23:08, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> > > > >
> > > > > On Thu, Apr 09, 2020 at 04:53:07PM -0400, Brian Gerst wrote:
> > > > > > > Can we use the -fno-zero-initialized-in-bss compiler flag instead of
> > > > > > > explicitly marking global variables?
> > > > > >
> > > > > > Scratch that.  Apparently it only works when a variable is explicitly
> > > > > > initialized to zero.
> > > > > >
> > > > > > --
> > > > > > Brian Gerst
> > > > >
> > > > > Right, there doesn't seem to be a compiler option to turn off the use of
> > > > > .bss altogether.
> > > >
> > > > Yeah. I'll try to come up with a way to consolidate this a bit across
> > > > architectures (which is a bit easier now that all of the EFI stub C
> > > > code lives in the same place). It is probably easiest to use a section
> > > > renaming trick similar to the one I added for ARM (as Arvind suggested
> > > > as well, IIRC), and get rid of the per-symbol annotations altogether.
> > >
> > > Does that work for 32-bit ARM, or does it need to be .data to tell the
> > > compiler to avoid generating GOT references? If that's fine, we don't
> > > actually need to rename sections -- linker script magic is enough. For
> > > eg, the below pulls the EFI stub bss into .data for x86 without the need
> > > for the annotations.
> > >
> > > diff --git a/arch/x86/boot/compressed/vmlinux.lds.S b/arch/x86/boot/compressed/vmlinux.lds.S
> > > index 508cfa6828c5..e324819c95bc 100644
> > > --- a/arch/x86/boot/compressed/vmlinux.lds.S
> > > +++ b/arch/x86/boot/compressed/vmlinux.lds.S
> > > @@ -52,6 +52,7 @@ SECTIONS
> > >                 _data = . ;
> > >                 *(.data)
> > >                 *(.data.*)
> > > +               drivers/firmware/efi/libstub/lib.a:(.bss .bss.*)
> > >                 _edata = . ;
> > >         }
> > >         . = ALIGN(L1_CACHE_BYTES);
> >
> > No, we can add this to ARM as well, and get rid of the
> > __efistub_global annotations entirely.
>
> Cool.
>
> >
> > We'll still need .data.efistub for the .data pieces, but that is a
> > separate issue.
>
> You can avoid that by using an archive specification like above. i.e.
> adding
>         drivers/firmware/efi/libstub/lib.a:(.data .data.*)
> to the .init.data output section will pull in just the .data input
> sections from the EFI stub into the .init.data section.

Sure. But the ARM decompressor linker script currently discards .data
before this point in the linker script, and relies on this as a safety
net to ensure that no new .data items get added to the decompressor
binary (which runs after the stub)

Re: [PATCH 3/9] efi/x86: Move efi stub globals from .bss to .data

[Arvind Sankar]

On Fri, Apr 10, 2020 at 08:03:15PM +0200, Ard Biesheuvel wrote:
> On Fri, 10 Apr 2020 at 20:01, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> >
> > On Fri, Apr 10, 2020 at 06:03:38PM +0200, Ard Biesheuvel wrote:
> > > On Fri, 10 Apr 2020 at 17:16, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> > > >
> > > > On Fri, Apr 10, 2020 at 10:20:42AM +0200, Ard Biesheuvel wrote:
> > > > > On Thu, 9 Apr 2020 at 23:08, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> > > > > >
> > > > > > On Thu, Apr 09, 2020 at 04:53:07PM -0400, Brian Gerst wrote:
> > > > > > > > Can we use the -fno-zero-initialized-in-bss compiler flag instead of
> > > > > > > > explicitly marking global variables?
> > > > > > >
> > > > > > > Scratch that.  Apparently it only works when a variable is explicitly
> > > > > > > initialized to zero.
> > > > > > >
> > > > > > > --
> > > > > > > Brian Gerst
> > > > > >
> > > > > > Right, there doesn't seem to be a compiler option to turn off the use of
> > > > > > .bss altogether.
> > > > >
> > > > > Yeah. I'll try to come up with a way to consolidate this a bit across
> > > > > architectures (which is a bit easier now that all of the EFI stub C
> > > > > code lives in the same place). It is probably easiest to use a section
> > > > > renaming trick similar to the one I added for ARM (as Arvind suggested
> > > > > as well, IIRC), and get rid of the per-symbol annotations altogether.
> > > >
> > > > Does that work for 32-bit ARM, or does it need to be .data to tell the
> > > > compiler to avoid generating GOT references? If that's fine, we don't
> > > > actually need to rename sections -- linker script magic is enough. For
> > > > eg, the below pulls the EFI stub bss into .data for x86 without the need
> > > > for the annotations.
> > > >
> > > > diff --git a/arch/x86/boot/compressed/vmlinux.lds.S b/arch/x86/boot/compressed/vmlinux.lds.S
> > > > index 508cfa6828c5..e324819c95bc 100644
> > > > --- a/arch/x86/boot/compressed/vmlinux.lds.S
> > > > +++ b/arch/x86/boot/compressed/vmlinux.lds.S
> > > > @@ -52,6 +52,7 @@ SECTIONS
> > > >                 _data = . ;
> > > >                 *(.data)
> > > >                 *(.data.*)
> > > > +               drivers/firmware/efi/libstub/lib.a:(.bss .bss.*)
> > > >                 _edata = . ;
> > > >         }
> > > >         . = ALIGN(L1_CACHE_BYTES);
> > >
> > > No, we can add this to ARM as well, and get rid of the
> > > __efistub_global annotations entirely.
> >
> > Cool.
> >
> > >
> > > We'll still need .data.efistub for the .data pieces, but that is a
> > > separate issue.
> >
> > You can avoid that by using an archive specification like above. i.e.
> > adding
> >         drivers/firmware/efi/libstub/lib.a:(.data .data.*)
> > to the .init.data output section will pull in just the .data input
> > sections from the EFI stub into the .init.data section.
> 
> Sure. But the ARM decompressor linker script currently discards .data
> before this point in the linker script, and relies on this as a safety
> net to ensure that no new .data items get added to the decompressor
> binary (which runs after the stub)

Ah.

Re: [PATCH 3/9] efi/x86: Move efi stub globals from .bss to .data

[Arvind Sankar]

On Fri, Apr 10, 2020 at 08:03:15PM +0200, Ard Biesheuvel wrote:
> > >
> > > We'll still need .data.efistub for the .data pieces, but that is a
> > > separate issue.
> >
> > You can avoid that by using an archive specification like above. i.e.
> > adding
> >         drivers/firmware/efi/libstub/lib.a:(.data .data.*)
> > to the .init.data output section will pull in just the .data input
> > sections from the EFI stub into the .init.data section.
> 
> Sure. But the ARM decompressor linker script currently discards .data
> before this point in the linker script, and relies on this as a safety
> net to ensure that no new .data items get added to the decompressor
> binary (which runs after the stub)

You should be able to use EXCLUDE_FILE to skip discarding the .data
section from libstub. That also supports the archive: syntax according
to the docs though I haven't tried it.

[PATCH 4/9] efi/x86: Always relocate the kernel for EFI handover entry

[Ard Biesheuvel]

From: Arvind Sankar <nivedita@alum.mit.edu>

Commit

  d5cdf4cfeac9 ("efi/x86: Don't relocate the kernel unless necessary")

tries to avoid relocating the kernel in the EFI stub as far as possible.

However, when systemd-boot is used to boot a unified kernel image [1],
the image is constructed by embedding the bzImage as a .linux section in
a PE executable that contains a small stub loader from systemd that will
call the EFI stub handover entry, together with additional sections and
potentially an initrd. When this image is constructed, by for example
dracut, the initrd is placed after the bzImage without ensuring that at
least init_size bytes are available for the bzImage. If the kernel is
not relocated by the EFI stub, this could result in the compressed
kernel's startup code in head_{32,64}.S overwriting the initrd.

To prevent this, unconditionally relocate the kernel if the EFI stub was
entered via the handover entry point.

[1] https://systemd.io/BOOT_LOADER_SPECIFICATION/#type-2-efi-unified-kernel-images

Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
Reported-by: Sergey Shatunov <me@prok.pw>
Fixes: d5cdf4cfeac9 ("efi/x86: Don't relocate the kernel unless necessary")
Link: https://lore.kernel.org/r/20200406180614.429454-2-nivedita@alum.mit.edu
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 drivers/firmware/efi/libstub/x86-stub.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
index 867a57e28980..05ccb229fb45 100644
--- a/drivers/firmware/efi/libstub/x86-stub.c
+++ b/drivers/firmware/efi/libstub/x86-stub.c
@@ -740,8 +740,15 @@ unsigned long efi_main(efi_handle_t handle,
 	 * now use KERNEL_IMAGE_SIZE, which will be 512MiB, the same as what
 	 * KASLR uses.
 	 *
-	 * Also relocate it if image_offset is zero, i.e. we weren't loaded by
-	 * LoadImage, but we are not aligned correctly.
+	 * Also relocate it if image_offset is zero, i.e. the kernel wasn't
+	 * loaded by LoadImage, but rather by a bootloader that called the
+	 * handover entry. The reason we must always relocate in this case is
+	 * to handle the case of systemd-boot booting a unified kernel image,
+	 * which is a PE executable that contains the bzImage and an initrd as
+	 * COFF sections. The initrd section is placed after the bzImage
+	 * without ensuring that there are at least init_size bytes available
+	 * for the bzImage, and thus the compressed kernel's startup code may
+	 * overwrite the initrd unless it is moved out of the way.
 	 */
 
 	buffer_start = ALIGN(bzimage_addr - image_offset,
@@ -751,8 +758,7 @@ unsigned long efi_main(efi_handle_t handle,
 	if ((buffer_start < LOAD_PHYSICAL_ADDR)				     ||
 	    (IS_ENABLED(CONFIG_X86_32) && buffer_end > KERNEL_IMAGE_SIZE)    ||
 	    (IS_ENABLED(CONFIG_X86_64) && buffer_end > MAXMEM_X86_64_4LEVEL) ||
-	    (image_offset == 0 && !IS_ALIGNED(bzimage_addr,
-					      hdr->kernel_alignment))) {
+	    (image_offset == 0)) {
 		status = efi_relocate_kernel(&bzimage_addr,
 					     hdr->init_size, hdr->init_size,
 					     hdr->pref_address,
-- 
2.17.1

[PATCH 5/9] efi/arm: Deal with ADR going out of range in efi_enter_kernel()

[Ard Biesheuvel]

Commit

  0698fac4ac2a ("efi/arm: Clean EFI stub exit code from cache instead of avoiding it")

introduced a PC-relative reference to 'call_cache_fn' into
efi_enter_kernel(), which lives way at the end of head.S. In some cases,
the ARM version of the ADR instruction does not have sufficient range,
resulting in a build error:

  arch/arm/boot/compressed/head.S:1453: Error: invalid constant (fffffffffffffbe4) after fixup

ARM defines an alternative with a wider range, called ADRL, but this does
not exist for Thumb-2. At the same time, the ADR instruction in Thumb-2
has a wider range, and so it does not suffer from the same issue.

So let's switch to ADRL for ARM builds, and keep the ADR for Thumb-2 builds.

Reported-by: Arnd Bergmann <arnd@arndb.de>
Tested-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/arm/boot/compressed/head.S | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/compressed/head.S b/arch/arm/boot/compressed/head.S
index 04f77214f050..61e6ee3ba75f 100644
--- a/arch/arm/boot/compressed/head.S
+++ b/arch/arm/boot/compressed/head.S
@@ -1454,7 +1454,8 @@ ENTRY(efi_enter_kernel)
 		@ running beyond the PoU, and so calling cache_off below from
 		@ inside the PE/COFF loader allocated region is unsafe unless
 		@ we explicitly clean it to the PoC.
-		adr	r0, call_cache_fn		@ region of code we will
+ ARM(		adrl	r0, call_cache_fn	)
+ THUMB(		adr	r0, call_cache_fn	)	@ region of code we will
 		adr	r1, 0f				@ run with MMU off
 		bl	cache_clean_flush
 		bl	cache_off
-- 
2.17.1

[tip: efi/urgent] efi/arm: Deal with ADR going out of range in efi_enter_kernel()

[tip-bot2 for Ard Biesheuvel]

The following commit has been merged into the efi/urgent branch of tip:

Commit-ID:     a94691680bace7e1404e4f235badb74e30467e86
Gitweb:        https://git.kernel.org/tip/a94691680bace7e1404e4f235badb74e30467e86
Author:        Ard Biesheuvel <ardb@kernel.org>
AuthorDate:    Thu, 09 Apr 2020 15:04:30 +02:00
Committer:     Ingo Molnar <mingo@kernel.org>
CommitterDate: Tue, 14 Apr 2020 08:32:14 +02:00

efi/arm: Deal with ADR going out of range in efi_enter_kernel()

Commit

  0698fac4ac2a ("efi/arm: Clean EFI stub exit code from cache instead of avoiding it")

introduced a PC-relative reference to 'call_cache_fn' into
efi_enter_kernel(), which lives way at the end of head.S. In some cases,
the ARM version of the ADR instruction does not have sufficient range,
resulting in a build error:

  arch/arm/boot/compressed/head.S:1453: Error: invalid constant (fffffffffffffbe4) after fixup

ARM defines an alternative with a wider range, called ADRL, but this does
not exist for Thumb-2. At the same time, the ADR instruction in Thumb-2
has a wider range, and so it does not suffer from the same issue.

So let's switch to ADRL for ARM builds, and keep the ADR for Thumb-2 builds.

Reported-by: Arnd Bergmann <arnd@arndb.de>
Tested-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Link: https://lore.kernel.org/r/20200409130434.6736-6-ardb@kernel.org
---
 arch/arm/boot/compressed/head.S | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/compressed/head.S b/arch/arm/boot/compressed/head.S
index cabdd8f..e8e1c86 100644
--- a/arch/arm/boot/compressed/head.S
+++ b/arch/arm/boot/compressed/head.S
@@ -1450,7 +1450,8 @@ ENTRY(efi_enter_kernel)
 		@ running beyond the PoU, and so calling cache_off below from
 		@ inside the PE/COFF loader allocated region is unsafe unless
 		@ we explicitly clean it to the PoC.
-		adr	r0, call_cache_fn		@ region of code we will
+ ARM(		adrl	r0, call_cache_fn	)
+ THUMB(		adr	r0, call_cache_fn	)	@ region of code we will
 		adr	r1, 0f				@ run with MMU off
 		bl	cache_clean_flush
 		bl	cache_off

[PATCH 6/9] Documentation: efi/x86: clarify EFI handover protocol and its requirements

[Ard Biesheuvel]

The EFI handover protocol was introduced on x86 to permit the boot
loader to pass a populated boot_params structure as an additional
function argument to the entry point. This allows the bootloader to
pass the base and size of a initrd image, which is more flexible
than relying on the EFI stub's file I/O routines, which can only
access the file system from which the kernel image itself was loaded
from firmware.

This approach requires a fair amount of internal knowledge regarding
the layout of the boot_params structure on the part of the boot loader,
as well as knowledge regarding the allowed placement of the initrd in
memory, and so it has been deprecated in favour of a new initrd loading
method that is based on existing UEFI protocols and best practices.

So update the x86 boot protocol documentation to clarify that the EFI
handover protocol has been deprecated, and while at it, add a note that
invoking the EFI handover protocol still requires the PE/COFF image to
be loaded properly (as opposed to simply being copied into memory).
Also, drop the code32_start header field from the list of values that
need to be provided, as this is no longer required.

Reviewed-by: Borislav Petkov <bp@suse.de>
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 Documentation/x86/boot.rst | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/Documentation/x86/boot.rst b/Documentation/x86/boot.rst
index fa7ddc0428c8..5325c71ca877 100644
--- a/Documentation/x86/boot.rst
+++ b/Documentation/x86/boot.rst
@@ -1399,8 +1399,8 @@ must have read/write permission; CS must be __BOOT_CS and DS, ES, SS
 must be __BOOT_DS; interrupt must be disabled; %rsi must hold the base
 address of the struct boot_params.
 
-EFI Handover Protocol
-=====================
+EFI Handover Protocol (deprecated)
+==================================
 
 This protocol allows boot loaders to defer initialisation to the EFI
 boot stub. The boot loader is required to load the kernel/initrd(s)
@@ -1408,6 +1408,12 @@ from the boot media and jump to the EFI handover protocol entry point
 which is hdr->handover_offset bytes from the beginning of
 startup_{32,64}.
 
+The boot loader MUST respect the kernel's PE/COFF metadata when it comes
+to section alignment, the memory footprint of the executable image beyond
+the size of the file itself, and any other aspect of the PE/COFF header
+that may affect correct operation of the image as a PE/COFF binary in the
+execution context provided by the EFI firmware.
+
 The function prototype for the handover entry point looks like this::
 
     efi_main(void *handle, efi_system_table_t *table, struct boot_params *bp)
@@ -1419,9 +1425,18 @@ UEFI specification. 'bp' is the boot loader-allocated boot params.
 
 The boot loader *must* fill out the following fields in bp::
 
-  - hdr.code32_start
   - hdr.cmd_line_ptr
   - hdr.ramdisk_image (if applicable)
   - hdr.ramdisk_size  (if applicable)
 
 All other fields should be zero.
+
+NOTE: The EFI Handover Protocol is deprecated in favour of the ordinary PE/COFF
+      entry point, combined with the LINUX_EFI_INITRD_MEDIA_GUID based initrd
+      loading protocol (refer to [0] for an example of the bootloader side of
+      this), which removes the need for any knowledge on the part of the EFI
+      bootloader regarding the internal representation of boot_params or any
+      requirements/limitations regarding the placement of the command line
+      and ramdisk in memory, or the placement of the kernel image itself.
+
+[0] https://github.com/u-boot/u-boot/commit/ec80b4735a593961fe701cc3a5d717d4739b0fd0
-- 
2.17.1

[tip: efi/urgent] Documentation/x86, efi/x86: Clarify EFI handover protocol and its requirements

[tip-bot2 for Ard Biesheuvel]

The following commit has been merged into the efi/urgent branch of tip:

Commit-ID:     8b84769a7a1505b279b337dae83d16390e83f5c1
Gitweb:        https://git.kernel.org/tip/8b84769a7a1505b279b337dae83d16390e83f5c1
Author:        Ard Biesheuvel <ardb@kernel.org>
AuthorDate:    Thu, 09 Apr 2020 15:04:31 +02:00
Committer:     Ingo Molnar <mingo@kernel.org>
CommitterDate: Tue, 14 Apr 2020 08:32:15 +02:00

Documentation/x86, efi/x86: Clarify EFI handover protocol and its requirements

The EFI handover protocol was introduced on x86 to permit the boot
loader to pass a populated boot_params structure as an additional
function argument to the entry point. This allows the bootloader to
pass the base and size of a initrd image, which is more flexible
than relying on the EFI stub's file I/O routines, which can only
access the file system from which the kernel image itself was loaded
from firmware.

This approach requires a fair amount of internal knowledge regarding
the layout of the boot_params structure on the part of the boot loader,
as well as knowledge regarding the allowed placement of the initrd in
memory, and so it has been deprecated in favour of a new initrd loading
method that is based on existing UEFI protocols and best practices.

So update the x86 boot protocol documentation to clarify that the EFI
handover protocol has been deprecated, and while at it, add a note that
invoking the EFI handover protocol still requires the PE/COFF image to
be loaded properly (as opposed to simply being copied into memory).
Also, drop the code32_start header field from the list of values that
need to be provided, as this is no longer required.

Reviewed-by: Borislav Petkov <bp@suse.de>
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Link: https://lore.kernel.org/r/20200409130434.6736-7-ardb@kernel.org
---
 Documentation/x86/boot.rst | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/Documentation/x86/boot.rst b/Documentation/x86/boot.rst
index fa7ddc0..5325c71 100644
--- a/Documentation/x86/boot.rst
+++ b/Documentation/x86/boot.rst
@@ -1399,8 +1399,8 @@ must have read/write permission; CS must be __BOOT_CS and DS, ES, SS
 must be __BOOT_DS; interrupt must be disabled; %rsi must hold the base
 address of the struct boot_params.
 
-EFI Handover Protocol
-=====================
+EFI Handover Protocol (deprecated)
+==================================
 
 This protocol allows boot loaders to defer initialisation to the EFI
 boot stub. The boot loader is required to load the kernel/initrd(s)
@@ -1408,6 +1408,12 @@ from the boot media and jump to the EFI handover protocol entry point
 which is hdr->handover_offset bytes from the beginning of
 startup_{32,64}.
 
+The boot loader MUST respect the kernel's PE/COFF metadata when it comes
+to section alignment, the memory footprint of the executable image beyond
+the size of the file itself, and any other aspect of the PE/COFF header
+that may affect correct operation of the image as a PE/COFF binary in the
+execution context provided by the EFI firmware.
+
 The function prototype for the handover entry point looks like this::
 
     efi_main(void *handle, efi_system_table_t *table, struct boot_params *bp)
@@ -1419,9 +1425,18 @@ UEFI specification. 'bp' is the boot loader-allocated boot params.
 
 The boot loader *must* fill out the following fields in bp::
 
-  - hdr.code32_start
   - hdr.cmd_line_ptr
   - hdr.ramdisk_image (if applicable)
   - hdr.ramdisk_size  (if applicable)
 
 All other fields should be zero.
+
+NOTE: The EFI Handover Protocol is deprecated in favour of the ordinary PE/COFF
+      entry point, combined with the LINUX_EFI_INITRD_MEDIA_GUID based initrd
+      loading protocol (refer to [0] for an example of the bootloader side of
+      this), which removes the need for any knowledge on the part of the EFI
+      bootloader regarding the internal representation of boot_params or any
+      requirements/limitations regarding the placement of the command line
+      and ramdisk in memory, or the placement of the kernel image itself.
+
+[0] https://github.com/u-boot/u-boot/commit/ec80b4735a593961fe701cc3a5d717d4739b0fd0

[PATCH 7/9] efi/libstub/file: merge filename buffers to reduce stack usage

[Ard Biesheuvel]

Arnd reports that commit

  9302c1bb8e47 ("efi/libstub: Rewrite file I/O routine")

reworks the file I/O routines in a way that triggers the following
warning:

  drivers/firmware/efi/libstub/file.c:240:1: warning: the frame size
            of 1200 bytes is larger than 1024 bytes [-Wframe-larger-than=]

We can work around this issue dropping an instance of efi_char16_t[256]
from the stack frame, and reusing the 'filename' field of the file info
struct that we use to obtain file information from EFI (which contains
the filename even though we already know it since we used it to open
the file in the first place)

Reported-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 drivers/firmware/efi/libstub/file.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/drivers/firmware/efi/libstub/file.c b/drivers/firmware/efi/libstub/file.c
index d4c7e5f59d2c..ea66b1f16a79 100644
--- a/drivers/firmware/efi/libstub/file.c
+++ b/drivers/firmware/efi/libstub/file.c
@@ -29,30 +29,31 @@
  */
 #define EFI_READ_CHUNK_SIZE	SZ_1M
 
+struct finfo {
+	efi_file_info_t info;
+	efi_char16_t	filename[MAX_FILENAME_SIZE];
+};
+
 static efi_status_t efi_open_file(efi_file_protocol_t *volume,
-				  efi_char16_t *filename_16,
+				  struct finfo *fi,
 				  efi_file_protocol_t **handle,
 				  unsigned long *file_size)
 {
-	struct {
-		efi_file_info_t info;
-		efi_char16_t	filename[MAX_FILENAME_SIZE];
-	} finfo;
 	efi_guid_t info_guid = EFI_FILE_INFO_ID;
 	efi_file_protocol_t *fh;
 	unsigned long info_sz;
 	efi_status_t status;
 
-	status = volume->open(volume, &fh, filename_16, EFI_FILE_MODE_READ, 0);
+	status = volume->open(volume, &fh, fi->filename, EFI_FILE_MODE_READ, 0);
 	if (status != EFI_SUCCESS) {
 		pr_efi_err("Failed to open file: ");
-		efi_char16_printk(filename_16);
+		efi_char16_printk(fi->filename);
 		efi_printk("\n");
 		return status;
 	}
 
-	info_sz = sizeof(finfo);
-	status = fh->get_info(fh, &info_guid, &info_sz, &finfo);
+	info_sz = sizeof(struct finfo);
+	status = fh->get_info(fh, &info_guid, &info_sz, fi);
 	if (status != EFI_SUCCESS) {
 		pr_efi_err("Failed to get file info\n");
 		fh->close(fh);
@@ -60,7 +61,7 @@ static efi_status_t efi_open_file(efi_file_protocol_t *volume,
 	}
 
 	*handle = fh;
-	*file_size = finfo.info.file_size;
+	*file_size = fi->info.file_size;
 	return EFI_SUCCESS;
 }
 
@@ -146,13 +147,13 @@ static efi_status_t handle_cmdline_files(efi_loaded_image_t *image,
 
 	alloc_addr = alloc_size = 0;
 	do {
-		efi_char16_t filename[MAX_FILENAME_SIZE];
+		struct finfo fi;
 		unsigned long size;
 		void *addr;
 
 		offset = find_file_option(cmdline, cmdline_len,
 					  optstr, optstr_size,
-					  filename, ARRAY_SIZE(filename));
+					  fi.filename, ARRAY_SIZE(fi.filename));
 
 		if (!offset)
 			break;
@@ -166,7 +167,7 @@ static efi_status_t handle_cmdline_files(efi_loaded_image_t *image,
 				return status;
 		}
 
-		status = efi_open_file(volume, filename, &file, &size);
+		status = efi_open_file(volume, &fi, &file, &size);
 		if (status != EFI_SUCCESS)
 			goto err_close_volume;
 
-- 
2.17.1

[tip: efi/urgent] efi/libstub/file: Merge file name buffers to reduce stack usage

[tip-bot2 for Ard Biesheuvel]

The following commit has been merged into the efi/urgent branch of tip:

Commit-ID:     464fb126d98a047953040cc9c754801dbda54e5d
Gitweb:        https://git.kernel.org/tip/464fb126d98a047953040cc9c754801dbda54e5d
Author:        Ard Biesheuvel <ardb@kernel.org>
AuthorDate:    Thu, 09 Apr 2020 15:04:32 +02:00
Committer:     Ingo Molnar <mingo@kernel.org>
CommitterDate: Tue, 14 Apr 2020 08:32:15 +02:00

efi/libstub/file: Merge file name buffers to reduce stack usage

Arnd reports that commit

  9302c1bb8e47 ("efi/libstub: Rewrite file I/O routine")

reworks the file I/O routines in a way that triggers the following
warning:

  drivers/firmware/efi/libstub/file.c:240:1: warning: the frame size
            of 1200 bytes is larger than 1024 bytes [-Wframe-larger-than=]

We can work around this issue dropping an instance of efi_char16_t[256]
from the stack frame, and reusing the 'filename' field of the file info
struct that we use to obtain file information from EFI (which contains
the file name even though we already know it since we used it to open
the file in the first place)

Reported-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Link: https://lore.kernel.org/r/20200409130434.6736-8-ardb@kernel.org
---
 drivers/firmware/efi/libstub/file.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/drivers/firmware/efi/libstub/file.c b/drivers/firmware/efi/libstub/file.c
index d4c7e5f..ea66b1f 100644
--- a/drivers/firmware/efi/libstub/file.c
+++ b/drivers/firmware/efi/libstub/file.c
@@ -29,30 +29,31 @@
  */
 #define EFI_READ_CHUNK_SIZE	SZ_1M
 
+struct finfo {
+	efi_file_info_t info;
+	efi_char16_t	filename[MAX_FILENAME_SIZE];
+};
+
 static efi_status_t efi_open_file(efi_file_protocol_t *volume,
-				  efi_char16_t *filename_16,
+				  struct finfo *fi,
 				  efi_file_protocol_t **handle,
 				  unsigned long *file_size)
 {
-	struct {
-		efi_file_info_t info;
-		efi_char16_t	filename[MAX_FILENAME_SIZE];
-	} finfo;
 	efi_guid_t info_guid = EFI_FILE_INFO_ID;
 	efi_file_protocol_t *fh;
 	unsigned long info_sz;
 	efi_status_t status;
 
-	status = volume->open(volume, &fh, filename_16, EFI_FILE_MODE_READ, 0);
+	status = volume->open(volume, &fh, fi->filename, EFI_FILE_MODE_READ, 0);
 	if (status != EFI_SUCCESS) {
 		pr_efi_err("Failed to open file: ");
-		efi_char16_printk(filename_16);
+		efi_char16_printk(fi->filename);
 		efi_printk("\n");
 		return status;
 	}
 
-	info_sz = sizeof(finfo);
-	status = fh->get_info(fh, &info_guid, &info_sz, &finfo);
+	info_sz = sizeof(struct finfo);
+	status = fh->get_info(fh, &info_guid, &info_sz, fi);
 	if (status != EFI_SUCCESS) {
 		pr_efi_err("Failed to get file info\n");
 		fh->close(fh);
@@ -60,7 +61,7 @@ static efi_status_t efi_open_file(efi_file_protocol_t *volume,
 	}
 
 	*handle = fh;
-	*file_size = finfo.info.file_size;
+	*file_size = fi->info.file_size;
 	return EFI_SUCCESS;
 }
 
@@ -146,13 +147,13 @@ static efi_status_t handle_cmdline_files(efi_loaded_image_t *image,
 
 	alloc_addr = alloc_size = 0;
 	do {
-		efi_char16_t filename[MAX_FILENAME_SIZE];
+		struct finfo fi;
 		unsigned long size;
 		void *addr;
 
 		offset = find_file_option(cmdline, cmdline_len,
 					  optstr, optstr_size,
-					  filename, ARRAY_SIZE(filename));
+					  fi.filename, ARRAY_SIZE(fi.filename));
 
 		if (!offset)
 			break;
@@ -166,7 +167,7 @@ static efi_status_t handle_cmdline_files(efi_loaded_image_t *image,
 				return status;
 		}
 
-		status = efi_open_file(volume, filename, &file, &size);
+		status = efi_open_file(volume, &fi, &file, &size);
 		if (status != EFI_SUCCESS)
 			goto err_close_volume;
 

[PATCH 8/9] efi/x86: Fix the deletion of variables in mixed mode

[Ard Biesheuvel]

From: Gary Lin <glin@suse.com>

efi_thunk_set_variable() treated the NULL "data" pointer as an invalid
parameter, and this broke the deletion of variables in mixed mode.
This commit fixes the check of data so that the userspace program can
delete a variable in mixed mode.

Fixes: 8319e9d5ad98ffcc ("efi/x86: Handle by-ref arguments covering multiple pages in mixed mode")
Cc: linux-efi@vger.kernel.org
Cc: Ard Biesheuvel <ardb@kernel.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Gary Lin <glin@suse.com>
Link: https://lore.kernel.org/r/20200408081606.1504-1-glin@suse.com
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/x86/platform/efi/efi_64.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
index 211bb9358b73..e0e2e8136cf5 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -638,7 +638,7 @@ efi_thunk_set_variable(efi_char16_t *name, efi_guid_t *vendor,
 	phys_vendor = virt_to_phys_or_null(vnd);
 	phys_data = virt_to_phys_or_null_size(data, data_size);
 
-	if (!phys_name || !phys_data)
+	if (!phys_name || (data && !phys_data))
 		status = EFI_INVALID_PARAMETER;
 	else
 		status = efi_thunk(set_variable, phys_name, phys_vendor,
@@ -669,7 +669,7 @@ efi_thunk_set_variable_nonblocking(efi_char16_t *name, efi_guid_t *vendor,
 	phys_vendor = virt_to_phys_or_null(vnd);
 	phys_data = virt_to_phys_or_null_size(data, data_size);
 
-	if (!phys_name || !phys_data)
+	if (!phys_name || (data && !phys_data))
 		status = EFI_INVALID_PARAMETER;
 	else
 		status = efi_thunk(set_variable, phys_name, phys_vendor,
-- 
2.17.1

[PATCH 9/9] efi/x86: Don't remap text<->rodata gap read-only for mixed mode

[Ard Biesheuvel]

Commit

  d9e3d2c4f10320 ("efi/x86: Don't map the entire kernel text RW for mixed mode")

updated the code that creates the 1:1 memory mapping to use read-only
attributes for the 1:1 alias of the kernel's text and rodata sections, to
protect it from inadvertent modification. However, it failed to take into
account that the unused gap between text and rodata is given to the page
allocator for general use.

If the vmap'ed stack happens to be allocated from this region, any by-ref
output arguments passed to EFI runtime services that are allocated on the
stack (such as the 'datasize' argument taken by GetVariable() when invoked
from efivar_entry_size()) will be referenced via a read-only mapping,
resulting in a page fault if the EFI code tries to write to it:

  BUG: unable to handle page fault for address: 00000000386aae88
  #PF: supervisor write access in kernel mode
  #PF: error_code(0x0003) - permissions violation
  PGD fd61063 P4D fd61063 PUD fd62063 PMD 386000e1
  Oops: 0003 [#1] SMP PTI
  CPU: 2 PID: 255 Comm: systemd-sysv-ge Not tainted 5.6.0-rc4-default+ #22
  Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 0.0.0 02/06/2015
  RIP: 0008:0x3eaeed95
  Code: ...  <89> 03 be 05 00 00 80 a1 74 63 b1 3e 83 c0 48 e8 44 d2 ff ff eb 05
  RSP: 0018:000000000fd73fa0 EFLAGS: 00010002
  RAX: 0000000000000001 RBX: 00000000386aae88 RCX: 000000003e9f1120
  RDX: 0000000000000001 RSI: 0000000000000000 RDI: 0000000000000001
  RBP: 000000000fd73fd8 R08: 00000000386aae88 R09: 0000000000000000
  R10: 0000000000000002 R11: 0000000000000000 R12: 0000000000000000
  R13: ffffc0f040220000 R14: 0000000000000000 R15: 0000000000000000
  FS:  00007f21160ac940(0000) GS:ffff9cf23d500000(0000) knlGS:0000000000000000
  CS:  0008 DS: 0018 ES: 0018 CR0: 0000000080050033
  CR2: 00000000386aae88 CR3: 000000000fd6c004 CR4: 00000000003606e0
  DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
  DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
  Call Trace:
  Modules linked in:
  CR2: 00000000386aae88
  ---[ end trace a8bfbd202e712834 ]---

Let's fix this by remapping text and rodata individually, and leave the
gaps mapped read-write.

Fixes: d9e3d2c4f10320 ("efi/x86: Don't map the entire kernel text RW for mixed mode")
Reported-by: Jiri Slaby <jslaby@suse.cz>
Tested-by: Jiri Slaby <jslaby@suse.cz>
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/x86/platform/efi/efi_64.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
index e0e2e8136cf5..c5e393f8bb3f 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -202,7 +202,7 @@ virt_to_phys_or_null_size(void *va, unsigned long size)
 
 int __init efi_setup_page_tables(unsigned long pa_memmap, unsigned num_pages)
 {
-	unsigned long pfn, text, pf;
+	unsigned long pfn, text, pf, rodata;
 	struct page *page;
 	unsigned npages;
 	pgd_t *pgd = efi_mm.pgd;
@@ -256,7 +256,7 @@ int __init efi_setup_page_tables(unsigned long pa_memmap, unsigned num_pages)
 
 	efi_scratch.phys_stack = page_to_phys(page + 1); /* stack grows down */
 
-	npages = (__end_rodata_aligned - _text) >> PAGE_SHIFT;
+	npages = (_etext - _text) >> PAGE_SHIFT;
 	text = __pa(_text);
 	pfn = text >> PAGE_SHIFT;
 
@@ -266,6 +266,14 @@ int __init efi_setup_page_tables(unsigned long pa_memmap, unsigned num_pages)
 		return 1;
 	}
 
+	npages = (__end_rodata - __start_rodata) >> PAGE_SHIFT;
+	rodata = __pa(__start_rodata);
+	pfn = rodata >> PAGE_SHIFT;
+	if (kernel_map_pages_in_pgd(pgd, pfn, rodata, npages, pf)) {
+		pr_err("Failed to map kernel rodata 1:1\n");
+		return 1;
+	}
+
 	return 0;
 }
 
-- 
2.17.1

[tip: efi/urgent] efi/x86: Don't remap text<->rodata gap read-only for mixed mode

[tip-bot2 for Ard Biesheuvel]

The following commit has been merged into the efi/urgent branch of tip:

Commit-ID:     f6103162008dfd37567f240b50e5e1ea7cf2e00c
Gitweb:        https://git.kernel.org/tip/f6103162008dfd37567f240b50e5e1ea7cf2e00c
Author:        Ard Biesheuvel <ardb@kernel.org>
AuthorDate:    Thu, 09 Apr 2020 15:04:34 +02:00
Committer:     Ingo Molnar <mingo@kernel.org>
CommitterDate: Tue, 14 Apr 2020 08:32:17 +02:00

efi/x86: Don't remap text<->rodata gap read-only for mixed mode

Commit

  d9e3d2c4f10320 ("efi/x86: Don't map the entire kernel text RW for mixed mode")

updated the code that creates the 1:1 memory mapping to use read-only
attributes for the 1:1 alias of the kernel's text and rodata sections, to
protect it from inadvertent modification. However, it failed to take into
account that the unused gap between text and rodata is given to the page
allocator for general use.

If the vmap'ed stack happens to be allocated from this region, any by-ref
output arguments passed to EFI runtime services that are allocated on the
stack (such as the 'datasize' argument taken by GetVariable() when invoked
from efivar_entry_size()) will be referenced via a read-only mapping,
resulting in a page fault if the EFI code tries to write to it:

  BUG: unable to handle page fault for address: 00000000386aae88
  #PF: supervisor write access in kernel mode
  #PF: error_code(0x0003) - permissions violation
  PGD fd61063 P4D fd61063 PUD fd62063 PMD 386000e1
  Oops: 0003 [#1] SMP PTI
  CPU: 2 PID: 255 Comm: systemd-sysv-ge Not tainted 5.6.0-rc4-default+ #22
  Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 0.0.0 02/06/2015
  RIP: 0008:0x3eaeed95
  Code: ...  <89> 03 be 05 00 00 80 a1 74 63 b1 3e 83 c0 48 e8 44 d2 ff ff eb 05
  RSP: 0018:000000000fd73fa0 EFLAGS: 00010002
  RAX: 0000000000000001 RBX: 00000000386aae88 RCX: 000000003e9f1120
  RDX: 0000000000000001 RSI: 0000000000000000 RDI: 0000000000000001
  RBP: 000000000fd73fd8 R08: 00000000386aae88 R09: 0000000000000000
  R10: 0000000000000002 R11: 0000000000000000 R12: 0000000000000000
  R13: ffffc0f040220000 R14: 0000000000000000 R15: 0000000000000000
  FS:  00007f21160ac940(0000) GS:ffff9cf23d500000(0000) knlGS:0000000000000000
  CS:  0008 DS: 0018 ES: 0018 CR0: 0000000080050033
  CR2: 00000000386aae88 CR3: 000000000fd6c004 CR4: 00000000003606e0
  DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
  DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
  Call Trace:
  Modules linked in:
  CR2: 00000000386aae88
  ---[ end trace a8bfbd202e712834 ]---

Let's fix this by remapping text and rodata individually, and leave the
gaps mapped read-write.

Fixes: d9e3d2c4f10320 ("efi/x86: Don't map the entire kernel text RW for mixed mode")
Reported-by: Jiri Slaby <jslaby@suse.cz>
Tested-by: Jiri Slaby <jslaby@suse.cz>
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Link: https://lore.kernel.org/r/20200409130434.6736-10-ardb@kernel.org
---
 arch/x86/platform/efi/efi_64.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
index e0e2e81..c5e393f 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -202,7 +202,7 @@ virt_to_phys_or_null_size(void *va, unsigned long size)
 
 int __init efi_setup_page_tables(unsigned long pa_memmap, unsigned num_pages)
 {
-	unsigned long pfn, text, pf;
+	unsigned long pfn, text, pf, rodata;
 	struct page *page;
 	unsigned npages;
 	pgd_t *pgd = efi_mm.pgd;
@@ -256,7 +256,7 @@ int __init efi_setup_page_tables(unsigned long pa_memmap, unsigned num_pages)
 
 	efi_scratch.phys_stack = page_to_phys(page + 1); /* stack grows down */
 
-	npages = (__end_rodata_aligned - _text) >> PAGE_SHIFT;
+	npages = (_etext - _text) >> PAGE_SHIFT;
 	text = __pa(_text);
 	pfn = text >> PAGE_SHIFT;
 
@@ -266,6 +266,14 @@ int __init efi_setup_page_tables(unsigned long pa_memmap, unsigned num_pages)
 		return 1;
 	}
 
+	npages = (__end_rodata - __start_rodata) >> PAGE_SHIFT;
+	rodata = __pa(__start_rodata);
+	pfn = rodata >> PAGE_SHIFT;
+	if (kernel_map_pages_in_pgd(pgd, pfn, rodata, npages, pf)) {
+		pr_err("Failed to map kernel rodata 1:1\n");
+		return 1;
+	}
+
 	return 0;
 }
 

Re: [GIT PULL 0/9] EFI fixes for v5.7-rc

[Theodore Y. Ts'o]

On Thu, Apr 09, 2020 at 03:04:25PM +0200, Ard Biesheuvel wrote:
> The following changes since commit 594e576d4b93b8cda3247542366b47e1b2ddc4dc:
> 
>   efi/libstub/arm: Fix spurious message that an initrd was loaded (2020-03-29 12:08:18 +0200)
> 
> are available in the Git repository at:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git tags/efi-urgent

Hi Ard,

By any chance does this series fix a kexec failure which I bisected
down to 0a67361dcdaa ("efi/x86: Remove runtime table address from
kexec EFI setup data")?   Or if it doesn't, is this a known failure?

I'm currently building Linus's latest branch to see if it's been fixed
since v5.6-11114-g9c94b39560c3 (which is where I first noticed it) and
while I was waiting for v5.6-12349-g87ebc45d2d32 to finish building so
I could test it, I noticed these patches, and so I figured I'd fire
off this quick question.

Thanks,

							- Ted

Re: [GIT PULL 0/9] EFI fixes for v5.7-rc

[Ard Biesheuvel]

On Thu, 9 Apr 2020 at 21:01, Theodore Y. Ts'o <tytso@mit.edu> wrote:
>
> On Thu, Apr 09, 2020 at 03:04:25PM +0200, Ard Biesheuvel wrote:
> > The following changes since commit 594e576d4b93b8cda3247542366b47e1b2ddc4dc:
> >
> >   efi/libstub/arm: Fix spurious message that an initrd was loaded (2020-03-29 12:08:18 +0200)
> >
> > are available in the Git repository at:
> >
> >   git://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git tags/efi-urgent
>
> Hi Ard,
>
> By any chance does this series fix a kexec failure which I bisected
> down to 0a67361dcdaa ("efi/x86: Remove runtime table address from
> kexec EFI setup data")?   Or if it doesn't, is this a known failure?
>

Hi Ted,

I wasn't aware of this issue, and this series will most likely not fix it.

> I'm currently building Linus's latest branch to see if it's been fixed
> since v5.6-11114-g9c94b39560c3 (which is where I first noticed it) and
> while I was waiting for v5.6-12349-g87ebc45d2d32 to finish building so
> I could test it, I noticed these patches, and so I figured I'd fire
> off this quick question.
>

I think we might be able to downright revert that patch if the
underlying assumption on my part is inaccurate, which was that the
fact that the boot code no longer uses the runtime table address
implies that there is no longer a reason to pass it.

Please involve me in the discussions on this issue.

Re: [GIT PULL 0/9] EFI fixes for v5.7-rc

[Theodore Y. Ts'o]

On Thu, Apr 09, 2020 at 09:04:42PM +0200, Ard Biesheuvel wrote:
> 
> > I'm currently building Linus's latest branch to see if it's been fixed
> > since v5.6-11114-g9c94b39560c3 (which is where I first noticed it) and
> > while I was waiting for v5.6-12349-g87ebc45d2d32 to finish building so
> > I could test it, I noticed these patches, and so I figured I'd fire
> > off this quick question.
> >
> 
> I think we might be able to downright revert that patch if the
> underlying assumption on my part is inaccurate, which was that the
> fact that the boot code no longer uses the runtime table address
> implies that there is no longer a reason to pass it.

Unfortunately, it doesn't cleanly revert, which is why I started
checking to see if it might be fixed already before trying to figure
out how to do a manual revert.  I just tested and the tip of Linus's
tree still has the failure.

The short description of the failure: I'm using Debian Stable (Buster)
with a 4.19 distro kernel and kexec-tools 2.0.18 to kexec into the
kernel under test.  I'm using a Google Compute Engine VM, and the
actual kexec command is here:

https://github.com/tytso/xfstests-bld/blob/master/kvm-xfstests/test-appliance/files/usr/local/lib/gce-kexec#L146

What happens is that the kexec'ed kernel immediately crashes, at which
point we drop back into the BIOS, and then it boots the Debain 4.19.0
distro kernel instead of the kernel to be tested boot.  Since we lose
the boot command line that was used from the kexec, the gce-xfstests
image retries the kexec, which fails, and the failing kexec repeats
until I manually kill the VM.

The bisect fingred v5.6-rc1-59-g0a67361dcdaa ("efi/x86: Remove runtime
table address from kexec EFI setup data") as the first failing commit.
Its immediate parent commit, v5.6-rc1-58-g06c0bd93434c works just
fine.

Is there any further debugging information that would be useful?

Thanks,

						- Ted

Re: [GIT PULL 0/9] EFI fixes for v5.7-rc

[Ard Biesheuvel]

On Thu, 9 Apr 2020 at 22:16, Theodore Y. Ts'o <tytso@mit.edu> wrote:
>
> On Thu, Apr 09, 2020 at 09:04:42PM +0200, Ard Biesheuvel wrote:
> >
> > > I'm currently building Linus's latest branch to see if it's been fixed
> > > since v5.6-11114-g9c94b39560c3 (which is where I first noticed it) and
> > > while I was waiting for v5.6-12349-g87ebc45d2d32 to finish building so
> > > I could test it, I noticed these patches, and so I figured I'd fire
> > > off this quick question.
> > >
> >
> > I think we might be able to downright revert that patch if the
> > underlying assumption on my part is inaccurate, which was that the
> > fact that the boot code no longer uses the runtime table address
> > implies that there is no longer a reason to pass it.
>
> Unfortunately, it doesn't cleanly revert, which is why I started
> checking to see if it might be fixed already before trying to figure
> out how to do a manual revert.  I just tested and the tip of Linus's
> tree still has the failure.
>
> The short description of the failure: I'm using Debian Stable (Buster)
> with a 4.19 distro kernel and kexec-tools 2.0.18 to kexec into the
> kernel under test.  I'm using a Google Compute Engine VM, and the
> actual kexec command is here:
>
> https://github.com/tytso/xfstests-bld/blob/master/kvm-xfstests/test-appliance/files/usr/local/lib/gce-kexec#L146
>
> What happens is that the kexec'ed kernel immediately crashes, at which
> point we drop back into the BIOS, and then it boots the Debain 4.19.0
> distro kernel instead of the kernel to be tested boot.  Since we lose
> the boot command line that was used from the kexec, the gce-xfstests
> image retries the kexec, which fails, and the failing kexec repeats
> until I manually kill the VM.
>
> The bisect fingred v5.6-rc1-59-g0a67361dcdaa ("efi/x86: Remove runtime
> table address from kexec EFI setup data") as the first failing commit.
> Its immediate parent commit, v5.6-rc1-58-g06c0bd93434c works just
> fine.
>
> Is there any further debugging information that would be useful?
>

Does this help at all?

diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index 781170d36f50..52f8138243df 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -180,6 +180,7 @@ extern void __init
efi_uv1_memmap_phys_epilog(pgd_t *save_pgd);

 struct efi_setup_data {
        u64 fw_vendor;
+       u64 __unused;
        u64 tables;
        u64 smbios;
        u64 reserved[8];

Re: [GIT PULL 0/9] EFI fixes for v5.7-rc

[Theodore Y. Ts'o]

On Thu, Apr 09, 2020 at 11:29:06PM +0200, Ard Biesheuvel wrote:
> > What happens is that the kexec'ed kernel immediately crashes, at which
> > point we drop back into the BIOS, and then it boots the Debain 4.19.0
> > distro kernel instead of the kernel to be tested boot.  Since we lose
> > the boot command line that was used from the kexec, the gce-xfstests
> > image retries the kexec, which fails, and the failing kexec repeats
> > until I manually kill the VM.
>
> Does this help at all?
> 
> diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
> index 781170d36f50..52f8138243df 100644
> --- a/arch/x86/include/asm/efi.h
> +++ b/arch/x86/include/asm/efi.h
> @@ -180,6 +180,7 @@ extern void __init
> efi_uv1_memmap_phys_epilog(pgd_t *save_pgd);
> 
>  struct efi_setup_data {
>         u64 fw_vendor;
> +       u64 __unused;
>         u64 tables;
>         u64 smbios;
>         u64 reserved[8];


Tested-by: Theodore Ts'o <tytso@mit.edu>

Yep, that fixed it.  Thanks!!

I wonder if this structure definition should be moved something like
arch/x86/include/uapi/asm/efi.h so it's more obvious that the
structure layout is used externally to the kernel?

						- Ted

Re: [GIT PULL 0/9] EFI fixes for v5.7-rc

[Ard Biesheuvel]

On Fri, 10 Apr 2020 at 01:57, Theodore Y. Ts'o <tytso@mit.edu> wrote:
>
> On Thu, Apr 09, 2020 at 11:29:06PM +0200, Ard Biesheuvel wrote:
> > > What happens is that the kexec'ed kernel immediately crashes, at which
> > > point we drop back into the BIOS, and then it boots the Debain 4.19.0
> > > distro kernel instead of the kernel to be tested boot.  Since we lose
> > > the boot command line that was used from the kexec, the gce-xfstests
> > > image retries the kexec, which fails, and the failing kexec repeats
> > > until I manually kill the VM.
> >
> > Does this help at all?
> >
> > diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
> > index 781170d36f50..52f8138243df 100644
> > --- a/arch/x86/include/asm/efi.h
> > +++ b/arch/x86/include/asm/efi.h
> > @@ -180,6 +180,7 @@ extern void __init
> > efi_uv1_memmap_phys_epilog(pgd_t *save_pgd);
> >
> >  struct efi_setup_data {
> >         u64 fw_vendor;
> > +       u64 __unused;
> >         u64 tables;
> >         u64 smbios;
> >         u64 reserved[8];
>
>
> Tested-by: Theodore Ts'o <tytso@mit.edu>
>

OK, I'll spin a proper patch

> Yep, that fixed it.  Thanks!!
>
> I wonder if this structure definition should be moved something like
> arch/x86/include/uapi/asm/efi.h so it's more obvious that the
> structure layout is used externally to the kernel?
>

Well, 95% of the data structures used by EFI are based on the UEFI
spec, so the base assumption is really that we cannot make changes
like these to begin with. But I'll add a DON'T TOUCH comment here in
any case.

Re: [GIT PULL 0/9] EFI fixes for v5.7-rc

[Dave Young]

Cc kexec list.
On 04/10/20 at 09:08am, Ard Biesheuvel wrote:
> On Fri, 10 Apr 2020 at 01:57, Theodore Y. Ts'o <tytso@mit.edu> wrote:
> >
> > On Thu, Apr 09, 2020 at 11:29:06PM +0200, Ard Biesheuvel wrote:
> > > > What happens is that the kexec'ed kernel immediately crashes, at which
> > > > point we drop back into the BIOS, and then it boots the Debain 4.19.0
> > > > distro kernel instead of the kernel to be tested boot.  Since we lose
> > > > the boot command line that was used from the kexec, the gce-xfstests
> > > > image retries the kexec, which fails, and the failing kexec repeats
> > > > until I manually kill the VM.
> > >
> > > Does this help at all?
> > >
> > > diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
> > > index 781170d36f50..52f8138243df 100644
> > > --- a/arch/x86/include/asm/efi.h
> > > +++ b/arch/x86/include/asm/efi.h
> > > @@ -180,6 +180,7 @@ extern void __init
> > > efi_uv1_memmap_phys_epilog(pgd_t *save_pgd);
> > >
> > >  struct efi_setup_data {
> > >         u64 fw_vendor;
> > > +       u64 __unused;
> > >         u64 tables;
> > >         u64 smbios;
> > >         u64 reserved[8];
> >
> >
> > Tested-by: Theodore Ts'o <tytso@mit.edu>
> >
> 
> OK, I'll spin a proper patch
> 
> > Yep, that fixed it.  Thanks!!
> >
> > I wonder if this structure definition should be moved something like
> > arch/x86/include/uapi/asm/efi.h so it's more obvious that the
> > structure layout is used externally to the kernel?
> >
> 
> Well, 95% of the data structures used by EFI are based on the UEFI
> spec, so the base assumption is really that we cannot make changes
> like these to begin with. But I'll add a DON'T TOUCH comment here in
> any case.
> 

The runtime cleanup looks a very good one, but I also missed that,
userspace kexec-tools will break with the efi setup_data changes. But
kexec_file_load will just work with the cleanup applied.

Ard, could you add kexec list in cc when you send the fix out?

Thanks
Dave

Re: [GIT PULL 0/9] EFI fixes for v5.7-rc

[Theodore Y. Ts'o]

On Fri, Apr 10, 2020 at 09:54:42PM +0800, Dave Young wrote:
> 
> The runtime cleanup looks a very good one, but I also missed that,
> userspace kexec-tools will break with the efi setup_data changes. But
> kexec_file_load will just work with the cleanup applied.

Hmmm, I wonder if there could be some kselftest or kunit tests that
would make it easier to pick up these sorts of regressions earlier?

      	      	     	     	      	    - Ted

Re: [GIT PULL 0/9] EFI fixes for v5.7-rc

[Dave Young]

On 04/11/20 at 03:43pm, Theodore Y. Ts'o wrote:
> On Fri, Apr 10, 2020 at 09:54:42PM +0800, Dave Young wrote:
> > 
> > The runtime cleanup looks a very good one, but I also missed that,
> > userspace kexec-tools will break with the efi setup_data changes. But
> > kexec_file_load will just work with the cleanup applied.
> 
> Hmmm, I wonder if there could be some kselftest or kunit tests that
> would make it easier to pick up these sorts of regressions earlier?

I thought about that before, but did not go with any actual actions.
kexec test needs a system reboot, Kdump is even harder to test, that is
the reason I hesitated about.

But since the breakage happens here and there frequently, it is time to
try it.  I think I will play with it, but I might be slow because of
other things,  welcome to post patches if anyone is interested :)

Thanks
Dave

Re: [GIT PULL 0/9] EFI fixes for v5.7-rc

[David Howells]

I've been seeing the:

        exit_boot() failed!
        efi_main() failed!

thing.  This fixes it.

Tested-by: David Howells <dhowells@redhat.com>