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
