Re: [RFC PATCH v1 10/25] unwind_user/eh_frame: Detect .eh_frame_hdr sections in executables

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing duplication of eh_frame_mt during fork() leaves child processes without stack unwind capability.
- [Medium] Executable segments with a trailing BSS section silently fail .eh_frame_hdr registration.
--

--- Patch [10]: [RFC PATCH v1 10/25] unwind_user/eh_frame: Detect .eh_frame_hdr sections in executables ---
commit e178ff4c7b227a1dcb3db08f8923210e443187ac
Author: Jens Remus <[email protected]>

unwind_user/eh_frame: Detect .eh_frame_hdr sections in executables

This commit modifies the ELF loader to automatically detect .eh_frame_hdr
sections during binary and interpreter loading. It associates these sections
with the mm_struct to support user-space stack unwinding.

> When loading an ELF executable, automatically detect .eh_frame_hdr
> sections and associate them with the mm_struct.

[Severity: High]
Are these eh_frame sections properly preserved for child processes during
fork()?

When a process forks, dup_mm() creates a new mm_struct and calls mm_init(),
which appears to initialize eh_frame_mt as an empty maple tree:

kernel/fork.c:mm_init_eh_frame() {
    ...
    mt_init(&mm->eh_frame_mt);
    ...
}

Is there any subsequent logic that duplicates the parent's eh_frame_mt
entries during dup_mm()? If not, won't child processes inherit correctly
duplicated VMAs but an empty eh_frame_mt, resulting in a complete loss of
stack unwinding capabilities for all children?

> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> index 16a56b6b3f6ca..8c1cdd8318c53 100644
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c

[ ... ]

> @@ -637,6 +638,23 @@ 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_eh_frame(const struct elf_phdr *text,
> +			     const struct elf_phdr *eh_frame,
> +			     unsigned long base_addr)
> +{
> +	unsigned long eh_frame_start, eh_frame_end,
> +		      text_start, text_end;
> +
> +	eh_frame_start = base_addr + eh_frame->p_vaddr;
> +	eh_frame_end   = eh_frame_start + eh_frame->p_memsz;
> +
> +	text_start   = base_addr + text->p_vaddr;
> +	text_end     = text_start + text->p_memsz;

[Severity: Medium]
Will this silently fail to register .eh_frame_hdr for segments with a
trailing BSS section?

When an ELF executable segment has p_memsz > p_filesz crossing a page
boundary, the loader maps the file-backed portion and creates a separate
anonymous VMA for the BSS.

Because text_end is calculated using p_memsz, it may extend beyond the
file-backed VMA boundary. When this is passed to eh_frame_add_section(),
it uses vma_lookup() and checks if the entire segment resides in a single
VMA:

kernel/unwind/eh_frame.c:eh_frame_add_section() {
    ...
    if (!text_vma ||
        !(text_vma->vm_flags & VM_EXEC) ||
        text_end > text_vma->vm_end) {
        return -EINVAL;
    ...
}

Since the file-backed VMA ends before text_end in this scenario, won't the
text_end > text_vma->vm_end check fail and terminate registration?

> +
> +	/* Ignore return value, eh_frame[_hdr] section isn't critical */
> +	eh_frame_add_section(eh_frame_start, eh_frame_end, text_start, text_end);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=10