Re: [PATCH v14 06/19] unwind_user/sframe: Detect .sframe sections in executables

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing `PT_LOAD` segment type check when registering executable sections for sframe unwinding.
- [High] Off-by-one error in maple tree insertion bounds causes adjacent executable segments to fail registration.
--

commit 610ebc2ab2a7fd5ae4cd4a346076c9d5c2acc195
Author: Josh Poimboeuf <jpoimboe@kernel.org>

unwind_user/sframe: Detect .sframe sections in executables

When loading an ELF executable, automatically detect an .sframe section
and associate it with the mm_struct.

> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> index 16a56b6b3f6ca..55047659a3cf4 100644
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
[ ... ]
> @@ -637,6 +638,21 @@ static inline int make_prot(u32 p_flags, struct arch_elf_state *arch_state,
>  	return arch_elf_adjust_prot(prot, arch_state, has_interp, is_interp);
>  }
>  
> +static void elf_add_sframe(struct elf_phdr *text, struct elf_phdr *sframe,
> +			   unsigned long base_addr)
> +{
> +	unsigned long sframe_start, sframe_end, text_start, text_end;
> +
> +	sframe_start = base_addr + sframe->p_vaddr;
> +	sframe_end   = sframe_start + sframe->p_memsz;
> +
> +	text_start   = base_addr + text->p_vaddr;
> +	text_end     = text_start + text->p_memsz;

Should text_end be calculated as text_start + text->p_memsz - 1?

Since the maple tree API strictly expects inclusive ranges [first, last] for 
insertions, passing the exclusive text_end would cause the inserted range 
to cover one byte beyond the actual segment.

If an ELF binary has adjacent executable PT_LOAD segments, could the first 
segment's inclusive end overlap with the next segment's start and fail 
subsequent insertions?

> +
> +	/* Ignore return value, sframe section isn't critical */
> +	sframe_add_section(sframe_start, sframe_end, text_start, text_end);
> +}
> +
[ ... ]
> @@ -712,6 +729,19 @@ static unsigned long load_elf_interp(struct elfhdr *interp_elf_ex,
>  				error = -ENOMEM;
>  				goto out;
>  			}
> +			break;
> +		}
> +		case PT_GNU_SFRAME:
> +			sframe_phdr = eppnt;
> +			break;
> +		}
> +	}
> +
> +	if (sframe_phdr) {
> +		eppnt = interp_elf_phdata;
> +		for (i = 0; i < interp_elf_ex->e_phnum; i++, eppnt++) {
> +			if (eppnt->p_flags & PF_X)
> +				elf_add_sframe(eppnt, sframe_phdr, load_addr);

Does this need to verify the segment is loadable (eppnt->p_type == PT_LOAD)?

If non-loadable segments like PT_GNU_STACK have the PF_X flag set, they
typically have p_vaddr and p_memsz as 0. Processing these segments would
compute bogus text boundaries and bypass VMA validation. If a legitimate
PT_LOAD segment overlaps with these bogus ranges, could its insertion fail
silently and break stack unwinding for the executable?

>  		}
>  	}
>  
[ ... ]
> @@ -1242,6 +1276,14 @@ static int load_elf_binary(struct linux_binprm *bprm)
>  			elf_brk = k;
>  	}
>  
> +	if (sframe_phdr) {
> +		for (i = 0, elf_ppnt = elf_phdata;
> +		     i < elf_ex->e_phnum; i++, elf_ppnt++) {
> +			if ((elf_ppnt->p_flags & PF_X))
> +				elf_add_sframe(elf_ppnt, sframe_phdr, load_bias);

Similarly, should this also check if the segment is a PT_LOAD segment before 
adding it to the sframe sections?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260505121718.3572346-1-jremus@linux.ibm.com?part=6