* Re: [PATCH v14 06/19] unwind_user/sframe: Detect .sframe sections in executables [not found] ` <20260505125336.72A36C2BCB4@smtp.kernel.org> @ 2026-05-06 14:56 ` Jens Remus 2026-05-06 15:36 ` Steven Rostedt 0 siblings, 1 reply; 3+ messages in thread From: Jens Remus @ 2026-05-06 14:56 UTC (permalink / raw) To: Steven Rostedt, Josh Poimboeuf, Indu Bhagat; +Cc: bpf, sashiko 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/ ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v14 06/19] unwind_user/sframe: Detect .sframe sections in executables 2026-05-06 14:56 ` [PATCH v14 06/19] unwind_user/sframe: Detect .sframe sections in executables Jens Remus @ 2026-05-06 15:36 ` Steven Rostedt 2026-05-08 23:05 ` Indu Bhagat 0 siblings, 1 reply; 3+ messages in thread From: Steven Rostedt @ 2026-05-06 15:36 UTC (permalink / raw) To: Jens Remus; +Cc: Josh Poimboeuf, Indu Bhagat, bpf, sashiko, Jose E. Marchesi 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v14 06/19] unwind_user/sframe: Detect .sframe sections in executables 2026-05-06 15:36 ` Steven Rostedt @ 2026-05-08 23:05 ` Indu Bhagat 0 siblings, 0 replies; 3+ messages in thread From: Indu Bhagat @ 2026-05-08 23:05 UTC (permalink / raw) To: Steven Rostedt, Jens Remus; +Cc: Josh Poimboeuf, bpf, sashiko, Jose E. Marchesi 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. ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2026-05-08 23:05 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20260505121718.3572346-7-jremus@linux.ibm.com>
[not found] ` <20260505125336.72A36C2BCB4@smtp.kernel.org>
2026-05-06 14:56 ` [PATCH v14 06/19] unwind_user/sframe: Detect .sframe sections in executables Jens Remus
2026-05-06 15:36 ` Steven Rostedt
2026-05-08 23:05 ` Indu Bhagat
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox