linux-kernel - Re: [Patch] Do not clear BSS region in x86 stub

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAMj1kXHBSNxrzbQoaDea7HFcjN9HHk5==tXg1WLHDzW61aj4cg@mail.gmail.com>
Date: Wed, 17 Jul 2024 08:26:49 -0700
From: Ard Biesheuvel <ardb@...nel.org>
To: "Shao, Marshall" <Marshall.Shao@...l.com>
Cc: "linux-efi@...r.kernel.org" <linux-efi@...r.kernel.org>, 
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, "hpa@...or.com" <hpa@...or.com>, 
	"dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>, "bp@...en8.de" <bp@...en8.de>, 
	"mingo@...hat.com" <mingo@...hat.com>, "tglx@...utronix.de" <tglx@...utronix.de>
Subject: Re: [Patch] Do not clear BSS region in x86 stub

On Wed, 17 Jul 2024 at 08:17, Shao, Marshall <Marshall.Shao@...l.com> wrote:
>
> Clearing the BSS region may cause the UEFI firmware to malfunction
> during boot.
>
> When booting the kernel from an older firmware version that has TPM
> enabled and the MemoryOverwriteRequestControl bit set to 1, the
> firmware's boot service might encounter an exception if it attempts
> to initialize the BSS region within the x86 stub.
>
> To circumvent the firmware exception, it is advisable to enlarge the
> BOOT_STACK_SIZE and to perform the initialization of static variables
> prior to the decompression of the bzImage.
>

What does 'advisable' mean? This patch looks like a few hacks thrown
together that happen to work around some issue in combination.

Please explain what the exact problem is, and how this patch fixes it.

If you don't know what the exact problem is, please say so.


> Signed-off-by: Marshall Shao <marshall.shao@...l.com>
> ---
>  arch/x86/boot/compressed/misc.c         | 8 +++-----
>  arch/x86/include/asm/boot.h             | 2 +-
>  drivers/firmware/efi/libstub/x86-stub.c | 5 -----
>  3 files changed, 4 insertions(+), 11 deletions(-)
>
> diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
> index b70e4a21c15f..bac5a3c55c2c 100644
> --- a/arch/x86/boot/compressed/misc.c
> +++ b/arch/x86/boot/compressed/misc.c
> @@ -356,11 +356,9 @@ unsigned long decompress_kernel(unsigned char *outbuf, unsigned long virt_addr,
>                                 void (*error)(char *x))
>  {
>         unsigned long entry;
> -
> -       if (!free_mem_ptr) {
> -               free_mem_ptr     = (unsigned long)boot_heap;
> -               free_mem_end_ptr = (unsigned long)boot_heap + sizeof(boot_heap);
> -       }
> +       free_mem_ptr     = (unsigned long)boot_heap;
> +       free_mem_end_ptr = (unsigned long)boot_heap + sizeof(boot_heap);
> +       malloc_ptr = free_mem_ptr;
>

This is not safe. This reinitializes the heap after allocations have
been made already.

It would be better to annotate free_mem_ptr as __section(".data") to
ensure that it is zeroed even if BSS is not cleared.

>         if (__decompress(input_data, input_len, NULL, NULL, outbuf, output_len,
>                          NULL, error) < 0)
> diff --git a/arch/x86/include/asm/boot.h b/arch/x86/include/asm/boot.h
> index 3e5b111e619d..312bc87ab027 100644
> --- a/arch/x86/include/asm/boot.h
> +++ b/arch/x86/include/asm/boot.h
> @@ -33,7 +33,7 @@
>  #endif
>
>  #ifdef CONFIG_X86_64
> -# define BOOT_STACK_SIZE       0x4000
> +# define BOOT_STACK_SIZE       0x10000
>

Why is this necessary? EFI boot does not use this stack, only legacy
boot via the decompressor.


>  /*
>   * Used by decompressor's startup_32() to allocate page tables for identity
> diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
> index 1983fd3bf392..d92d2ccc709b 100644
> --- a/drivers/firmware/efi/libstub/x86-stub.c
> +++ b/drivers/firmware/efi/libstub/x86-stub.c
> @@ -21,7 +21,6 @@
>  #include "efistub.h"
>  #include "x86-stub.h"
>
> -extern char _bss[], _ebss[];
>
>  const efi_system_table_t *efi_system_table;
>  const efi_dxe_services_table_t *efi_dxe_table;
> @@ -476,9 +475,6 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle,
>         efi_status_t status;
>         char *cmdline_ptr;
>
> -       if (efi_is_native())
> -               memset(_bss, 0, _ebss - _bss);
> -

This bit has already been removed in mainline.

>         efi_system_table = sys_table_arg;
>
>         /* Check if we were booted by the EFI firmware */
> @@ -1000,7 +996,6 @@ void __noreturn efi_stub_entry(efi_handle_t handle,
>  void efi_handover_entry(efi_handle_t handle, efi_system_table_t *sys_table_arg,
>                         struct boot_params *boot_params)
>  {
> -       memset(_bss, 0, _ebss - _bss);

Removing this is not safe. free_mem_ptr is not the only global
variable in BSS that needs to be zeroed, there are others too.

>         efi_stub_entry(handle, sys_table_arg, boot_params);
>  }
>
> --
> 2.34.1
>