[PATCH] dma-buf: dma-heap: close installed fd when copy_to_user() fails

[PATCH] dma-buf: dma-heap: close installed fd when copy_to_user() fails

[Baineng Shou]

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.

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: stable@vger.kernel.org
Signed-off-by: Baineng Shou <shoubaineng@gmail.com>
---

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);
-- 
2.34.1

Re: [PATCH] dma-buf: dma-heap: close installed fd when copy_to_user() fails

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Calling close_fd() on an installed file descriptor creates a race condition that can inadvertently close an unrelated file descriptor belonging to the process.
--

commit 61669c0c051aa2c310121591ca8897cc10b33840
Author: Baineng Shou <shoubaineng@gmail.com>

dma-buf: dma-heap: close installed fd when copy_to_user() fails

This patch attempts to fix a file descriptor leak when copy_to_user()
fails after allocating a DMA heap buffer. It tries to clean up the
leaked file descriptor by calling close_fd() if the copy to userspace
is unsuccessful.

> diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c
> index a76bf3f8b071a..0dd7a84b06bf4 100644
> --- a/drivers/dma-buf/dma-heap.c
> +++ b/drivers/dma-buf/dma-heap.c
[ ... ]
> @@ -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);

[Severity: High]
Does calling close_fd() on an installed file descriptor create a race
condition here in dma_heap_ioctl()?

By the time the execution reaches this error path, the file descriptor
has already been published to the process's file descriptor table via
fd_install() inside dma_heap_ioctl_allocate(). This means userspace
can already access it.

A concurrent thread could close the newly allocated file descriptor and
open a new file, reusing the same file descriptor number before the
copy_to_user() completes and fails.

If that happens, wouldn't this close_fd() inadvertently close an unrelated
file descriptor belonging to the process?

> +			h->fd = -1;
> +		}
>  		ret = -EFAULT;
> +	}
> +
>  err:
>  	if (kdata != stack_kdata)
>  		kfree(kdata);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260703080922.1838362-1-shoubaineng@gmail.com?part=1

Re: [PATCH] dma-buf: dma-heap: close installed fd when copy_to_user() fails

[Christian König]

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: stable@vger.kernel.org
> Signed-off-by: Baineng Shou <shoubaineng@gmail.com>
> ---
> 
> 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);

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

[Baineng Shou]

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].

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@amd.com/

Fixes: c02a81fba74f ("dma-buf: Add dma-buf heaps framework")
Cc: stable@vger.kernel.org
Signed-off-by: Baineng Shou <shoubaineng@gmail.com>
---
 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);
-- 
2.34.1

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Bypassing the standard dma_buf_fd() API skips the emission of the trace_dma_buf_fd tracepoint.
--

--- Patch [1]: [PATCH v2] dma-buf: dma-heap: don't publish fd before copy_to_user() succeeds ---
commit 869904722955c74d0c6e8c974d3c64c6befabac6
Author: Baineng Shou <shoubaineng@gmail.com>

dma-buf: dma-heap: don't publish fd before copy_to_user() succeeds

This commit restructures DMA_HEAP_IOCTL_ALLOC to delay publishing the
file descriptor until after copy_to_user() succeeds. It open-codes the
fd allocation and installation steps instead of using dma_buf_fd() to
prevent an fd leak on copy_to_user() failure.

Link: https://lore.kernel.org/dri-devel/175e98de-f414-47d7-81c1-c0fe0a8f7f62@amd.com/

> diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c
> index 3937dd41bb0f6..983dee947c851 100644
> --- a/drivers/dma-buf/dma-heap.c
> +++ b/drivers/dma-buf/dma-heap.c
[ ... ]
> @@ -175,15 +162,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);

[Severity: Medium]
By open-coding the fd reservation and installation here instead of using
dma_buf_fd(), does this skip the emission of the trace_dma_buf_fd tracepoint?

If so, observability tools, BPF scripts, and perf tracing relying on
trace_dma_buf_fd to map dmabufs to file descriptors will silently miss all
allocations originating from the dma-heap subsystem.

> +		}
> +
>  		break;
>  	default:
>  		ret = -ENOTTY;
>  		goto err;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710105740.3080070-1-shoubaineng@gmail.com?part=1

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

[T.J. Mercier]

On Fri, Jul 10, 2026 at 4:06 AM <sashiko-bot@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] Bypassing the standard dma_buf_fd() API skips the emission of the trace_dma_buf_fd tracepoint.
> --
>
> --- Patch [1]: [PATCH v2] dma-buf: dma-heap: don't publish fd before copy_to_user() succeeds ---
> commit 869904722955c74d0c6e8c974d3c64c6befabac6
> Author: Baineng Shou <shoubaineng@gmail.com>
>
> dma-buf: dma-heap: don't publish fd before copy_to_user() succeeds
>
> This commit restructures DMA_HEAP_IOCTL_ALLOC to delay publishing the
> file descriptor until after copy_to_user() succeeds. It open-codes the
> fd allocation and installation steps instead of using dma_buf_fd() to
> prevent an fd leak on copy_to_user() failure.
>
> Link: https://lore.kernel.org/dri-devel/175e98de-f414-47d7-81c1-c0fe0a8f7f62@amd.com/
>
> > diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c
> > index 3937dd41bb0f6..983dee947c851 100644
> > --- a/drivers/dma-buf/dma-heap.c
> > +++ b/drivers/dma-buf/dma-heap.c
> [ ... ]
> > @@ -175,15 +162,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);
>
> [Severity: Medium]
> By open-coding the fd reservation and installation here instead of using
> dma_buf_fd(), does this skip the emission of the trace_dma_buf_fd tracepoint?
>
> If so, observability tools, BPF scripts, and perf tracing relying on
> trace_dma_buf_fd to map dmabufs to file descriptors will silently miss all
> allocations originating from the dma-heap subsystem.

Hi Xiang Gao,

It looks like this change in its current form breaks the export
tracing that you introduced. It looks fixable by adding a call to
DMA_BUF_TRACE in after copy_to_user in dma_heap_ioctl. However I'm
wondering if you're aware of the dmabuf iterator that traverses the
dmabuf_list? I've backported it to 5.10, so it's available on all our
kernels: https://r.android.com/q/Iaeda328f7693acc1506269c12cac5e7955e5c091
Someone else from Xiaomi even helped with the 6.12 backport. So I
guess I'm wondering if the dmabuf iterator would work for you instead
of the tracepoints.

Thanks,
T.J.

> > +             }
> > +
> >               break;
> >       default:
> >               ret = -ENOTTY;
> >               goto err;
> >       }
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260710105740.3080070-1-shoubaineng@gmail.com?part=1

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

[寿柏能]

[-- Attachment #1: Type: text/plain, Size: 3986 bytes --]

Hi T.J.,

Thanks for the review.

You are right, the export tracing was inadvertently lost when splitting
dma_buf_fd() apart. I've fixed it in v3 by introducing a new
dma_buf_fd_install() helper in dma-buf.c that wraps the DMA_BUF_TRACE
call together with fd_install(), and using that in dma-heap.c instead
of a bare fd_install(). The trace event fires after copy_to_user()
succeeds, just before the fd becomes visible to userspace — the same
observable moment as before.


Baineng

T.J. Mercier <tjmercier@google.com> 于2026年7月11日周六 04:20写道：

> On Fri, Jul 10, 2026 at 4:06 AM <sashiko-bot@kernel.org> wrote:
> >
> > Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider:
> > - [Medium] Bypassing the standard dma_buf_fd() API skips the emission of
> the trace_dma_buf_fd tracepoint.
> > --
> >
> > --- Patch [1]: [PATCH v2] dma-buf: dma-heap: don't publish fd before
> copy_to_user() succeeds ---
> > commit 869904722955c74d0c6e8c974d3c64c6befabac6
> > Author: Baineng Shou <shoubaineng@gmail.com>
> >
> > dma-buf: dma-heap: don't publish fd before copy_to_user() succeeds
> >
> > This commit restructures DMA_HEAP_IOCTL_ALLOC to delay publishing the
> > file descriptor until after copy_to_user() succeeds. It open-codes the
> > fd allocation and installation steps instead of using dma_buf_fd() to
> > prevent an fd leak on copy_to_user() failure.
> >
> > Link:
> https://lore.kernel.org/dri-devel/175e98de-f414-47d7-81c1-c0fe0a8f7f62@amd.com/
> >
> > > diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c
> > > index 3937dd41bb0f6..983dee947c851 100644
> > > --- a/drivers/dma-buf/dma-heap.c
> > > +++ b/drivers/dma-buf/dma-heap.c
> > [ ... ]
> > > @@ -175,15 +162,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);
> >
> > [Severity: Medium]
> > By open-coding the fd reservation and installation here instead of using
> > dma_buf_fd(), does this skip the emission of the trace_dma_buf_fd
> tracepoint?
> >
> > If so, observability tools, BPF scripts, and perf tracing relying on
> > trace_dma_buf_fd to map dmabufs to file descriptors will silently miss
> all
> > allocations originating from the dma-heap subsystem.
>
> Hi Xiang Gao,
>
> It looks like this change in its current form breaks the export
> tracing that you introduced. It looks fixable by adding a call to
> DMA_BUF_TRACE in after copy_to_user in dma_heap_ioctl. However I'm
> wondering if you're aware of the dmabuf iterator that traverses the
> dmabuf_list? I've backported it to 5.10, so it's available on all our
> kernels: https://r.android.com/q/Iaeda328f7693acc1506269c12cac5e7955e5c091
> Someone else from Xiaomi even helped with the 6.12 backport. So I
> guess I'm wondering if the dmabuf iterator would work for you instead
> of the tracepoints.
>
> Thanks,
> T.J.
>
> > > +             }
> > > +
> > >               break;
> > >       default:
> > >               ret = -ENOTTY;
> > >               goto err;
> > >       }
> >
> > --
> > Sashiko AI review ·
> https://sashiko.dev/#/patchset/20260710105740.3080070-1-shoubaineng@gmail.com?part=1
>

[-- Attachment #2: Type: text/html, Size: 5422 bytes --]

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

[T.J. Mercier]

On Fri, Jul 10, 2026 at 9:18 PM 寿柏能 <shoubaineng@gmail.com> wrote:
>
> Hi T.J.,
>
> Thanks for the review.
>
> You are right, the export tracing was inadvertently lost when splitting
> dma_buf_fd() apart. I've fixed it in v3 by introducing a new
> dma_buf_fd_install() helper in dma-buf.c that wraps the DMA_BUF_TRACE
> call together with fd_install(), and using that in dma-heap.c instead
> of a bare fd_install(). The trace event fires after copy_to_user()
> succeeds, just before the fd becomes visible to userspace — the same
> observable moment as before.
>
>
> Baineng

Thanks Baineng, that LGTM.  Please have my RB:
Reviewed-by: T.J. Mercier <tjmercier@google.com>

You'll probably want to resend this with a "[PATCH v3]" subject though.

Originally I wasn't sure if we really needed dma_buf_fd_install
exported, but I looked around and see that fastrpc_dmabuf_alloc
(drivers/misc/fastrpc.c), gntdev_ioctl_dmabuf_exp_from_refs
(drivers/xen/gntdev-dmabuf.c), and I think video_usercopy
(drivers/media/v4l2-core/v4l2-ioctl.c) also suffer from this same
problem. Want to take a stab at those too? :)

-T.J.


>
> T.J. Mercier <tjmercier@google.com> 于2026年7月11日周六 04:20写道：
>>
>> On Fri, Jul 10, 2026 at 4:06 AM <sashiko-bot@kernel.org> wrote:
>> >
>> > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>> > - [Medium] Bypassing the standard dma_buf_fd() API skips the emission of the trace_dma_buf_fd tracepoint.
>> > --
>> >
>> > --- Patch [1]: [PATCH v2] dma-buf: dma-heap: don't publish fd before copy_to_user() succeeds ---
>> > commit 869904722955c74d0c6e8c974d3c64c6befabac6
>> > Author: Baineng Shou <shoubaineng@gmail.com>
>> >
>> > dma-buf: dma-heap: don't publish fd before copy_to_user() succeeds
>> >
>> > This commit restructures DMA_HEAP_IOCTL_ALLOC to delay publishing the
>> > file descriptor until after copy_to_user() succeeds. It open-codes the
>> > fd allocation and installation steps instead of using dma_buf_fd() to
>> > prevent an fd leak on copy_to_user() failure.
>> >
>> > Link: https://lore.kernel.org/dri-devel/175e98de-f414-47d7-81c1-c0fe0a8f7f62@amd.com/
>> >
>> > > diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c
>> > > index 3937dd41bb0f6..983dee947c851 100644
>> > > --- a/drivers/dma-buf/dma-heap.c
>> > > +++ b/drivers/dma-buf/dma-heap.c
>> > [ ... ]
>> > > @@ -175,15 +162,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);
>> >
>> > [Severity: Medium]
>> > By open-coding the fd reservation and installation here instead of using
>> > dma_buf_fd(), does this skip the emission of the trace_dma_buf_fd tracepoint?
>> >
>> > If so, observability tools, BPF scripts, and perf tracing relying on
>> > trace_dma_buf_fd to map dmabufs to file descriptors will silently miss all
>> > allocations originating from the dma-heap subsystem.
>>
>> Hi Xiang Gao,
>>
>> It looks like this change in its current form breaks the export
>> tracing that you introduced. It looks fixable by adding a call to
>> DMA_BUF_TRACE in after copy_to_user in dma_heap_ioctl. However I'm
>> wondering if you're aware of the dmabuf iterator that traverses the
>> dmabuf_list? I've backported it to 5.10, so it's available on all our
>> kernels: https://r.android.com/q/Iaeda328f7693acc1506269c12cac5e7955e5c091
>> Someone else from Xiaomi even helped with the 6.12 backport. So I
>> guess I'm wondering if the dmabuf iterator would work for you instead
>> of the tracepoints.
>>
>> Thanks,
>> T.J.
>>
>> > > +             }
>> > > +
>> > >               break;
>> > >       default:
>> > >               ret = -ENOTTY;
>> > >               goto err;
>> > >       }
>> >
>> > --
>> > Sashiko AI review · https://sashiko.dev/#/patchset/20260710105740.3080070-1-shoubaineng@gmail.com?part=1

[PATCH v3 0/2] dma-buf: fix fd leak when copy_to_user() fails after fd_install()

[Baineng Shou]

Several drivers call dma_buf_fd() — which internally calls fd_install()
— before copy_to_user() returns the fd number to userspace.  If
copy_to_user() fails, the fd is already published in the caller's fd
table but the ioctl returns an error, so userspace never learns the fd
number.  Worse, the window between fd_install() and copy_to_user()
allows other threads to observe and manipulate the fd (dup, close,
SCM_RIGHTS), making any "close it on the failure path" fix unsafe.

The fix is to split the allocation into three steps: reserve an fd with
get_unused_fd_flags() (not yet visible to other threads), do
copy_to_user(), and only then publish the fd with fd_install() via the
new dma_buf_fd_install() helper.  On copy_to_user() failure,
put_unused_fd() + dma_buf_put() cleanly unwind with no user-visible
side effects.

Patch 1 introduces dma_buf_fd_install() in dma-buf.c (wrapping
fd_install() together with the DMA_BUF_TRACE call to preserve export
tracing) and applies the fix to dma-heap.

Patch 2 applies the same fix to fastrpc, which even had a comment
acknowledging the problem could not be fixed before.

v1: https://lore.kernel.org/dri-devel/20260703080922.1838362-1-shoubaineng@gmail.com/
v2: https://lore.kernel.org/dri-devel/20260710105430.3059661-1-shoubaineng@gmail.com/

Changes in v3:
 - Split into two patches (dma-heap + fastrpc separately)
 - Add dma_buf_fd_install() to preserve trace_dma_buf_fd tracepoint
   (spotted by T.J. Mercier and sashiko-bot on v2)
 - Add fastrpc fix using the new helper (suggested by T.J. Mercier)

Baineng Shou (2):
  dma-buf: dma-heap: don't publish fd before copy_to_user() succeeds
  misc: fastrpc: don't publish fd before copy_to_user() succeeds

 drivers/dma-buf/dma-buf.c  | 20 ++++++++++
 drivers/dma-buf/dma-heap.c | 80 +++++++++++++++++++-------------------
 drivers/misc/fastrpc.c     | 16 +++-----
 include/linux/dma-buf.h    |  1 +
 4 files changed, 67 insertions(+), 50 deletions(-)

-- 
2.34.1

[PATCH v3 1/2] dma-buf: dma-heap: don't publish fd before copy_to_user() succeeds

[Baineng Shou]

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].

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. dma_buf_fd_install()       publishes the fd and emits the
                                trace_dma_buf_fd tracepoint -- from
                                here on the ioctl cannot fail.

A new dma_buf_fd_install() helper is introduced in dma-buf.c to wrap
fd_install() together with the DMA_BUF_TRACE() call, preserving the
export tracing that dma_buf_fd() provides.  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 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@amd.com/

Fixes: c02a81fba74f ("dma-buf: Add dma-buf heaps framework")
Cc: stable@vger.kernel.org
Reviewed-by: T.J. Mercier <tjmercier@google.com>
Signed-off-by: Baineng Shou <shoubaineng@gmail.com>
---
 drivers/dma-buf/dma-buf.c  | 20 ++++++++++
 drivers/dma-buf/dma-heap.c | 80 +++++++++++++++++++-------------------
 include/linux/dma-buf.h    |  1 +
 3 files changed, 61 insertions(+), 40 deletions(-)

diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index d504c636dc29..4c9add51f9ef 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -803,6 +803,26 @@ int dma_buf_fd(struct dma_buf *dmabuf, int flags)
 }
 EXPORT_SYMBOL_NS_GPL(dma_buf_fd, "DMA_BUF");
 
+/**
+ * dma_buf_fd_install - install a reserved fd for a dma-buf
+ * @dmabuf:	[in]	pointer to dma_buf
+ * @fd:		[in]	fd reserved with get_unused_fd_flags()
+ *
+ * Publishes a previously reserved fd into the caller's fd table.
+ * Must only be called after all fallible work (e.g. copy_to_user)
+ * has succeeded, as it cannot be undone safely once called.
+ *
+ * The caller is responsible for having emitted the trace event
+ * (via dma_buf_fd() or get_unused_fd_flags() + this function)
+ * before calling this.
+ */
+void dma_buf_fd_install(struct dma_buf *dmabuf, int fd)
+{
+	DMA_BUF_TRACE(trace_dma_buf_fd, dmabuf, fd);
+	fd_install(fd, dmabuf->file);
+}
+EXPORT_SYMBOL_NS_GPL(dma_buf_fd_install, "DMA_BUF");
+
 /**
  * dma_buf_get - returns the struct dma_buf related to an fd
  * @fd:	[in]	fd associated with the struct dma_buf to be returned
diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c
index a76bf3f8b071..43c32fb28313 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 {
+			dma_buf_fd_install(dmabuf, fd);
+		}
+
 		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);
diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
index d1203da56fc5..d15b2b31d3c9 100644
--- a/include/linux/dma-buf.h
+++ b/include/linux/dma-buf.h
@@ -567,6 +567,7 @@ void dma_buf_unpin(struct dma_buf_attachment *attach);
 struct dma_buf *dma_buf_export(const struct dma_buf_export_info *exp_info);
 
 int dma_buf_fd(struct dma_buf *dmabuf, int flags);
+void dma_buf_fd_install(struct dma_buf *dmabuf, int fd);
 struct dma_buf *dma_buf_get(int fd);
 void dma_buf_put(struct dma_buf *dmabuf);
 
-- 
2.34.1

Re: [PATCH v3 1/2] dma-buf: dma-heap: don't publish fd before copy_to_user() succeeds

[David Laight]

On Tue, 14 Jul 2026 19:46:53 +0800
Baineng Shou <shoubaineng@gmail.com> 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].
...

My 2c:

The other option is just to leave it as a 'problem for user space'.
No reasonable program is going to handle the EFAULT return by doing
anything other than exiting.
Even getting an EFAULT is really an indication that the application
is already in a real mess - most likely with a badly corrupted heap.

Anything else leaves error recovery code in the kernel that is pretty
much never executed and open to a variety of bugs.
While the recovery here is probably ok, there are some sockopt calls
where it is all more complicated.

	David

Re: [PATCH v3 1/2] dma-buf: dma-heap: don't publish fd before copy_to_user() succeeds

[寿柏能]

[-- Attachment #1: Type: text/plain, Size: 2240 bytes --]

Hi David,

Thanks for the feedback.

The concern is not just about the EFAULT return — it's about the race
window between fd_install() and copy_to_user().  Once fd_install()
returns, the fd is immediately observable by other threads in the same
process (via /proc/self/fd, SCM_RIGHTS, etc.), even before
copy_to_user() has a chance to fail.  The triggering condition is a
deliberate mprotect() flip, not a corrupted heap.

The fix itself is small and follows the standard kernel idiom:
get_unused_fd_flags() reserves the fd without publishing it, so the
window between reservation and install is entirely under kernel control.

Baineng

David Laight <david.laight.linux@gmail.com> 于2026年7月14日周二 21:14写道：

> On Tue, 14 Jul 2026 19:46:53 +0800
> Baineng Shou <shoubaineng@gmail.com> 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].
> ...
>
> My 2c:
>
> The other option is just to leave it as a 'problem for user space'.
> No reasonable program is going to handle the EFAULT return by doing
> anything other than exiting.
> Even getting an EFAULT is really an indication that the application
> is already in a real mess - most likely with a badly corrupted heap.
>
> Anything else leaves error recovery code in the kernel that is pretty
> much never executed and open to a variety of bugs.
> While the recovery here is probably ok, there are some sockopt calls
> where it is all more complicated.
>
>         David
>

[-- Attachment #2: Type: text/html, Size: 2784 bytes --]

Re: [PATCH v3 1/2] dma-buf: dma-heap: don't publish fd before copy_to_user() succeeds

[David Laight]

On Tue, 14 Jul 2026 21:38:07 +0800
寿柏能 <shoubaineng@gmail.com> wrote:

> Hi David,
> 
> Thanks for the feedback.
> 
> The concern is not just about the EFAULT return — it's about the race
> window between fd_install() and copy_to_user().  Once fd_install()
> returns, the fd is immediately observable by other threads in the same
> process (via /proc/self/fd, SCM_RIGHTS, etc.), even before
> copy_to_user() has a chance to fail.  The triggering condition is a
> deliberate mprotect() flip, not a corrupted heap.

That is what makes doing the close wrong.
But that is a program aggressively trying to hit the timing window,
not a normal program that has managed to pass an invalid pointer.
The most likely reason for a real program passing an invalid pointer
is a corrupted heap (assuming the stupid coding errors are fixed).

It is really no different from the sockopt code that receives
SCM_RIGHTS messages.
In that case once you've removed the FILE from the socket (or similar)
you really don't want to have to put it back because the write to the
sockopt buffer or length field fails.
The chance of correctly reverting the kernel state is small - and won't
be tested.

	David

> 
> The fix itself is small and follows the standard kernel idiom:
> get_unused_fd_flags() reserves the fd without publishing it, so the
> window between reservation and install is entirely under kernel control.
> 
> Baineng
> 
> David Laight <david.laight.linux@gmail.com> 于2026年7月14日周二 21:14写道：
> 
> > On Tue, 14 Jul 2026 19:46:53 +0800
> > Baineng Shou <shoubaineng@gmail.com> 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].  
> > ...
> >
> > My 2c:
> >
> > The other option is just to leave it as a 'problem for user space'.
> > No reasonable program is going to handle the EFAULT return by doing
> > anything other than exiting.
> > Even getting an EFAULT is really an indication that the application
> > is already in a real mess - most likely with a badly corrupted heap.
> >
> > Anything else leaves error recovery code in the kernel that is pretty
> > much never executed and open to a variety of bugs.
> > While the recovery here is probably ok, there are some sockopt calls
> > where it is all more complicated.
> >
> >         David
> >  

[PATCH v3 2/2] misc: fastrpc: don't publish fd before copy_to_user() succeeds

[Baineng Shou]

fastrpc_ioctl_alloc_dmabuf() calls dma_buf_fd() which installs the fd
into the caller's fd table before copy_to_user() copies the fd number
back to userspace.  If copy_to_user() fails, the fd is already visible
to other threads in the same process but the ioctl returns -EFAULT.
The existing comment in the code even acknowledges the problem:

  "The usercopy failed, but we can't do much about it, as dma_buf_fd()
   already called fd_install()..."

Now that dma_buf_fd_install() is available (introduced to fix the same
issue in dma-heap), apply the same pattern here: reserve the fd with
get_unused_fd_flags(), attempt copy_to_user(), and only on success call
dma_buf_fd_install() to publish it atomically with the tracepoint.  On
copy_to_user() failure, put_unused_fd() and dma_buf_put() cleanly
unwind without any user-visible side effects.

Fixes: 6cffd79504ce ("misc: fastrpc: Add support for dmabuf exporter")
Cc: stable@vger.kernel.org
Signed-off-by: Baineng Shou <shoubaineng@gmail.com>
---
 drivers/misc/fastrpc.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index f3a49384586d..c5143cd25767 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -1709,24 +1709,20 @@ static int fastrpc_dmabuf_alloc(struct fastrpc_user *fl, char __user *argp)
 		return err;
 	}
 
-	bp.fd = dma_buf_fd(buf->dmabuf, O_ACCMODE);
+	bp.fd = get_unused_fd_flags(O_ACCMODE);
 	if (bp.fd < 0) {
 		dma_buf_put(buf->dmabuf);
-		return -EINVAL;
+		return bp.fd;
 	}
 
 	if (copy_to_user(argp, &bp, sizeof(bp))) {
-		/*
-		 * The usercopy failed, but we can't do much about it, as
-		 * dma_buf_fd() already called fd_install() and made the
-		 * file descriptor accessible for the current process. It
-		 * might already be closed and dmabuf no longer valid when
-		 * we reach this point. Therefore "leak" the fd and rely on
-		 * the process exit path to do any required cleanup.
-		 */
+		put_unused_fd(bp.fd);
+		dma_buf_put(buf->dmabuf);
 		return -EFAULT;
 	}
 
+	dma_buf_fd_install(buf->dmabuf, bp.fd);
+
 	return 0;
 }
 
-- 
2.34.1

Re: [PATCH v3 2/2] misc: fastrpc: don't publish fd before copy_to_user() succeeds

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [Critical] The `fastrpc_user` context can be used-after-free if an exported dmabuf outlives the fastrpc device file descriptor.
- [Critical] Concurrent calls to `FASTRPC_IOCTL_MUNMAP` lead to list corruption, Use-After-Free, and Double-Free of a `fastrpc_buf`.
- [Critical] Concurrent calls to `FASTRPC_IOCTL_MEM_UNMAP` cause a Use-After-Free and Double-Free of a `fastrpc_map`.
--

--- Patch [2]: [PATCH v3 2/2] misc: fastrpc: don't publish fd before copy_to_user() succeeds ---
commit 385ee7a3198521d1682f7120112f746dede7b20d
Author: Baineng Shou <shoubaineng@gmail.com>
misc: fastrpc: don't publish fd before copy_to_user() succeeds

This commit fixes an issue where the DMA buffer file descriptor is published 
to userspace before a successful usercopy. It reserves an unused fd, performs 
the copy, and only installs the fd if the copy succeeds. On failure, it 
cleanly unwinds the fd and buffer allocations.

> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index f3a49384586d1..c5143cd257674 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
[ ... ]
> @@ -1709,24 +1709,20 @@ static int fastrpc_dmabuf_alloc(struct fastrpc_user *fl, char __user *argp)
>  		return err;
>  	}
>  
> -	bp.fd = dma_buf_fd(buf->dmabuf, O_ACCMODE);
> +	bp.fd = get_unused_fd_flags(O_ACCMODE);

[Severity: Critical]
This is a pre-existing issue, but does fastrpc_buf_alloc() fail to take a 
reference on the fastrpc_user context?

Looking just above this diff in fastrpc_dmabuf_alloc():

err = fastrpc_buf_alloc(fl, fl->sctx->dev, bp.size, &buf);

This allocates the buffer and assigns the user context to buf->fl, but does 
not increment the user context's reference count. If the fastrpc device file 
descriptor is closed, fastrpc_device_release() drops the reference to fl and 
frees it, meaning the exported dmabuf can outlive it.

If the dmabuf is closed later, fastrpc_release() calls fastrpc_buf_free(), 
which dereferences the freed context here:

fastrpc_ipa_to_dma_addr(buf->fl->cctx, buf->dma_addr)

Can this lead to a use-after-free of the fastrpc_user context?

[Severity: Critical]
This is also a pre-existing issue, but does concurrent execution of 
FASTRPC_IOCTL_MUNMAP lead to list corruption or double-frees?

In fastrpc_req_munmap(), the buf pointer is located under the lock:

buf = iter;
break;

The lock is then dropped without taking a reference count on the buffer. The 
raw pointer is passed to fastrpc_req_munmap_impl(). If two threads invoke 
the ioctl concurrently with the same arguments, could they both find the 
same buf and race? 

They would both concurrently execute this in fastrpc_req_munmap_impl():

list_del(&buf->node);
spin_unlock(&fl->lock);
fastrpc_buf_free(buf);

Does this result in list_del() and fastrpc_buf_free() executing twice on 
the same pointer?

[Severity: Critical]
This is another pre-existing issue, but does fastrpc_req_mem_unmap_impl() 
have a similar race condition?

The map is extracted from the list under the lock, and then the lock is 
dropped without incrementing the map's reference count:

map = iter;
break;
}
}
spin_unlock(&fl->lock);

Multiple threads could extract the same map pointer. If both threads 
successfully proceed, they will both call:

fastrpc_map_put(map);

Since fastrpc_map_get() was never called during the lookup, does the first 
thread drop the reference count to 0 and free the map prematurely? This 
could cause the second thread to trigger a use-after-free on failure paths 
or a double-free when calling fastrpc_map_put() again.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714114654.3885457-1-shoubaineng@gmail.com?part=2

Re: [PATCH v3 0/2] dma-buf: fix fd leak when copy_to_user() fails after fd_install()

[Christian König]

On 7/14/26 13:46, Baineng Shou wrote:
> Several drivers call dma_buf_fd() — which internally calls fd_install()
> — before copy_to_user() returns the fd number to userspace.  If
> copy_to_user() fails, the fd is already published in the caller's fd
> table but the ioctl returns an error, so userspace never learns the fd
> number.  Worse, the window between fd_install() and copy_to_user()
> allows other threads to observe and manipulate the fd (dup, close,
> SCM_RIGHTS), making any "close it on the failure path" fix unsafe.
> 
> The fix is to split the allocation into three steps: reserve an fd with
> get_unused_fd_flags() (not yet visible to other threads), do
> copy_to_user(), and only then publish the fd with fd_install() via the
> new dma_buf_fd_install() helper.  On copy_to_user() failure,
> put_unused_fd() + dma_buf_put() cleanly unwind with no user-visible
> side effects.
> 
> Patch 1 introduces dma_buf_fd_install() in dma-buf.c (wrapping
> fd_install() together with the DMA_BUF_TRACE call to preserve export
> tracing) and applies the fix to dma-heap.
> 
> Patch 2 applies the same fix to fastrpc, which even had a comment
> acknowledging the problem could not be fixed before.

drivers/gpu/drm/drm_prime.c is also using fd_install() of a DMA-buf file descriptor manually.

Would be nice if we could us the new dma_buf_fd_install() for tracing here as well.

Apart from that feel free to add Acked-by: Christian König <christian.koenig@amd.com> to the whole series.

Regards,
Christian.

> 
> v1: https://lore.kernel.org/dri-devel/20260703080922.1838362-1-shoubaineng@gmail.com/
> v2: https://lore.kernel.org/dri-devel/20260710105430.3059661-1-shoubaineng@gmail.com/
> 
> Changes in v3:
>  - Split into two patches (dma-heap + fastrpc separately)
>  - Add dma_buf_fd_install() to preserve trace_dma_buf_fd tracepoint
>    (spotted by T.J. Mercier and sashiko-bot on v2)
>  - Add fastrpc fix using the new helper (suggested by T.J. Mercier)
> 
> Baineng Shou (2):
>   dma-buf: dma-heap: don't publish fd before copy_to_user() succeeds
>   misc: fastrpc: don't publish fd before copy_to_user() succeeds
> 
>  drivers/dma-buf/dma-buf.c  | 20 ++++++++++
>  drivers/dma-buf/dma-heap.c | 80 +++++++++++++++++++-------------------
>  drivers/misc/fastrpc.c     | 16 +++-----
>  include/linux/dma-buf.h    |  1 +
>  4 files changed, 67 insertions(+), 50 deletions(-)
> 

[PATCH v3] drm/prime: use dma_buf_fd_install() to preserve export tracing

[Baineng Shou]

drm_gem_prime_handle_to_fd() open-codes fd reservation and install
using get_unused_fd_flags() + fd_install() directly.  This bypasses
the DMA_BUF_TRACE() call that dma_buf_fd() emits, so observability
tools relying on the trace_dma_buf_fd tracepoint silently miss all
DRM PRIME exports.

Replace the bare fd_install() with dma_buf_fd_install(), which wraps
fd_install() together with DMA_BUF_TRACE(), restoring full tracepoint
coverage.  No functional change; the fd lifecycle (get_unused_fd_flags
→ work → install) is already correct.

Note: this patch depends on dma_buf_fd_install() introduced in
"dma-buf: dma-heap: don't publish fd before copy_to_user() succeeds"
[1].

[1] https://lore.kernel.org/dri-devel/20260714114654.3885457-2-shoubaineng@gmail.com/

Suggested-by: Christian König <christian.koenig@amd.com>
Signed-off-by: Baineng Shou <shoubaineng@gmail.com>
---
 drivers/gpu/drm/drm_prime.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index 9b44c78cd77f..fe3436d1235d 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -524,7 +524,7 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev,
 		return PTR_ERR(dmabuf);
 	}
 
-	fd_install(fd, dmabuf->file);
+	dma_buf_fd_install(dmabuf, fd);
 	*prime_fd = fd;
 	return 0;
 }
-- 
2.34.1

Re: [PATCH v3] drm/prime: use dma_buf_fd_install() to preserve export tracing

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Build failure due to missing dependency `dma_buf_fd_install`.

Pre-existing issues:
- [High] Permanent file descriptor and dmabuf leak if copy_to_user() fails in drm_ioctl.
--

--- Patch [1]: [PATCH v3] drm/prime: use dma_buf_fd_install() to preserve export tracing ---
commit 0d84b19a88b6c4fa09505ca93169d721f3560e20
Author: Baineng Shou <shoubaineng@gmail.com>

drm/prime: use dma_buf_fd_install() to preserve export tracing

This commit replaces fd_install() with dma_buf_fd_install() in
drm_gem_prime_handle_to_fd() to ensure DMA_BUF_TRACE() is called when
exporting a PRIME handle. It notes a dependency on a separate patch
for the new helper.

Link: https://lore.kernel.org/dri-devel/20260714114654.3885457-2-shoubaineng@gmail.com/

> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> index 9b44c78cd77fc..fe3436d1235db 100644
> --- a/drivers/gpu/drm/drm_prime.c
> +++ b/drivers/gpu/drm/drm_prime.c
> @@ -524,7 +524,7 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev,
>  		return PTR_ERR(dmabuf);
>  	}
>  
> -	fd_install(fd, dmabuf->file);
> +	dma_buf_fd_install(dmabuf, fd);
[Severity: High]
Is dma_buf_fd_install() defined in the current tree?

The commit message notes a dependency on a patch that introduces this
function, but without it present in the branch, won't this cause a build
failure due to an implicit function declaration and undefined symbol?

[Severity: High]
This is a pre-existing issue, but can this leak the fd and dmabuf if
copy_to_user() fails later in drm_ioctl()?

If userspace calls the DRM_IOCTL_PRIME_HANDLE_TO_FD ioctl with a read-only
memory mapping for the argument pointer, drm_gem_prime_handle_to_fd() installs
the fd:

drivers/gpu/drm/drm_prime.c:drm_gem_prime_handle_to_fd() {
    ...
    dma_buf_fd_install(dmabuf, fd);
    ...
}

Afterward, the generic ioctl dispatcher attempts to write the resulting fd
back to userspace:

drivers/gpu/drm/drm_ioctl.c:drm_ioctl() {
    ...
    if (copy_to_user((void __user *)arg, kdata, out_size) != 0)
        retcode = -EFAULT;
    ...
}

If this copy fails, the ioctl returns -EFAULT and the user application never
receives the fd number to close it. Will the fd remain open in the process,
permanently leaking the dma_buf and associated GEM object until the process
exits?

>  	*prime_fd = fd;
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714132727.200995-1-shoubaineng@gmail.com?part=1

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@amd.com/
> 
> Fixes: c02a81fba74f ("dma-buf: Add dma-buf heaps framework")
> Cc: stable@vger.kernel.org
> Signed-off-by: Baineng Shou <shoubaineng@gmail.com>

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@amd.com>

> ---
>  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);

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

[Baineng Shou]

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].

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@amd.com/

Fixes: c02a81fba74f ("dma-buf: Add dma-buf heaps framework")
Cc: stable@vger.kernel.org
Signed-off-by: Baineng Shou <shoubaineng@gmail.com>
---
 drivers/dma-buf/dma-buf.c  | 20 ++++++++++
 drivers/dma-buf/dma-heap.c | 80 +++++++++++++++++++-------------------
 include/linux/dma-buf.h    |  1 +
 3 files changed, 61 insertions(+), 40 deletions(-)

diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index d504c636dc29..4c9add51f9ef 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -803,6 +803,26 @@ int dma_buf_fd(struct dma_buf *dmabuf, int flags)
 }
 EXPORT_SYMBOL_NS_GPL(dma_buf_fd, "DMA_BUF");
 
+/**
+ * dma_buf_fd_install - install a reserved fd for a dma-buf
+ * @dmabuf:	[in]	pointer to dma_buf
+ * @fd:		[in]	fd reserved with get_unused_fd_flags()
+ *
+ * Publishes a previously reserved fd into the caller's fd table.
+ * Must only be called after all fallible work (e.g. copy_to_user)
+ * has succeeded, as it cannot be undone safely once called.
+ *
+ * The caller is responsible for having emitted the trace event
+ * (via dma_buf_fd() or get_unused_fd_flags() + this function)
+ * before calling this.
+ */
+void dma_buf_fd_install(struct dma_buf *dmabuf, int fd)
+{
+	DMA_BUF_TRACE(trace_dma_buf_fd, dmabuf, fd);
+	fd_install(fd, dmabuf->file);
+}
+EXPORT_SYMBOL_NS_GPL(dma_buf_fd_install, "DMA_BUF");
+
 /**
  * dma_buf_get - returns the struct dma_buf related to an fd
  * @fd:	[in]	fd associated with the struct dma_buf to be returned
diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c
index a76bf3f8b071..43c32fb28313 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 {
+			dma_buf_fd_install(dmabuf, fd);
+		}
+
 		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);
diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
index d1203da56fc5..d15b2b31d3c9 100644
--- a/include/linux/dma-buf.h
+++ b/include/linux/dma-buf.h
@@ -567,6 +567,7 @@ void dma_buf_unpin(struct dma_buf_attachment *attach);
 struct dma_buf *dma_buf_export(const struct dma_buf_export_info *exp_info);
 
 int dma_buf_fd(struct dma_buf *dmabuf, int flags);
+void dma_buf_fd_install(struct dma_buf *dmabuf, int fd);
 struct dma_buf *dma_buf_get(int fd);
 void dma_buf_put(struct dma_buf *dmabuf);
 
-- 
2.34.1

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Pre-existing Use-After-Free in dma_buf_fd() via concurrent close() after fd installation.
--

--- Patch [1]: [PATCH v2] dma-buf: dma-heap: don't publish fd before copy_to_user() succeeds ---
commit a57039a5644f218873e246c9fb9fe2f87ef4dc55
Author: Baineng Shou <shoubaineng@gmail.com>

dma-buf: dma-heap: don't publish fd before copy_to_user() succeeds

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.

Link: https://lore.kernel.org/dri-devel/175e98de-f414-47d7-81c1-c0fe0a8f7f62@amd.com/

> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> index 71f37544a5c61..c2f455d028b30 100644
> --- a/drivers/dma-buf/dma-buf.c
> +++ b/drivers/dma-buf/dma-buf.c
> @@ -799,6 +799,26 @@ int dma_buf_fd(struct dma_buf *dmabuf, int flags)
>  }
>  EXPORT_SYMBOL_NS_GPL(dma_buf_fd, "DMA_BUF");

[Severity: Critical]
This is a pre-existing issue, but does dma_buf_fd() have a use-after-free
if the file descriptor is closed concurrently?

Looking at dma_buf_fd() just above this diff:

    fd = FD_ADD(flags, dmabuf->file);
    DMA_BUF_TRACE(trace_dma_buf_fd, dmabuf, fd);

Once FD_ADD() installs the fd, it becomes visible to userspace. If a
concurrent thread guesses the fd and calls close(fd), the file's
reference count drops to zero, triggering fput() and eventually freeing
the dmabuf structure.

When the original thread continues to DMA_BUF_TRACE(), won't it
dereference the now-freed dmabuf pointer?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260711041455.3375292-1-shoubaineng@gmail.com?part=1