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

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

[Jens Remus]

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/

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

[Steven Rostedt]

On Wed, 6 May 2026 16:56:01 +0200
Jens Remus <jremus@linux.ibm.com> wrote:

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

Let's keep the end exclusive, and add the "- 1" to text_end before
passing it to the mtree_insert_range(). I did that to get it working for
me.

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

Indu or Jose?

-- Steve

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

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

[Indu Bhagat]

On 2026-05-06 08:36, Steven Rostedt wrote:
> On Wed, 6 May 2026 16:56:01 +0200
> Jens Remus<jremus@linux.ibm.com> wrote:
> 
>>>> --- 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()?
> Let's keep the end exclusive, and add the "- 1" to text_end before
> passing it to the mtree_insert_range(). I did that to get it working for
> me.
> 
>>> 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?
> Indu or Jose?

I think checking for PT_LOAD makes sense as we want to detect the 
executable text segments.