Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [High] The commit improperly merges fuzzer reproducer code that intentionally stalls the kernel, introducing a severe 50ms delay inside a spinlock with local interrupts disabled. Pre-existing issues: - [Critical] Calling `dma_fence_get()` in `sync_timeline_signal()` on a fence whose refcount may have already dropped to 0 leads to a use-after-free, double-free, and rb-tree corruption. -- --- Patch [1]: [BUG] WARNING: refcount bug in sync_timeline_signal --- commit 6c2b9513def2f661abe36d3f3a4c129d8212ea60 Author: Jaeyoung Chung <[email protected]> WARNING: refcount bug in sync_timeline_signal This commit merges a fuzzer's reproducer delay patch directly into the sw_sync subsystem. It does not provide a fix for the underlying race condition, but instead introduces an intentional delay. > diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c > index 8df20b0218a9e..d30ce9d915ba7 100644 > --- a/drivers/dma-buf/sw_sync.c > +++ b/drivers/dma-buf/sw_sync.c [ ... ] > @@ -215,6 +216,10 @@ static void sync_timeline_signal(struct sync_timeline > *obj, unsigned int inc) > > spin_lock_irq(&obj->lock); > > + if (strncmp(current->comm, "syzrepro1", 9) == 0) { > + mdelay(50); > + } > + [Severity: High] Is it intentional to merge reproducer delay code into the tree? Adding a 50-millisecond busy loop (mdelay(50)) inside a critical section protected by spin_lock_irq(&obj->lock) in sync_timeline_signal() can cause severe latency spikes and missed hardware interrupts. An unprivileged user could trigger this by naming their task 'syzrepro1' and interacting with the sw_sync subsystem to create a local Denial of Service. > obj->value += inc; > > list_for_each_entry_safe(pt, next, &obj->pt_list, link) { [Severity: Critical] This is a pre-existing issue, but does sync_timeline_signal() unsafely call dma_fence_get() on a fence whose refcount may have already dropped to 0? When a userspace program closes a sync_file descriptor, it drops the underlying sync_pt fence refcount to 0, which triggers timeline_fence_release(). timeline_fence_release() blocks waiting for obj->lock. Concurrently, if sync_timeline_signal() holds obj->lock and iterates over obj->pt_list, it encounters the 0-refcount fence and unconditionally calls dma_fence_get(&pt->base): drivers/dma-buf/sw_sync.c:sync_timeline_signal() { ... dma_fence_get(&pt->base); list_move_tail(&pt->link, &signalled); rb_erase(&pt->node, &obj->pt_tree); ... } It then moves the fence to a local list, erases it from the tree, and unlocks. timeline_fence_release() then acquires the lock, illegally modifies the other thread's local list, performs a double rb_erase() on the tree, and frees the fence: drivers/dma-buf/sw_sync.c:timeline_fence_release() { ... if (!list_empty(&pt->link)) { list_del(&pt->link); rb_erase(&pt->node, &parent->pt_tree); } ... } Finally, sync_timeline_signal() iterates its local list, accessing and putting the freed fence, leading to a use-after-free and double free. Would using dma_fence_get_rcu() in sync_timeline_signal() safely increment the refcount only if it is non-zero? -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
