On 11/5/26 06:27, Edward Adam Davis wrote:
> There is a race condition between closing a sync file and the SW_SYNC_IOC_INC
> operation on a sync obj. When a fence is created via SW_SYNC_IOC_CREATE_FENCE,
> its reference count is initialized to 1. When the sync file is closed, the
> fence's reference count drops to 0. A race occurs if SW_SYNC_IOC_INC accesses
> the fence whose ref count has already reached 0 before dma_fence_release()
> calls timeline_fence_release() to erase the corresponding pt_tree node from
> the sync object, potentially hitting:
>
> refcount_t: addition on 0; use-after-free.
> ------------[ cut here ]------------
> kernel BUG at lib/refcount.c:34!
> Oops: invalid opcode: 0000 [#1] SMP KASAN NOPTI
> Call Trace:
> kref_get include/linux/kref.h:45 [inline]
> dma_fence_get include/linux/dma-fence.h:323 [inline]
> sync_timeline_signal+0x6d9/0x8a0 drivers/dma-buf/sw_sync.c:224
> sw_sync_ioctl_inc drivers/dma-buf/sw_sync.c:405 [inline]
> sw_sync_ioctl+0x7b4/0x13f0 drivers/dma-buf/sw_sync.c:470
>
> Use dma_fence_get_rcu() to replace dma_get_fence(), if fence reference
> count is 0 then do like timeline fence release things: remove pt link
> and erase pt node.
>
> Fixes: e531fdb5cd5e ("dma-buf/sw_sync: Avoid recursive lock during fence
> signal")
Please completely drop that. This is not something which has any justification
to be backported.
> Reported-by: [email protected]
> Closes: https://syzkaller.appspot.com/bug?extid=cba1a1f459230395a076
> Signed-off-by: Edward Adam Davis <[email protected]>
> ---
> v1 -> v2: use dma_fence_get_rcu() and remove link & erase pt node
>
> drivers/dma-buf/sw_sync.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c
> index 8df20b0218a9..261bc98b1ed6 100644
> --- a/drivers/dma-buf/sw_sync.c
> +++ b/drivers/dma-buf/sw_sync.c
> @@ -221,7 +221,11 @@ static void sync_timeline_signal(struct sync_timeline
> *obj, unsigned int inc)
> if (!timeline_fence_signaled(&pt->base))
> break;
>
> - dma_fence_get(&pt->base);
> + if (!dma_fence_get_rcu(&pt->base)) {
Well that is at least better than before but still needs a code comment
explaining why this is possible without holding the RCU read side lock.
Something like /* It's possible to race with timeline_fence_release() here */.
> + list_del_init(&pt->link);
> + rb_erase(&pt->node, &obj->pt_tree);
What happens is that you race with timeline_fence_release(), so that here is
superfluous and potentially even harmful.
Regards,
Christian.
> + continue;
> + }
>
> list_move_tail(&pt->link, &signalled);
> rb_erase(&pt->node, &obj->pt_tree);
> --
> 2.43.0
>
