linux-kernel - Re: [Linaro-mm-sig] [PATCH v2] dma-buf/sw_sync: Avoid recursive lock during fence signal

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <a05e0c2e-fd62-4a8e-9fa4-dffaf86f7730@gmail.com>
Date:   Tue, 22 Aug 2023 15:01:50 +0200
From:   Christian König <ckoenig.leichtzumerken@...il.com>
To:     Rob Clark <robdclark@...il.com>, dri-devel@...ts.freedesktop.org
Cc:     Christian König <christian.koenig@....com>,
        Rob Clark <robdclark@...omium.org>,
        Bas Nieuwenhuizen <bas@...nieuwenhuizen.nl>,
        Sumit Semwal <sumit.semwal@...aro.org>,
        Gustavo Padovan <gustavo@...ovan.org>,
        Chris Wilson <chris@...is-wilson.co.uk>,
        "open list:SYNC FILE FRAMEWORK" <linux-media@...r.kernel.org>,
        "moderated list:DMA BUFFER SHARING FRAMEWORK" 
        <linaro-mm-sig@...ts.linaro.org>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [Linaro-mm-sig] [PATCH v2] dma-buf/sw_sync: Avoid recursive lock
 during fence signal

Am 18.08.23 um 16:59 schrieb Rob Clark:
> From: Rob Clark <robdclark@...omium.org>
>
> If a signal callback releases the sw_sync fence, that will trigger a
> deadlock as the timeline_fence_release recurses onto the fence->lock
> (used both for signaling and the the timeline tree).
>
> To avoid that, temporarily hold an extra reference to the signalled
> fences until after we drop the lock.
>
> (This is an alternative implementation of https://patchwork.kernel.org/patch/11664717/
> which avoids some potential UAF issues with the original patch.)
>
> v2: Remove now obsolete comment, use list_move_tail() and
>      list_del_init()
>
> Reported-by: Bas Nieuwenhuizen <bas@...nieuwenhuizen.nl>
> Fixes: d3c6dd1fb30d ("dma-buf/sw_sync: Synchronize signal vs syncpt free")
> Signed-off-by: Rob Clark <robdclark@...omium.org>

Reviewed-by: Christian König <christian.koenig@....com>

> ---
>   drivers/dma-buf/sw_sync.c | 18 +++++++++---------
>   1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c
> index 63f0aeb66db6..f0a35277fd84 100644
> --- a/drivers/dma-buf/sw_sync.c
> +++ b/drivers/dma-buf/sw_sync.c
> @@ -191,6 +191,7 @@ static const struct dma_fence_ops timeline_fence_ops = {
>    */
>   static void sync_timeline_signal(struct sync_timeline *obj, unsigned int inc)
>   {
> +	LIST_HEAD(signalled);
>   	struct sync_pt *pt, *next;
>   
>   	trace_sync_timeline(obj);
> @@ -203,21 +204,20 @@ static void sync_timeline_signal(struct sync_timeline *obj, unsigned int inc)
>   		if (!timeline_fence_signaled(&pt->base))
>   			break;
>   
> -		list_del_init(&pt->link);
> +		dma_fence_get(&pt->base);
> +
> +		list_move_tail(&pt->link, &signalled);
>   		rb_erase(&pt->node, &obj->pt_tree);
>   
> -		/*
> -		 * A signal callback may release the last reference to this
> -		 * fence, causing it to be freed. That operation has to be
> -		 * last to avoid a use after free inside this loop, and must
> -		 * be after we remove the fence from the timeline in order to
> -		 * prevent deadlocking on timeline->lock inside
> -		 * timeline_fence_release().
> -		 */
>   		dma_fence_signal_locked(&pt->base);
>   	}
>   
>   	spin_unlock_irq(&obj->lock);
> +
> +	list_for_each_entry_safe(pt, next, &signalled, link) {
> +		list_del_init(&pt->link);
> +		dma_fence_put(&pt->base);
> +	}
>   }
>   
>   /**