Re: [PATCH v2 04/10] efi/libstub: Capture loaded image information in struct efi_image_info

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] efi/libstub: use of _text instead of _start breaks RISC-V image
  offsets
--

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

efi/libstub: Capture loaded image information in struct efi_image_info

This commit introduces struct efi_image_info to record the loaded kernel
image's layout and entry point offsets, passing it to efi_stub_common() for
both the uncompressed EFI stub and EFI zboot.

> diff --git a/arch/riscv/kernel/head.S b/arch/riscv/kernel/head.S
> index 1631fd6bc786b..6c81f32be70b5 100644
> --- a/arch/riscv/kernel/head.S
> +++ b/arch/riscv/kernel/head.S
> @@ -28,6 +28,7 @@ SYM_CODE_START(_start)
>  	 * that expects this header format!!
>  	 */
>  #ifdef CONFIG_EFI
> +SYM_FUNC_ALIAS(efi_primary_entry, _start_kernel)
>  	/*
>  	 * This instruction decodes to "MZ" ASCII required by UEFI.
>  	 */
> diff --git a/drivers/firmware/efi/libstub/Makefile.zboot b/drivers/firmware/efi/libstub/Makefile.zboot
> index 52baacf3e9d94..e2769bbec963f 100644
> --- a/drivers/firmware/efi/libstub/Makefile.zboot
> +++ b/drivers/firmware/efi/libstub/Makefile.zboot
> @@ -26,9 +26,18 @@ zboot-size-len-$(CONFIG_KERNEL_ZSTD)	:= 4
>  $(obj)/vmlinuz: $(obj)/vmlinux.bin FORCE
>  	+$(call if_changed,$(zboot-method-y))
>  
> +# subtract the two symbol values to obtain the offset of struct efi_image_info
> +get_image_info_offset = $(shell $(NM) -n $(objtree)/vmlinux |\
> +			grep -E ' _text$$| __efistub_efistub_image_info$$' |\
> +			awk 'NR == 1 { a = toupper($$1) } \
> +			     NR == 2 { print "obase=16; ibase=16; " toupper($$1) "-" a }' |\
> +			bc)
> +
>  # avoid eager evaluation to prevent references to non-existent build artifacts
>  OBJCOPYFLAGS_vmlinuz.o = -I binary -O $(EFI_ZBOOT_BFD_TARGET) $(EFI_ZBOOT_OBJCOPY_FLAGS) \
> -			  --rename-section .data=.gzdata,load,alloc,readonly,contents
> +			 --rename-section .data=.gzdata,load,alloc,readonly,contents \
> +			 --add-symbol efistub_image_info_offset=0x$(get_image_info_offset)
> +

[ ... ]

> diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
> index f5eec8c12894c..394796472cc07 100644
> --- a/drivers/firmware/efi/libstub/efistub.h
> +++ b/drivers/firmware/efi/libstub/efistub.h
> @@ -1148,11 +1148,53 @@ efi_status_t handle_kernel_image(unsigned long *image_addr,
>  				 efi_loaded_image_t *image,
>  				 efi_handle_t image_handle);
>  
> +struct efi_image_info {
> +	s32	_text_offset;
> +	s32	__inittext_end_offset;
> +	s32	_edata_offset;
> +	s32	_end_offset;
> +	s32	entry_offset;
> +};
> +
> +const struct efi_image_info *efistub_get_image_info(void);
> +const struct efi_image_info *zboot_get_image_info(unsigned long image_base);
> +
> +static inline
> +unsigned long efistub_kernel_base(const struct efi_image_info *image_info)
> +{
> +	return (unsigned long)offset_to_ptr(&image_info->_text_offset);
> +}
> +
> +static inline
> +unsigned long efistub_kernel_size(const struct efi_image_info *image_info)
> +{
> +	return image_info->_edata_offset - image_info->_text_offset;
> +}
> +
> +static inline
> +unsigned long efistub_kernel_memsize(const struct efi_image_info *image_info)
> +{
> +	return image_info->_end_offset - image_info->_text_offset;
> +}
> +
> +static inline
> +unsigned long efistub_kernel_codesize(const struct efi_image_info *image_info)
> +{
> +	return image_info->__inittext_end_offset - image_info->_text_offset;
> +}
> +
> +static inline
> +unsigned long efistub_kernel_entry_offset(const struct efi_image_info *image_info)
> +{
> +	return image_info->entry_offset - image_info->_text_offset;
> +}

[ ... ]

> diff --git a/drivers/firmware/efi/libstub/image_info.S b/drivers/firmware/efi/libstub/image_info.S
> new file mode 100644
> index 0000000000000..3d3dded0f451a
> --- /dev/null
> +++ b/drivers/firmware/efi/libstub/image_info.S
> @@ -0,0 +1,12 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#define REL_REF(sym)	.long	sym - 0b
> +
> +	.globl		__efistub_efistub_image_info
> +	.p2align	3
> +__efistub_efistub_image_info:
> +0:	REL_REF(_text)
> +	REL_REF(__inittext_end)
> +	REL_REF(_edata)
> +	REL_REF(_end)
> +	REL_REF(efi_primary_entry)

[Severity: High]
Does using _text as the image base reference break these offset calculations
on RISC-V?

Unlike arm64 and LoongArch, RISC-V places _start and HEAD_TEXT_SECTION
before _text:

arch/riscv/kernel/vmlinux.lds.S:
	. = LOAD_OFFSET;
	_start = .;
	HEAD_TEXT_SECTION
	. = ALIGN(PAGE_SIZE);

	.text : {
		_text = .;
		_stext = .;

Because arch/riscv/kernel/head.S is in __HEAD (.head.text), the binary
Image starts at _start and efi_primary_entry (_start_kernel) sits below
_text.

As a result, get_image_info_offset in Makefile.zboot computes
efistub_image_info_offset relative to _text instead of the start of the
decompressed image (_start), so zboot_get_image_info() points (_text -
_start) bytes before struct efi_image_info.  In addition,
efistub_kernel_base() returns _text rather than _start, the size helpers
exclude .head.text, and efistub_kernel_entry_offset() computes a negative
s32 (_start_kernel - _text) that sign-extends to a bogus unsigned long.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110121731.2728824-12-ardb+git@google.com?part=4