Re: [PATCH 2/5] efi/libstub: Pass a struct efi_image_info to efi_stub_common()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [High] efi/libstub: freed memory retains RO/XP attributes in
  efi_zboot_entry() [1]
--

commit ddd62e73ed528e3f2c2ae65f0467b40b21e1c353
Author: Ard Biesheuvel <ardb@kernel.org>

efi/libstub: Pass a struct efi_image_info to efi_stub_common()

This commit passes a struct efi_image_info pointer from both efi_pe_entry()
and efi_zboot_entry() into efi_stub_common().

> diff --git a/drivers/firmware/efi/libstub/zboot.c b/drivers/firmware/efi/libstub/zboot.c
> index 4a5ffe164873e..97ae706c034ad 100644
> --- a/drivers/firmware/efi/libstub/zboot.c
> +++ b/drivers/firmware/efi/libstub/zboot.c
[ ... ]
> @@ -93,9 +98,15 @@ efi_zboot_entry(efi_handle_t handle, efi_system_table_t *systab)
>  	}
>  
>  	// Decompress the payload into the newly allocated buffer
> -	status = efi_zboot_decompress((void *)image_base, alloc_size) ?:
> -	         efi_stub_common(handle, image, image_base, cmdline_ptr);
> +	status = efi_zboot_decompress((void *)image_base, alloc_size);
> +	if (status != EFI_SUCCESS)
> +		goto error;
> +
> +	auto image_info = zboot_get_image_info(image_base);
> +	status = efi_stub_common(handle, image, image_base, cmdline_ptr,
> +				 image_info);
>  
> +error:
>  	efi_free(alloc_size, image_base);
>  	return status;
>  }

[Severity: High]
This problem wasn't introduced by this patch, but can efi_free() return
pages at image_base to the firmware pool while they still carry EFI_MEMORY_RO
and EFI_MEMORY_XP attributes [1]?

When efi_zboot_decompress() succeeds, it remaps the decompressed image's code
as read-only and its data as non-executable via efi_remap_image().  If
efi_stub_common() then fails, efi_free() frees image_base without clearing
those attributes, and UEFI does not require FreePages() to clear memory
attributes automatically.  Later firmware allocations that reuse those pages
(such as during boot fallback) can fault and crash when writing data or
executing code.

[1] https://sashiko.dev/#/bug/linux-e232037c-ef9b-47bd-b6d0-1bea4732c0e2

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106141936.2744481-7-ardb+git@google.com?part=2