Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues: - [High] Removing `task_is_running()` from `rethook_find_ret_addr()` exposes stack unwinders to use-after-free, wild pointer dereferences, and list corruption when unwinding running tasks, because `rethook_node` recycling and unwinding lack adequate RCU synchronization. -- commit 83e1fac9c64377191cc2b41ef9da781c46f1e95a Author: Tengda Wu <[email protected]> rethook: Remove the running task check in rethook_find_ret_addr() This commit removes the task_is_running() check from rethook_find_ret_addr(). The commit message states the check is unnecessary because unwinders hold RCU and rethook_node structures are RCU-freed. Callers are expected to provide their own safe context. > The iteration is already safe from crashes because > unwind_next_frame() holds RCU and rethook_node structures are RCU-freed; > even if the iteration goes off the rails and returns invalid information, > it will not crash. Callers that require consistency must provide a safe > context themselves. [ ... ] > diff --git a/kernel/trace/rethook.c b/kernel/trace/rethook.c > index 5a8bdf88999a9..1e7fdebe3cd5d 100644 > --- a/kernel/trace/rethook.c > +++ b/kernel/trace/rethook.c [ ... ] > @@ -250,9 +251,6 @@ unsigned long rethook_find_ret_addr(struct task_struct > *tsk, unsigned long frame > if (WARN_ON_ONCE(!cur)) > return 0; > > - if (tsk != current && task_is_running(tsk)) > - return 0; > - > do { > ret = __rethook_find_ret_addr(tsk, cur); [Severity: High] This is a pre-existing issue, but does removing this check entirely expose the stack unwinders to a use-after-free and list corruption? The commit message mentions that rethook_node structures are RCU-freed, but looking at rethook_recycle(), the nodes appear to be immediately pushed back to the lockless objpool without an RCU grace period: kernel/trace/rethook.c:rethook_recycle() { ... objpool_push(node, &node->rethook->pool); } Additionally, while unwind_next_frame() does hold RCU for a single frame iteration, stack unwinders like x86 ORC drop the read lock between frames: arch/x86/kernel/unwind_orc.c:unwind_next_frame() { ... guard(rcu)(); ... } Because the unwinder retains the list cursor (*cur) across frame iterations while RCU is dropped, could unwinding a running task allow the target task to recycle the node? If the node is recycled and instantly reallocated to another task, the next call to __rethook_find_ret_addr() would execute: kernel/trace/rethook.c:__rethook_find_ret_addr() { ... node = node->next; on a stale pointer. Could removing this check turn a theoretical race into an easily triggered use-after-free when reading /proc/<pid>/stack for tasks executing kretprobes? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
