Re: [RFC PATCH v1 08/25] unwind_user/eh_frame: Store .eh_frame_hdr section data in per-mm maple tree

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] The `eh_frame_mt` maple tree is initialized without `MT_FLAGS_USE_RCU`, causing internal nodes to be synchronously freed, which crashes concurrent lockless readers.
- [High] fork() fails to inherit .eh_frame_hdr registrations, breaking unwinding for child processes.
- [High] A TOCTOU race condition in `eh_frame_remove_section` allows a double-free of one section and a memory leak of another.
- [High] An integer overflow in `table_end` calculation allows maliciously crafted `.eh_frame_hdr` sections to bypass bounds checking.
- [Low] Appending INIT_MM_EH_FRAME in init_mm causes a missing comma syntax error on multiple architectures.
--

--- Patch [8]: [RFC PATCH v1 08/25] unwind_user/eh_frame: Store .eh_frame_hdr section data in per-mm maple tree ---
commit bd68e8b201de54350859df41e271e507dd497773
Author: Jens Remus <[email protected]>
Subject: unwind_user/eh_frame: Store .eh_frame_hdr section data in per-mm maple tree

This commit associates .eh_frame_hdr sections with their mm by adding them to
a per-mm maple tree which is indexed by the corresponding text address
range. A single .eh_frame_hdr section can be associated with multiple
text ranges.

> diff --git a/include/linux/eh_frame.h b/include/linux/eh_frame.h
> --- a/include/linux/eh_frame.h
> +++ b/include/linux/eh_frame.h
[ ... ]
> @@ -19,14 +24,27 @@ struct eh_frame_section {
>  	u8		binary_search_table_enc;
>  };
>  
> +#define INIT_MM_EH_FRAME .eh_frame_mt = MTREE_INIT(eh_frame_mt, 0),

[Severity: Critical]
Is the MT_FLAGS_USE_RCU flag required here?

Since readers are locklessly iterating the tree using mt_for_each() (which
relies on rcu_read_lock()), omitting MT_FLAGS_USE_RCU causes concurrent
mtree_erase() or mtree_insert_range() to synchronously free internal nodes via
kfree() instead of kfree_rcu(). Can this cause a use-after-free for the
concurrent readers?

> +extern void eh_frame_free_mm(struct mm_struct *mm);
> +
>  extern int eh_frame_add_section(unsigned long eh_frame_hdr_start,
[ ... ]
> diff --git a/kernel/fork.c b/kernel/fork.c
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
[ ... ]
> @@ -1082,6 +1084,13 @@ static void mmap_init_lock(struct mm_struct *mm)
>  #endif
>  }
>  
> +static void mm_init_eh_frame(struct mm_struct *mm)
> +{
> +#ifdef CONFIG_HAVE_UNWIND_USER_EH_FRAME
> +	mt_init(&mm->eh_frame_mt);

[Severity: Critical]
Should this use mt_init_flags() with MT_FLAGS_USE_RCU?

Similar to the INIT_MM_EH_FRAME macro, initializing without RCU could lead to
a use-after-free if internal maple tree nodes are freed while lockless readers
are iterating.

[Severity: High]
Does fork() fail to inherit .eh_frame_hdr registrations?

A process that registers unwinding information and then calls fork() will have
its VMAs copied in dup_mmap(), but the child receives this empty maple tree
without any copied eh_frame_mt entries. Will this break stack unwinding for
the child process?

> +#endif
> +}
> +
>  static struct mm_struct *mm_init(struct mm_struct *mm, struct task_struct *p)
[ ... ]
> diff --git a/kernel/unwind/eh_frame.c b/kernel/unwind/eh_frame.c
> --- a/kernel/unwind/eh_frame.c
> +++ b/kernel/unwind/eh_frame.c
[ ... ]
> +static int __eh_frame_remove_section(struct mm_struct *mm,
> +				     struct eh_frame_section *sec)
> +{
> +	if (!mtree_erase(&mm->eh_frame_mt, sec->text_start)) {
> +		dbg("mtree_erase failed: text=%lx\n", sec->text_start);
> +		return -EINVAL;
> +	}
> +
> +	call_srcu(&eh_frame_srcu, &sec->rcu, eh_frame_free_srcu);
> +
> +	return 0;
> +}
> +
>  int eh_frame_remove_section(unsigned long eh_frame_hdr_start)
>  {
> -	return -ENOSYS;
> +	struct mm_struct *mm = current->mm;
> +	struct eh_frame_section *sec;
> +	unsigned long index = 0;
> +	bool found = false;
> +	int ret = 0;
> +
> +	guard(srcu)(&eh_frame_srcu);
> +
> +	mt_for_each(&mm->eh_frame_mt, sec, index, ULONG_MAX) {
> +		if (sec->eh_frame_hdr_start == eh_frame_hdr_start) {
> +			found = true;
> +			ret |= __eh_frame_remove_section(mm, sec);
> +		}
> +	}

[Severity: High]
Can this mtree_erase() and call_srcu() sequence race with another thread and
cause a double-free?

If Thread A locklessly iterates and finds sec1 in mt_for_each(), and Thread B
concurrently removes sec1, Thread C could map a new section sec2 at the same
text_start address.

When Thread A calls __eh_frame_remove_section(mm, sec1), mtree_erase() would
successfully remove and return sec2. However, Thread A then passes the
original sec1 pointer to call_srcu(), double-freeing sec1 and leaking sec2.

[ ... ]
> diff --git a/mm/init-mm.c b/mm/init-mm.c
> --- a/mm/init-mm.c
> +++ b/mm/init-mm.c
> @@ -48,6 +49,7 @@ struct mm_struct init_mm = {
>  #endif
>  	.flexible_array	= MM_STRUCT_FLEXIBLE_ARRAY_INIT,
>  	INIT_MM_CONTEXT(init_mm)
> +	INIT_MM_EH_FRAME
>  };

[Severity: Low]
Does appending INIT_MM_EH_FRAME here cause a missing comma syntax error on
some architectures?

While x86's INIT_MM_CONTEXT was updated to include a trailing comma in this
patch, other architectures (like UML or PowerPC Book3s 32-bit) may lack this
trailing comma in their INIT_MM_CONTEXT definitions, breaking the build.


In a related function, eh_frame_read_header() contains a potential overflow:

kernel/unwind/eh_frame.c:eh_frame_read_header() {
    ...
		table_end = table_start + fde_count * entry_size;
		if (table_end > sec->eh_frame_hdr_end)
			return -EINVAL;
    ...
}

[Severity: High]
Can this arithmetic multiplication overflow and bypass the bounds check?

If a malicious .eh_frame_hdr provides an artificially large fde_count,
fde_count * entry_size could overflow the unsigned long capacity, wrapping
around to a small value. This would bypass the bounds check and allow
out-of-bounds reads during later unwinding binary searches.

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