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

[Jens Remus <jremus@linux.ibm.com>]

On 5/5/2026 2:53 PM, sashiko-bot@kernel.org wrote:
> 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.

Shall we keep the text_end and sframe_end is pointing past the
respective section end semantic and just adjust the value passed on the
call to mtree_insert_range()?

> 
> 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)

			if (eppnt->p_flags & PF_X && eppnt->p_type == PT_LOAD)

>> +				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?

Do you agree?

> 
>>  		}
>>  	}
>>  
> [ ... ]
>> @@ -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))

			if (elf_ppnt->p_flags & PF_X && elf_ppnt->p_type == PT_LOAD)

>> +				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?

Likewise.

Regards,
Jens
-- 
Jens Remus
Linux on Z Development (D3303)
jremus@de.ibm.com / jremus@linux.ibm.com

IBM Deutschland Research & Development GmbH; Vorsitzender des Aufsichtsrats: Wolfgang Wendt; Geschäftsführung: David Faller; Sitz der Gesellschaft: Ehningen; Registergericht: Amtsgericht Stuttgart, HRB 243294
IBM Data Privacy Statement: https://www.ibm.com/privacy/