On 7/3/26 10:09, Baineng Shou wrote:
> DMA_HEAP_IOCTL_ALLOC allocates a dma-buf and installs an fd into the
> caller's fd table via fd_install() before dma_heap_ioctl() copies the
> result back to userspace. If the trailing copy_to_user() fails, the
> ioctl returns -EFAULT and userspace never learns the fd number, but
> the fd (and the underlying dma-buf reference) remain in the caller's
> fd table and are leaked for the lifetime of the process.
This came up many many times before and is the completely wrong approach to
handle that.
Background is that this opens up a race window between install_fd() and
close_fd() which is much more worse than the original issue and can be abused
for a couple of different things.
If I'm not completely mistaken we now have automated checkers in place which
find such use case in the upstream kernel and complain about it.
IIRC the correct approach is to call get_unused_fd_flags() to get an fd, do
your things, copy it to user space etc... and then finally do the
fd_install(fd, dmabuf->file) when nothing can go wrong any more.
If an error happens use put_unused_fd() to clean up.
Regards,
Christian.
>
> The failure 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()). Each such ioctl leaks one dmabuf fd; repeating
> the call quickly fills /proc/<pid>/fd with anonymous "/dmabuf:"
> entries that only go away when the process exits.
>
> Fix it by closing the installed fd (and clearing the fd field of the
> kernel-side copy) when copy_to_user() fails after a successful
> allocation, so the error path matches what userspace observes: no fd
> was returned, therefore no fd is left behind.
>
> Fixes: c02a81fba74f ("dma-buf: Add dma-buf heaps framework")
> Cc: [email protected]
> Signed-off-by: Baineng Shou <[email protected]>
> ---
>
> Reproducer (full source, gcc -o poc poc.c; run as root):
>
> // poc.c -- leak one dma-buf fd per DMA_HEAP_IOCTL_ALLOC
> // when copy_to_user() fails
> #include <fcntl.h>
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
> #include <unistd.h>
> #include <sys/ioctl.h>
> #include <sys/mman.h>
> #include <linux/dma-heap.h>
>
> int main(int argc, char **argv)
> {
> int n = argc > 1 ? atoi(argv[1]) : 100;
> long ps = sysconf(_SC_PAGESIZE);
>
> int heap = open("/dev/dma_heap/system", O_RDWR | O_CLOEXEC);
> if (heap < 0)
> return perror("open"), 1;
>
> for (int i = 0; i < n; i++) {
> /* Put a valid request in a page, then make the page
> * read-only: copy_from_user() still succeeds and the
> * dma-buf is allocated and fd_install()'d, but the
> * trailing copy_to_user() fails and the fd, never
> * returned to us, is leaked.
> */
> struct dma_heap_allocation_data *req =
> mmap(NULL, ps, PROT_READ | PROT_WRITE,
> MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
>
> memset(req, 0, sizeof(*req));
> req->len = ps;
> req->fd_flags = O_RDWR | O_CLOEXEC;
>
> mprotect(req, ps, PROT_READ);
> ioctl(heap, DMA_HEAP_IOCTL_ALLOC, req); /* -EFAULT */
> munmap(req, ps);
> }
>
> printf("done: check ls -l /proc/%d/fd for %d leaked fds\n",
> getpid(), n);
> pause();
> return 0;
> }
>
> Before the fix, ./poc 10 leaves 10 anonymous dmabuf fds in the
> caller's fd table:
>
> # ls -l /proc/$(pgrep poc)/fd
> lrwx------ 1 root root 64 Jan 1 00:03 3 -> /dev/dma_heap/system
> lrwx------ 1 root root 64 Jan 1 00:03 4 -> /dmabuf:
> lrwx------ 1 root root 64 Jan 1 00:03 5 -> /dmabuf:
> lrwx------ 1 root root 64 Jan 1 00:03 6 -> /dmabuf:
> lrwx------ 1 root root 64 Jan 1 00:03 7 -> /dmabuf:
> lrwx------ 1 root root 64 Jan 1 00:03 8 -> /dmabuf:
> lrwx------ 1 root root 64 Jan 1 00:03 9 -> /dmabuf:
> lrwx------ 1 root root 64 Jan 1 00:03 10 -> /dmabuf:
> lrwx------ 1 root root 64 Jan 1 00:03 11 -> /dmabuf:
> lrwx------ 1 root root 64 Jan 1 00:03 12 -> /dmabuf:
> lrwx------ 1 root root 64 Jan 1 00:03 13 -> /dmabuf:
>
> After the fix, only /dev/dma_heap/system remains open; the
> anonymous "/dmabuf:" entries are gone.
>
> drivers/dma-buf/dma-heap.c | 11 ++++++++++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c
> index a76bf3f8b071..0dd7a84b06bf 100644
> --- a/drivers/dma-buf/dma-heap.c
> +++ b/drivers/dma-buf/dma-heap.c
> @@ -18,6 +18,7 @@
> #include <linux/uaccess.h>
> #include <linux/xarray.h>
> #include <uapi/linux/dma-heap.h>
> +#include <linux/fdtable.h>
>
> #define DEVNAME "dma_heap"
>
> @@ -181,8 +182,16 @@ static long dma_heap_ioctl(struct file *file, unsigned
> int ucmd,
> goto err;
> }
>
> - if (copy_to_user((void __user *)arg, kdata, out_size) != 0)
> + if (copy_to_user((void __user *)arg, kdata, out_size) != 0) {
> + if (kcmd == DMA_HEAP_IOCTL_ALLOC && ret == 0) {
> + struct dma_heap_allocation_data *h = (void *)kdata;
> +
> + close_fd(h->fd);
> + h->fd = -1;
> + }
> ret = -EFAULT;
> + }
> +
> err:
> if (kdata != stack_kdata)
> kfree(kdata);
