[Intel-gfx] [PATCH] dma-buf: Give dma-fence-array distinct lockclasses

[Koenig, Christian]

Am 24.08.19 um 15:58 schrieb Chris Wilson:
> In order to allow dma-fence-array as a generic container for fences, we
> need to allow for it to contain other dma-fence-arrays. By giving each
> dma-fence-array construction their own lockclass, we allow different
> types of dma-fence-array to nest, but still do not allow on class of
> dma-fence-array to contain itself (even though they have distinct
> locks).
>
> In practice, this means that each subsystem gets its own dma-fence-array
> class and we can freely use dma-fence-arrays as containers within the
> dmabuf core without angering lockdep.

I've considered this for as well. E.g. to use the dma_fence_array 
implementation instead of coming up with the dma_fence_chain container.

But as it turned out when userspace can control nesting, it is trivial 
to chain enough dma_fence_arrays together to cause an in kernel stack 
overflow. Which in turn creates a really nice attack vector.

So as long as userspace has control over dma_fence_array nesting this is 
a clear NAK and actually extremely dangerous.

It actually took me quite a while to get the dma_fence_chain container 
recursion less to avoid stuff like this.

Regards,
Christian.

>
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Christian König <christian.koenig at amd.com>
> Cc: Daniel Vetter <daniel at ffwll.ch>
> ---
>   drivers/dma-buf/dma-fence-array.c | 13 ++++++++-----
>   include/linux/dma-fence-array.h   | 16 ++++++++++++----
>   2 files changed, 20 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/dma-buf/dma-fence-array.c b/drivers/dma-buf/dma-fence-array.c
> index d3fbd950be94..d9bcdbb66d46 100644
> --- a/drivers/dma-buf/dma-fence-array.c
> +++ b/drivers/dma-buf/dma-fence-array.c
> @@ -147,10 +147,11 @@ EXPORT_SYMBOL(dma_fence_array_ops);
>    * If @signal_on_any is true the fence array signals if any fence in the array
>    * signals, otherwise it signals when all fences in the array signal.
>    */
> -struct dma_fence_array *dma_fence_array_create(int num_fences,
> -					       struct dma_fence **fences,
> -					       u64 context, unsigned seqno,
> -					       bool signal_on_any)
> +struct dma_fence_array *__dma_fence_array_create(int num_fences,
> +						 struct dma_fence **fences,
> +						 u64 context, unsigned seqno,
> +						 bool signal_on_any,
> +						 struct lock_class_key *key)
>   {
>   	struct dma_fence_array *array;
>   	size_t size = sizeof(*array);
> @@ -162,6 +163,8 @@ struct dma_fence_array *dma_fence_array_create(int num_fences,
>   		return NULL;
>   
>   	spin_lock_init(&array->lock);
> +	lockdep_set_class(&array->lock, key);
> +
>   	dma_fence_init(&array->base, &dma_fence_array_ops, &array->lock,
>   		       context, seqno);
>   	init_irq_work(&array->work, irq_dma_fence_array_work);
> @@ -174,7 +177,7 @@ struct dma_fence_array *dma_fence_array_create(int num_fences,
>   
>   	return array;
>   }
> -EXPORT_SYMBOL(dma_fence_array_create);
> +EXPORT_SYMBOL(__dma_fence_array_create);
>   
>   /**
>    * dma_fence_match_context - Check if all fences are from the given context
> diff --git a/include/linux/dma-fence-array.h b/include/linux/dma-fence-array.h
> index 303dd712220f..1395f9428cdb 100644
> --- a/include/linux/dma-fence-array.h
> +++ b/include/linux/dma-fence-array.h
> @@ -74,10 +74,18 @@ to_dma_fence_array(struct dma_fence *fence)
>   	return container_of(fence, struct dma_fence_array, base);
>   }
>   
> -struct dma_fence_array *dma_fence_array_create(int num_fences,
> -					       struct dma_fence **fences,
> -					       u64 context, unsigned seqno,
> -					       bool signal_on_any);
> +#define dma_fence_array_create(num, fences, context, seqno, any) ({ \
> +	static struct lock_class_key __key;			\
> +								\
> +	__dma_fence_array_create((num), (fences), (context), (seqno), (any), \
> +		        	 &__key);			\
> +})
> +
> +struct dma_fence_array *__dma_fence_array_create(int num_fences,
> +						 struct dma_fence **fences,
> +						 u64 context, unsigned seqno,
> +						 bool signal_on_any,
> +						 struct lock_class_key *key);
>   
>   bool dma_fence_match_context(struct dma_fence *fence, u64 context);
>