Re: [PATCH v2] dma-buf: dma-heap: don't publish fd before copy_to_user() succeeds

[Christian König]

On 7/10/26 12:57, Baineng Shou wrote:
> DMA_HEAP_IOCTL_ALLOC allocates a dma-buf and installs an fd into the
> caller's fd table via dma_buf_fd() -> fd_install() before
> dma_heap_ioctl() copies the result back to userspace.  If the trailing
> copy_to_user() fails, userspace never learns the fd number, but the
> fd (and the underlying dma-buf reference) are already visible to
> other threads in the same process and are leaked for the lifetime of
> the process.
> 
> The obvious "close it on the failure path" fix is unsafe: once
> fd_install() has run, another thread can already dup() the fd, send
> it via SCM_RIGHTS, or close() it and let its number be reused, so a
> subsequent close_fd() from the ioctl path can operate on an unrelated
> file.  This was pointed out by Christian König on v1 [1].

IIRC it was Greg who pointed that out numerous times, I'm just repeating what I was told.

> 
> Restructure the allocation path so that fd_install() is the last,
> unfailable step of a successful ioctl:
> 
>   1. heap->ops->allocate()      creates the dma_buf.
>   2. get_unused_fd_flags()      reserves an fd number in the caller's
>                                 fd table without publishing it, so
>                                 no other thread can observe it.
>   3. copy_to_user()             delivers the fd number to userspace;
>                                 on failure the fd is returned with
>                                 put_unused_fd() and the dma_buf
>                                 reference is dropped with
>                                 dma_buf_put(), leaving no user-
>                                 visible state behind.
>   4. fd_install()               publishes the fd -- from here on the
>                                 ioctl cannot fail.
> 
> To make this possible, dma_heap_ioctl_allocate() is refactored to
> return the struct dma_buf * directly (returning ERR_PTR on failure)
> so the caller holds the dmabuf reference across steps 3 and 4.
> The fd is written into the kdata buffer before copy_to_user() so
> the reserved fd number reaches userspace atomically with the install.
> 
> The failure at step 3 is easily reachable from userspace: pass a
> struct dma_heap_allocation_data that lives in a page whose protection
> is flipped to PROT_READ between copy_from_user() and copy_to_user()
> (e.g. via mprotect()).  Before this change each such ioctl leaks one
> dmabuf fd; after it, the fd table is unchanged on failure and only
> /dev/dma_heap/<name> remains open.
> 
> No UAPI or heap-driver interface change.
> 
> [1] https://lore.kernel.org/dri-devel/175e98de-f414-47d7-81c1-c0fe0a8f7f62@xxxxxxx/
> 
> Fixes: c02a81fba74f ("dma-buf: Add dma-buf heaps framework")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Baineng Shou <shoubaineng@xxxxxxxxx>

Patch looks sane to me, but somebody with more background in DMA-buf heaps should probably take a look as well.

Acked-by: Christian König <christian.koenig@xxxxxxx>

> ---
>  drivers/dma-buf/dma-heap.c | 80 +++++++++++++++++++-------------------
>  1 file changed, 40 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c
> index a76bf3f8b071..0a9bf62eb06c 100644
> --- a/drivers/dma-buf/dma-heap.c
> +++ b/drivers/dma-buf/dma-heap.c
> @@ -55,33 +55,6 @@ MODULE_PARM_DESC(mem_accounting,
>  		 "Enable cgroup-based memory accounting for dma-buf heap allocations (default=false).");
>  EXPORT_SYMBOL_NS_GPL(mem_accounting, "DMA_BUF_HEAP");
>  
> -static int dma_heap_buffer_alloc(struct dma_heap *heap, size_t len,
> -				 u32 fd_flags,
> -				 u64 heap_flags)
> -{
> -	struct dma_buf *dmabuf;
> -	int fd;
> -
> -	/*
> -	 * Allocations from all heaps have to begin
> -	 * and end on page boundaries.
> -	 */
> -	len = PAGE_ALIGN(len);
> -	if (!len)
> -		return -EINVAL;
> -
> -	dmabuf = heap->ops->allocate(heap, len, fd_flags, heap_flags);
> -	if (IS_ERR(dmabuf))
> -		return PTR_ERR(dmabuf);
> -
> -	fd = dma_buf_fd(dmabuf, fd_flags);
> -	if (fd < 0) {
> -		dma_buf_put(dmabuf);
> -		/* just return, as put will call release and that will free */
> -	}
> -	return fd;
> -}
> -
>  static int dma_heap_open(struct inode *inode, struct file *file)
>  {
>  	struct dma_heap *heap;
> @@ -99,30 +72,42 @@ static int dma_heap_open(struct inode *inode, struct file *file)
>  	return 0;
>  }
>  
> -static long dma_heap_ioctl_allocate(struct file *file, void *data)
> +static struct dma_buf *dma_heap_ioctl_allocate(struct file *file, void *data)
>  {
>  	struct dma_heap_allocation_data *heap_allocation = data;
>  	struct dma_heap *heap = file->private_data;
> +	struct dma_buf *dmabuf;
>  	int fd;
> +	size_t len;
>  
>  	if (heap_allocation->fd)
> -		return -EINVAL;
> +		return ERR_PTR(-EINVAL);
>  
>  	if (heap_allocation->fd_flags & ~DMA_HEAP_VALID_FD_FLAGS)
> -		return -EINVAL;
> +		return ERR_PTR(-EINVAL);
>  
>  	if (heap_allocation->heap_flags & ~DMA_HEAP_VALID_HEAP_FLAGS)
> -		return -EINVAL;
> +		return ERR_PTR(-EINVAL);
> +
> +	len = PAGE_ALIGN(heap_allocation->len);
> +	if (!len)
> +		return ERR_PTR(-EINVAL);
> +
> +	dmabuf = heap->ops->allocate(heap, len, heap_allocation->fd_flags,
> +				     heap_allocation->heap_flags);
>  
> -	fd = dma_heap_buffer_alloc(heap, heap_allocation->len,
> -				   heap_allocation->fd_flags,
> -				   heap_allocation->heap_flags);
> -	if (fd < 0)
> -		return fd;
> +	if (IS_ERR(dmabuf))
> +		return dmabuf;
> +
> +	fd = get_unused_fd_flags(heap_allocation->fd_flags);
> +	if (fd < 0) {
> +		dma_buf_put(dmabuf);
> +		return ERR_PTR(fd);
> +	}
>  
>  	heap_allocation->fd = fd;
>  
> -	return 0;
> +	return dmabuf;
>  }
>  
>  static unsigned int dma_heap_ioctl_cmds[] = {
> @@ -138,6 +123,8 @@ static long dma_heap_ioctl(struct file *file, unsigned int ucmd,
>  	unsigned int in_size, out_size, drv_size, ksize;
>  	int nr = _IOC_NR(ucmd);
>  	int ret = 0;
> +	int fd;
> +	struct dma_buf *dmabuf;
>  
>  	if (nr >= ARRAY_SIZE(dma_heap_ioctl_cmds))
>  		return -EINVAL;
> @@ -174,15 +161,28 @@ static long dma_heap_ioctl(struct file *file, unsigned int ucmd,
>  
>  	switch (kcmd) {
>  	case DMA_HEAP_IOCTL_ALLOC:
> -		ret = dma_heap_ioctl_allocate(file, kdata);
> +		dmabuf = dma_heap_ioctl_allocate(file, kdata);
> +
> +		if (IS_ERR(dmabuf)) {
> +			ret = PTR_ERR(dmabuf);
> +			break;
> +		}
> +
> +		fd = ((struct dma_heap_allocation_data *)kdata)->fd;
> +		if (copy_to_user((void __user *)arg, kdata, out_size) != 0) {
> +			put_unused_fd(fd);
> +			dma_buf_put(dmabuf);
> +			ret = -EFAULT;
> +		} else {
> +			fd_install(fd, dmabuf->file);
> +		}
> +
>  		break;
>  	default:
>  		ret = -ENOTTY;
>  		goto err;
>  	}
>  
> -	if (copy_to_user((void __user *)arg, kdata, out_size) != 0)
> -		ret = -EFAULT;
>  err:
>  	if (kdata != stack_kdata)
>  		kfree(kdata);