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

[H. Peter Anvin]

On July 17, 2024 12:33:08 AM PDT, "Shao, Marshall" <Marshall.Shao@xxxxxxxx> wrote:
>From: Marshall Shao <marshall.shao@xxxxxxxx>
>
>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.
>
>Signed-off-by: Marshall Shao <marshall.shao@xxxxxxxx>
>---
> 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;
>
>        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
>
> /*
>  * 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);
>-
>        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);
>        efi_stub_entry(handle, sys_table_arg, boot_params);
> }
>
>--
>2.34.1
>
>
>Internal Use - Confidential
>

This is strange. Is the problem that the BSS is not properly reflected in the UEFI PECOFF header?