Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [Critical] The patch introduces a Use-After-Free vulnerability by failing to set `target->debugger_process = NULL` during error unwind, leaving a dangling pointer. Pre-existing issues: - [High] Permanent reference leak of a `kfd_process` due to a race condition between `KFD_IOC_DBG_TRAP_ENABLE` and target process termination. -- commit 8dd55ae921073df4d72c79c03882687ae0f9c626 Author: Yongqiang Sun <[email protected]> drm/amdkfd: Unwind debug trap enable on copy_to_user failure This patch adds cleanup logic to the copy_to_user failure path in kfd_dbg_trap_enable() to undo partial setup, including releasing the debug event file and decrementing the debugged process count. > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_debug.c > b/drivers/gpu/drm/amd/amdkfd/kfd_debug.c > index ba99e0f258aee..5a9435dcd839b 100644 > --- a/drivers/gpu/drm/amd/amdkfd/kfd_debug.c > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_debug.c > @@ -812,6 +812,12 @@ int kfd_dbg_trap_enable(struct kfd_process *target, > uint32_t fd, > > if (copy_to_user(runtime_info, (void *)&target->runtime_info, > copy_size)) { > kfd_dbg_trap_deactivate(target, false, 0); > + fput(target->dbg_ev_file); > + target->dbg_ev_file = NULL; > + if (target->debugger_process) > + > atomic_dec(&target->debugger_process->debugged_process_count); [Severity: Critical] Does this error path leave target->debugger_process as a dangling pointer? In kfd_chardev.c:kfd_ioc_dbg_trap(), the target's debugger_process is set before calling kfd_dbg_trap_enable(): if (target != p) target->debugger_process = p; r = kfd_dbg_trap_enable(target, ...); When kfd_dbg_trap_enable() fails and unwinds, it decrements the debugged_process_count but does not set target->debugger_process to NULL. If the debugger process then exits, its cleanup routine will skip this target (because debugged_process_count is 0), and the debugger's kfd_process struct will be freed. If the target process later attempts to self-debug by calling KFD_IOC_DBG_TRAP_ENABLE on itself, kfd_ioc_dbg_trap() will bypass overwriting the dangling pointer because target == p: if (target != p) target->debugger_process = p; Then kfd_dbg_trap_enable() will execute: if (target->debugger_process) atomic_inc(&target->debugger_process->debugged_process_count); Could this sequence lead to a use-after-free and memory corruption? Should target->debugger_process be set to NULL on this error path? > + target->debug_trap_enabled = false; > + kfd_unref_process(target); > r = -EFAULT; > } > [Severity: High] This is a pre-existing issue, but does kfd_ioc_dbg_trap() have a race condition with target process termination that can leak a kfd_process reference? If KFD_IOC_DBG_TRAP_ENABLE starts and blocks on mutex_lock(&target->mutex), and the target process simultaneously terminates, the target's mmu_notifier release triggers and removes it from the global hash table: kfd_process_notifier_release() hash_del_rcu(&p->kfd_processes); Then it calls kfd_dbg_trap_disable() without holding target->mutex: kfd_process_notifier_release_internal() p->mm = NULL; kfd_dbg_trap_disable(p); Since ENABLE hasn't completed, debug_trap_enabled is false, and disable returns immediately. When ENABLE finally acquires the mutex, it succeeds, sets debug_trap_enabled = true, and takes an extra kref on the target: kfd_chardev.c:kfd_ioc_dbg_trap() r = kfd_dbg_trap_enable(target, ...); When the debugger process eventually exits, its cleanup iterates over kfd_processes_table to detach targets. Because the target was already removed via hash_del_rcu(), the debugger fails to find it. Since kfd_dbg_trap_disable() is never called for this target again, does this permanently leak the extra kref taken by ENABLE, along with all associated GPU resources? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
