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

[Steven Rostedt <rostedt@kernel.org>]

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