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/[email protected]/ > > Fixes: c02a81fba74f ("dma-buf: Add dma-buf heaps framework") > Cc: [email protected] > Signed-off-by: Baineng Shou <[email protected]> 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 <[email protected]> > --- > 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);
