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

On 2026-05-06 08:36, Steven Rostedt wrote:
> On Wed, 6 May 2026 16:56:01 +0200
> Jens Remus<[email protected]> 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.