Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [Critical] Race condition in `__eh_frame_remove_section()` leads to double-free, memory leak, and SRCU list corruption. - [Critical] Unwinder error path triggers hard deadlock and NMI-safety violations by calling `eh_frame_remove_section()`. - [Medium] Strict size matching in prctl handlers rejects UAPI structure extensions, breaking forward compatibility. - [Medium] Silent data truncation of 64-bit `eh_frame_hdr_start` in `eh_frame_unregister()` on 32-bit platforms. --
--- Patch [25]: [RFC PATCH v1 25/25] unwind_user/eh_frame: Add prctl() interface for (un)registering .eh_frame_hdr sections --- commit 04469d758625968e94108e567fd1bac500c0f41d Author: Jens Remus <[email protected]> unwind_user/eh_frame: Add prctl() interface for (un)registering .eh_frame_hdr sections This patch adds two new prctl options to allow dynamic linkers to register and unregister .eh_frame_hdr sections with the kernel. It introduces the eh_frame_setup structure to pass section bounds from userspace and adds the corresponding handlers. Link: https://lore.kernel.org/all/[email protected]/ > diff --git a/kernel/unwind/eh_frame.c b/kernel/unwind/eh_frame.c > index 49e8a3e8d794a..12279f02381d0 100644 > --- a/kernel/unwind/eh_frame.c > +++ b/kernel/unwind/eh_frame.c [ ... ] > @@ -1723,3 +1724,48 @@ void eh_frame_free_mm(struct mm_struct *mm) > > mtree_destroy(&mm->eh_frame_mt); > } > + > +int eh_frame_register(struct eh_frame_setup __user *user_data, > __kernel_size_t size) > +{ > + struct eh_frame_setup data; > + unsigned long eh_frame_hdr_end, text_end; > + > + if (!user_data && !size) > + return -EINVAL; > + > + if (size != sizeof(data)) > + return -EINVAL; [Severity: Medium] Could this strict size check prevent future UAPI extensions from being handled properly by older kernels? If a newer userspace binary passes a legitimately extended eh_frame_setup struct to an older kernel, it looks like it will be rejected with -EINVAL. Would it be better to use copy_struct_from_user() here to safely handle zero-padded extensions? > + > + if (copy_from_user(&data, user_data, sizeof(data))) > + return -EFAULT; [ ... ] > +int eh_frame_unregister(struct eh_frame_setup __user *user_data, > __kernel_size_t size) > +{ > + struct eh_frame_setup data; > + > + if (!user_data && !size) > + return -EINVAL; > + > + if (size != sizeof(data)) > + return -EINVAL; [Severity: Medium] As with eh_frame_register(), does this strict size comparison break forward compatibility for future struct extensions? > + > + if (copy_from_user(&data, user_data, sizeof(data))) > + return -EFAULT; > + > + /* Unregister only uses eh_frame_hdr_start */ > + if (data.eh_frame_hdr_size || data.text_start || data.text_size) > + return -EINVAL; > + > + return eh_frame_remove_section(data.eh_frame_hdr_start); [Severity: Medium] Does this code silently truncate the 64-bit parameter on 32-bit platforms, potentially causing the wrong section to be unregistered? The data.eh_frame_hdr_start parameter is a __u64, but it is passed directly into eh_frame_remove_section() which takes an unsigned long. If userspace provides an out-of-bounds 64-bit value on a 32-bit system, it appears it would be implicitly truncated to a valid 32-bit address rather than being rejected. Looking at the underlying implementation of eh_frame_remove_section() that is exposed by this new prctl, is there a race condition in __eh_frame_remove_section() that could lead to a double-free? kernel/unwind/eh_frame.c:__eh_frame_remove_section() { ... if (!mtree_erase(&mm->eh_frame_mt, sec->text_start)) { dbg_sec("mtree_erase failed: text=%lx\n", sec->text_start); return -EINVAL; } call_srcu(&eh_frame_srcu, &sec->rcu, eh_frame_free_srcu); ... } [Severity: Critical] If a thread unregisters the section and a third thread immediately registers a new section at the exact same address, would mtree_erase() erase and return the new section instead? Because the code evaluates the return value simply as non-NULL, it seems it would proceed to call call_srcu() on the old section again, potentially causing a double-free and SRCU list corruption. Also, regarding the interaction with the unwinder fast path in eh_frame_find(): kernel/unwind/eh_frame.c:eh_frame_find() { ... if (ret && (ret != -ENOENT && ret != -EOPNOTSUPP)) { dbg_sec("removing bad .eh_frame[_hdr] section\n"); if (eh_frame_remove_section(sec->eh_frame_hdr_start)) ... } [Severity: Critical] Can this sequence cause a hard deadlock or NMI-safety violation? Stack unwinding is frequently invoked from NMI or IRQ contexts. If eh_frame_find() encounters a parsing error, it unconditionally calls eh_frame_remove_section(). This attempts to acquire the mtree_lock spinlock and invokes call_srcu(), both of which are forbidden in NMI/IRQ contexts. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=25
