Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
New issues:
- [High] The strict `size != sizeof(data)` check completely breaks UAPI backward and forward compatibility for extensible structs.
- [Medium] Silent truncation of the 64-bit `eh_frame_hdr_start` address to a 32-bit `unsigned long` during unregistration on 32-bit architectures.
Pre-existing issues:
- [High] If `mas_erase` fails due to an out-of-memory condition, it returns the target entry but leaves it in the maple tree. `__eh_frame_remove_section` fails to check `mas_is_err(mas)`, freeing the entry while it remains accessible in the tree.
- [High] The unregister prctl triggers an unbounded iteration over the maple tree while holding a spinlock, allowing an unprivileged user to cause a CPU soft lockup.
--
--- Patch [24]: [RFC PATCH v2 24/24] unwind_user/eh_frame: Add prctl() interface for (un)registering .eh_frame_hdr sections ---
commit 51adfd3b1e2795e1bc35c760119a8842b4e76983
Author: Jens Remus [off-list ref]
unwind_user/eh_frame: Add prctl() interface for (un)registering .eh_frame_hdr sections
This commit adds prctl interfaces to register and unregister .eh_frame_hdr
sections. It allows dynamic linkers to notify the kernel about the location
of these sections using a new eh_frame_setup structure.
Link: https://lore.kernel.org/all/20260528151023.00f5ec4e@gandalf.local.home/ (local)
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
@@ -1609,3 +1610,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: High]
Does this strict size check prevent future UAPI extensions? Since this
is a new UAPI structure, rejecting sizes that do not exactly match the
kernel's structure size prevents extending the structure later without
breaking backward compatibility. Would it be better to use
copy_struct_from_user() here to handle size mismatches by verifying
trailing zeroes?
quoted hunk ↗ jump to hunk
+
+ if (copy_from_user(&data, user_data, sizeof(data)))
+ return -EFAULT;
+
+ if (check_add_overflow(data.eh_frame_hdr_start, data.eh_frame_hdr_size,
+ &eh_frame_hdr_end))
+ return -EINVAL;
+
+ if (check_add_overflow(data.text_start, data.text_size, &text_end))
+ return -EINVAL;
+
+ return eh_frame_add_section(data.eh_frame_hdr_start, eh_frame_hdr_end,
+ data.text_start, text_end);
+}
+
+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: High]
Is this strict size check similarly going to cause extensibility issues
for the unregister path? Like the register function, could this use
copy_struct_from_user() to safely allow future modifications?
quoted hunk ↗ jump to hunk
+
+ 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]
Will data.eh_frame_hdr_start be silently truncated on 32-bit architectures?
The user provides a 64-bit __u64 address, but eh_frame_remove_section()
expects an unsigned long. If the upper 32 bits are set by userspace on a
32-bit architecture, this could truncate into a valid 32-bit address without
any bounds validation.
[Severity: High]
This isn't a bug introduced by this patch, but does exposing this function
to userspace via prctl allow an unprivileged user to cause a CPU soft lockup?
Looking at eh_frame_remove_section(), it performs an unbounded iteration over
the maple tree while holding a spinlock:
kernel/unwind/eh_frame.c:eh_frame_remove_section() {
...
mtree_lock(&mm->eh_frame_mt);
mas_for_each(&mas, sec, ULONG_MAX) {
if (sec->eh_frame_hdr_start == eh_frame_hdr_start) {
found = true;
ret |= __eh_frame_remove_section(&mas, sec);
}
}
mtree_unlock(&mm->eh_frame_mt);
...
}
Could an attacker register a massive number of non-overlapping sections and
then trigger a long unbounded loop with preemption disabled?
[Severity: High]
This is also a pre-existing issue, but the new prctl makes it trivially
reachable by unprivileged userspace, which could lead to a use-after-free.
When __eh_frame_remove_section() attempts to remove a section:
kernel/unwind/eh_frame.c:__eh_frame_remove_section() {
...
if (mas_erase(mas) != sec) {
dbg_sec("mas_erase failed: text=%lx\n", sec->text_start);
return -EINVAL;
}
call_srcu(&eh_frame_srcu, &sec->rcu, eh_frame_free_srcu);
...
}
If mas_erase() fails due to an out-of-memory condition, it can set an error
state and return the target entry without removing it from the tree. Because
the code does not check mas_is_err(mas), it frees the entry while it remains
accessible in the maple tree. Should this verify mas_is_err() before freeing
the section?
+}
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260821195259.2688377-1-jremus@linux.ibm.com?part=24