dma-buf: dma-heap: close installed fd when copy_to_user() fails
Subsystems (10)
PATCH 1/1 dma-buf: dma-heap: close installed fd when copy_to_user() fails
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.
---
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(-)
drivers/dma-buf/dma-heap.c +10 -1
@@ -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);
Signature · 1 line
2.34.1
[email protected]
AI reviewThank 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…
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 <[email protected]>
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.
Quoted 4 lines
drivers/dma-buf/dma-heap.c +0
-0
[ ... ]
drivers/dma-buf/dma-heap.c static long dma_heap_ioctl(struct file *file, unsigned int ucmd, · line 189
Quoted 10 lines
drivers/dma-buf/dma-heap.c +5
-1@@ -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?
Quoted 8 lines
drivers/dma-buf/dma-heap.c +4
-0+ h->fd =-1 ; + } ret = -EFAULT ; + } + err:if (kdata != stack_kdata)kfree (kdata);
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
Christian König
maintainerQuoted 6 lines · Baineng Shou
DMA_HEAP_IOCTL_ALLOCallocates a dma-buf and installs an fd into the caller's fd table viafd_install()beforedma_heap_ioctl()copies the result back to userspace. If the trailingcopy_to_user()fails, the ioctl returns-EFAULTand 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.
Quoted 118 lines
The failure is easily reachable from userspace: pass a struct
dma_heap_allocation_datathat lives in a page whose protection is flipped toPROT_READbetweencopy_from_user()andcopy_to_user()(e.g. viamprotect()). 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")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/systemremains open; the anonymous "/dmabuf:" entries are gone.drivers/dma-buf/dma-heap.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)drivers/dma-buf/dma-heap.c +10
-1@@ -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 1/1 dma-buf: dma-heap: don't publish fd before copy_to_user() succeeds
In reply to Baineng ShouDMA_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/[email protected]/
--- drivers/dma-buf/dma-heap.c | 80 +++++++++++++++++++------------------- 1 file changed, 40 insertions(+), 40 deletions(-)
drivers/dma-buf/dma-heap.c +40 -40
@@ -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);
Signature · 1 line
2.34.1
[email protected]
AI reviewRe: [PATCH v2] dma-buf: dma-heap: don't publish fd before copy_to_user() succeeds
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. --
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 <[email protected]>
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.
Quoted 4 lines
drivers/dma-buf/dma-heap.c +0
-0
[ ... ]
Quoted 9 lines
drivers/dma-buf/dma-heap.c +4
-1@@ -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);
drivers/dma-buf/dma-heap.c static long dma_heap_ioctl(struct file *file, unsigned int ucmd, · line 178
Quoted 10 lines
drivers/dma-buf/dma-heap.c +10
-0+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.
Quoted 7 lines
drivers/dma-buf/dma-heap.c +2
-0+ } +break ;default : ret = -ENOTTY ; goto err; }
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
T.J. Mercier
reviewerRe: [PATCH v2] dma-buf: dma-heap: don't publish fd before copy_to_user() succeeds
Quoted 47 lines · On Fri, Jul 10, 2026 at 4:06 AM <[email protected]> wrote:
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Bypassing the standarddma_buf_fd()API skips the emission of thetrace_dma_buf_fdtracepoint. ----- Patch [1]: [PATCH v2] dma-buf: dma-heap: don't publish fd before
copy_to_user()succeeds --- commit869904722955c74d0c6e8c974d3c64c6befabac6
Author: Baineng Shou <[email protected]>dma-buf: dma-heap: don't publish fd before
copy_to_user()succeedsThis commit restructures
DMA_HEAP_IOCTL_ALLOCto delay publishing the file descriptor until aftercopy_to_user()succeeds. It open-codes the fd allocation and installation steps instead of usingdma_buf_fd()to prevent an fd leak oncopy_to_user()failure.Quoted 4 lines
drivers/dma-buf/dma-heap.c +0
-0[ ... ]
Quoted 9 lines
drivers/dma-buf/dma-heap.c +4
-1@@ -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);drivers/dma-buf/dma-heap.c static long dma_heap_ioctl(struct file *file, unsigned int ucmd, · line 178
Quoted 10 lines
drivers/dma-buf/dma-heap.c +10
-0+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 usingdma_buf_fd(), does this skip the emission of thetrace_dma_buf_fdtracepoint?
Quoted 3 lines
If so, observability tools, BPF scripts, and perf tracing relying on
trace_dma_buf_fdto 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.
Quoted 10 lines
drivers/dma-buf/dma-heap.c static long dma_heap_ioctl(struct file *file, unsigned int ucmd, · line 184
Quoted 7 lines
drivers/dma-buf/dma-heap.c +2
-0+ } +break ;default : ret = -ENOTTY ; goto err; }--
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
Re: [PATCH v2] dma-buf: dma-heap: don't publish fd before copy_to_user() succeeds
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
Quoted 87 lines · T.J. Mercier <[email protected]> 于2026年7月11日周六 04:20写道:
Quoted 2 lines · On Fri, Jul 10, 2026 at 4:06 AM <[email protected]> wrote:
Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:
Quoted 1 line
- [Medium] Bypassing the standard
dma_buf_fd()API skips the emission ofthe
trace_dma_buf_fdtracepoint.Quoted 3 lines
--
--- Patch [1]: [PATCH v2] dma-buf: dma-heap: don't publish fd before
copy_to_user()succeeds ---Quoted 11 lines
commit
869904722955c74d0c6e8c974d3c64c6befabac6
Author: Baineng Shou <[email protected]>dma-buf: dma-heap: don't publish fd before
copy_to_user()succeedsThis commit restructures
DMA_HEAP_IOCTL_ALLOCto delay publishing the file descriptor until aftercopy_to_user()succeeds. It open-codes the fd allocation and installation steps instead of usingdma_buf_fd()to prevent an fd leak oncopy_to_user()failure.Link:
https://lore.kernel.org/dri-devel/[email protected]/
Quoted 7 lines
Quoted 5 lines
drivers/dma-buf/dma-heap.c +0
-0[ ... ]
drivers/dma-buf/dma-heap.c static long dma_heap_ioctl(struct file *file,
Quoted 1 line
drivers/dma-buf/dma-heap.c +0
-0@@ -175,15 +162,28 @@ static long dma_heap_ioctl(struct file *file,unsigned int ucmd,
Quoted 13 lines
drivers/dma-buf/dma-heap.c static long dma_heap_ioctl(struct file *file, unsigned int ucmd,
Quoted 13 lines
drivers/dma-buf/dma-heap.c +9
-1switch (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) {
Quoted 9 lines
drivers/dma-buf/dma-heap.c static long dma_heap_ioctl(struct file *file, unsigned int ucmd, · line 177
Quoted 5 lines
drivers/dma-buf/dma-heap.c +5
-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 usingdma_buf_fd(), does this skip the emission of thetrace_dma_buf_fdtracepoint?
Quoted 3 lines
If so, observability tools, BPF scripts, and perf tracing relying on
trace_dma_buf_fdto map dmabufs to file descriptors will silently missall
Quoted 1 line
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_TRACEin aftercopy_to_userindma_heap_ioctl. However I'm wondering if you're aware of the dmabuf iterator that traverses thedmabuf_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.Quoted 10 lines
drivers/dma-buf/dma-heap.c static long dma_heap_ioctl(struct file *file, unsigned int ucmd, · line 184
Quoted 7 lines
drivers/dma-buf/dma-heap.c +2
-0+ } +break ;default : ret = -ENOTTY ; goto err; }--
Sashiko AI review ·
Re: [PATCH v2] dma-buf: dma-heap: don't publish fd before copy_to_user() succeeds
Quoted 14 lines · 寿柏能
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 newdma_buf_fd_install()helper indma-buf.cthat wraps theDMA_BUF_TRACEcall together withfd_install(), and using that indma-heap.cinstead of a barefd_install(). The trace event fires aftercopy_to_user()succeeds, just before the fd becomes visible to userspace — the same observable moment as before.
Quoted 1 line
Baineng
Thanks Baineng, that LGTM. Please have my RB:
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.
Quoted 80 lines
Quoted 78 lines · T.J. Mercier <[email protected]> 于2026年7月11日周六 04:20写道:
Quoted 47 lines · On Fri, Jul 10, 2026 at 4:06 AM <[email protected]> wrote:
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Bypassing the standarddma_buf_fd()API skips the emission of thetrace_dma_buf_fdtracepoint. ----- Patch [1]: [PATCH v2] dma-buf: dma-heap: don't publish fd before
copy_to_user()succeeds --- commit869904722955c74d0c6e8c974d3c64c6befabac6
Author: Baineng Shou <[email protected]>dma-buf: dma-heap: don't publish fd before
copy_to_user()succeedsThis commit restructures
DMA_HEAP_IOCTL_ALLOCto delay publishing the file descriptor until aftercopy_to_user()succeeds. It open-codes the fd allocation and installation steps instead of usingdma_buf_fd()to prevent an fd leak oncopy_to_user()failure.Quoted 4 lines
drivers/dma-buf/dma-heap.c +0
-0[ ... ]
Quoted 9 lines
drivers/dma-buf/dma-heap.c +4
-1@@ -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);drivers/dma-buf/dma-heap.c static long dma_heap_ioctl(struct file *file, unsigned int ucmd, · line 178
Quoted 10 lines
drivers/dma-buf/dma-heap.c +10
-0+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 usingdma_buf_fd(), does this skip the emission of thetrace_dma_buf_fdtracepoint?Quoted 3 lines
If so, observability tools, BPF scripts, and perf tracing relying on
trace_dma_buf_fdto 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_TRACEin aftercopy_to_userindma_heap_ioctl. However I'm wondering if you're aware of the dmabuf iterator that traverses thedmabuf_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.Quoted 10 lines
drivers/dma-buf/dma-heap.c static long dma_heap_ioctl(struct file *file, unsigned int ucmd, · line 184
Quoted 7 lines
drivers/dma-buf/dma-heap.c +2
-0+ } +break ;default : ret = -ENOTTY ; goto err; }--
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
v3 cover dma-buf: fix fd leak when copy_to_user() fails after fd_install()
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/[email protected]/
v2: https://lore.kernel.org/dri-devel/[email protected]/
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(-)
Signature · 1 line
2.34.1
PATCH v3 1/2 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. 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/[email protected]/
--- 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(-)
drivers/dma-buf/dma-buf.c +20 -0
@@ -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
drivers/dma-buf/dma-heap.c +40 -40
124 lines not on this page · raw message
include/linux/dma-buf.h +1 -0
@@ -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);
Signature · 1 line
2.34.1
Re: [PATCH v3 1/2] dma-buf: dma-heap: don't publish fd before copy_to_user() succeeds
Quoted 8 lines · On Tue, 14 Jul 2026 19:46:53 +0800 Baineng Shou <[email protected]> wrote:
DMA_HEAP_IOCTL_ALLOCallocates a dma-buf and installs an fd into the caller's fd table viadma_buf_fd()->fd_install()beforedma_heap_ioctl()copies the result back to userspace. If the trailingcopy_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.
Quoted 5 lines
The obvious "close it on the failure path" fix is unsafe: once
fd_install()has run, another thread can alreadydup()the fd, send it viaSCM_RIGHTS, orclose()it and let its number be reused, so a subsequentclose_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
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
Quoted 33 lines · David Laight <[email protected]> 于2026年7月14日周二 21:14写道:
Quoted 8 lines · On Tue, 14 Jul 2026 19:46:53 +0800 Baineng Shou <[email protected]> wrote:
DMA_HEAP_IOCTL_ALLOCallocates a dma-buf and installs an fd into the caller's fd table viadma_buf_fd()->fd_install()beforedma_heap_ioctl()copies the result back to userspace. If the trailingcopy_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.Quoted 5 lines
The obvious "close it on the failure path" fix is unsafe: once
fd_install()has run, another thread can alreadydup()the fd, send it viaSCM_RIGHTS, orclose()it and let its number be reused, so a subsequentclose_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
Quoted 10 lines · On Tue, 14 Jul 2026 21:38:07 +0800 寿柏能 <[email protected]> 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()andcopy_to_user(). Oncefd_install()returns, the fd is immediately observable by other threads in the same process (via/proc/self/fd,SCM_RIGHTS, etc.), even beforecopy_to_user()has a chance to fail. The triggering condition is a deliberatemprotect()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
Quoted 42 lines
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
Quoted 33 lines · David Laight <[email protected]> 于2026年7月14日周二 21:14写道:
Quoted 8 lines · On Tue, 14 Jul 2026 19:46:53 +0800 Baineng Shou <[email protected]> wrote:
DMA_HEAP_IOCTL_ALLOCallocates a dma-buf and installs an fd into the caller's fd table viadma_buf_fd()->fd_install()beforedma_heap_ioctl()copies the result back to userspace. If the trailingcopy_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.Quoted 5 lines
The obvious "close it on the failure path" fix is unsafe: once
fd_install()has run, another thread can alreadydup()the fd, send it viaSCM_RIGHTS, orclose()it and let its number be reused, so a subsequentclose_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
Hi David
The rollback here is the simplest possible case (put_unused_fd + dma_buf_put), not the complex sockopt scenario you describe.
Baineng
Quoted 75 lines · David Laight <[email protected]> 于2026年7月14日周二 22:33写道:
Quoted 10 lines · On Tue, 14 Jul 2026 21:38:07 +0800 寿柏能 <[email protected]> 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()andcopy_to_user(). Oncefd_install()returns, the fd is immediately observable by other threads in the same process (via/proc/self/fd,SCM_RIGHTS, etc.), even beforecopy_to_user()has a chance to fail. The triggering condition is a deliberatemprotect()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_RIGHTSmessages.
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
Quoted 15 lines
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
Quoted 6 lines · David Laight <[email protected]> 于2026年7月14日周二 21:14写道:
Quoted 3 lines · On Tue, 14 Jul 2026 19:46:53 +0800 Baineng Shou <[email protected]> wrote:
DMA_HEAP_IOCTL_ALLOCallocates a dma-buf and installs an fd into the caller's fd table viadma_buf_fd()->fd_install()beforedma_heap_ioctl()copies the result back to userspace. If thetrailing
Quoted 27 lines
Quoted 27 lines
Quoted 10 lines
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 alreadydup()the fd, send it viaSCM_RIGHTS, orclose()it and let its number be reused, so a subsequentclose_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
A message that is not in the archive (sent off-list or not yet received): CAGCp47zxaZsoBKeXz2YdyxbX8QOy_g8dGwnC9gVpJ-Sqng48Qg@mail.gmail.com
Sumit Semwal
maintainerRe: [PATCH v3 1/2] dma-buf: dma-heap: don't publish fd before copy_to_user() succeeds
Hello Baineng,
Quoted 2 lines · 寿柏能
Ping. Any chance this can be picked up?
Thank you for the patch.
While discussing with T J, we realised that adding a selftest for this would also be helpful. Could you please re-submit with a selftest added?
Quoted 2 lines
Baineng
Thanks and best regards,
Sumit.
Quoted 85 lines
Quoted 83 lines · 寿柏能 <[email protected]> 于2026年7月15日周三 10:04写道:
Hi David
The rollback here is the simplest possible case (
put_unused_fd+dma_buf_put), not the complex sockopt scenario you describe.Baineng
Quoted 74 lines · David Laight <[email protected]> 于2026年7月14日周二 22:33写道:
Quoted 10 lines · On Tue, 14 Jul 2026 21:38:07 +0800 寿柏能 <[email protected]> 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()andcopy_to_user(). Oncefd_install()returns, the fd is immediately observable by other threads in the same process (via/proc/self/fd,SCM_RIGHTS, etc.), even beforecopy_to_user()has a chance to fail. The triggering condition is a deliberatemprotect()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_RIGHTSmessages.
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
Quoted 42 lines
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
Quoted 33 lines · David Laight <[email protected]> 于2026年7月14日周二 21:14写道:
Quoted 8 lines · On Tue, 14 Jul 2026 19:46:53 +0800 Baineng Shou <[email protected]> wrote:
DMA_HEAP_IOCTL_ALLOCallocates a dma-buf and installs an fd into the caller's fd table viadma_buf_fd()->fd_install()beforedma_heap_ioctl()copies the result back to userspace. If the trailingcopy_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.Quoted 5 lines
The obvious "close it on the failure path" fix is unsafe: once
fd_install()has run, another thread can alreadydup()the fd, send it viaSCM_RIGHTS, orclose()it and let its number be reused, so a subsequentclose_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
v5 cover dma-buf: fix fd leak when copy_to_user() fails after fd_install()
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.
Patch 3 replaces the bare fd_install() in drm_gem_prime_handle_to_fd() with dma_buf_fd_install() to restore tracepoint coverage for DRM PRIME exports (suggested by Christian König).
Patch 4 adds a selftest to tools/testing/selftests/dmabuf-heaps/ that reproduces the fd-leak scenario (mprotect flip between copy_from_user and copy_to_user) and verifies the fd count is unchanged after a failed ioctl (suggested by Sumit Semwal).
v1: https://lore.kernel.org/dri-devel/[email protected]/
v2: https://lore.kernel.org/dri-devel/[email protected]/
v3: https://lore.kernel.org/dri-devel/[email protected]/
v4: https://lore.kernel.org/dri-devel/[email protected]/
Changes in v5:
- Add selftest (patch 4) reproducing the fd-leak scenario (Sumit Semwal)
Changes in v4:
- Add patch 3: drm/prime: use dma_buf_fd_install() (Christian König)
- Add Acked-by: Christian König to patches 1 and 2
Changes in v3:
- Split into two patches (dma-heap + fastrpc separately)
- Add dma_buf_fd_install() to preserve trace_dma_buf_fd tracepoint
- Add fastrpc fix using the new helper (T.J. Mercier)
Baineng Shou (4):
dma-buf: dma-heap: don't publish fd before copy_to_user() succeeds
misc: fastrpc: don't publish fd before copy_to_user() succeeds drm/prime: use dma_buf_fd_install() to preserve export tracing
selftests: dmabuf-heaps: add fd-leak-on-EFAULT regression test
drivers/dma-buf/dma-buf.c | 20 +++ drivers/dma-buf/dma-heap.c | 80 ++++++------ drivers/gpu/drm/drm_prime.c | 2 +- drivers/misc/fastrpc.c | 16 +-- include/linux/dma-buf.h | 1 + .../selftests/dmabuf-heaps/dmabuf-heap.c | 115 +++++++++++++++++- 6 files changed, 182 insertions(+), 52 deletions(-)
Signature · 1 line
2.34.1
PATCH v5 1/4 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. 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/[email protected]/
--- 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(-)
drivers/dma-buf/dma-buf.c +20 -0
@@ -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
drivers/dma-buf/dma-heap.c +40 -40
124 lines not on this page · raw message
include/linux/dma-buf.h +1 -0
@@ -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);
Signature · 1 line
2.34.1
PATCH v5 2/4 misc: fastrpc: don't publish fd before copy_to_user() succeeds
In reply to Baineng Shoufastrpc_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.
--- drivers/misc/fastrpc.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-)
drivers/misc/fastrpc.c +6 -10
@@ -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 ; }
Signature · 1 line
2.34.1
v5 cover dma-buf: fix fd leak when copy_to_user() fails after fd_install()
In reply to Sumit SemwalSeveral 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.
Patch 3 replaces the bare fd_install() in drm_gem_prime_handle_to_fd() with dma_buf_fd_install() to restore tracepoint coverage for DRM PRIME exports (suggested by Christian König).
Patch 4 adds a selftest to tools/testing/selftests/dmabuf-heaps/ that reproduces the fd-leak scenario (mprotect flip between copy_from_user and copy_to_user) and verifies the fd count is unchanged after a failed ioctl (suggested by Sumit Semwal).
v1: https://lore.kernel.org/dri-devel/[email protected]/
v2: https://lore.kernel.org/dri-devel/[email protected]/
v3: https://lore.kernel.org/dri-devel/[email protected]/
v4: https://lore.kernel.org/dri-devel/[email protected]/
Changes in v5:
- Add selftest (patch 4) reproducing the fd-leak scenario (Sumit Semwal)
Changes in v4:
- Add patch 3: drm/prime: use dma_buf_fd_install() (Christian König)
- Add Acked-by: Christian König to patches 1 and 2
Changes in v3:
- Split into two patches (dma-heap + fastrpc separately)
- Add dma_buf_fd_install() to preserve trace_dma_buf_fd tracepoint
- Add fastrpc fix using the new helper (T.J. Mercier)
Baineng Shou (4):
dma-buf: dma-heap: don't publish fd before copy_to_user() succeeds
misc: fastrpc: don't publish fd before copy_to_user() succeeds drm/prime: use dma_buf_fd_install() to preserve export tracing
selftests: dmabuf-heaps: add fd-leak-on-EFAULT regression test
drivers/dma-buf/dma-buf.c | 20 +++ drivers/dma-buf/dma-heap.c | 80 ++++++------ drivers/gpu/drm/drm_prime.c | 2 +- drivers/misc/fastrpc.c | 16 +-- include/linux/dma-buf.h | 1 + .../selftests/dmabuf-heaps/dmabuf-heap.c | 115 +++++++++++++++++- 6 files changed, 182 insertions(+), 52 deletions(-)
Signature · 1 line
2.34.1
PATCH v5 1/4 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. 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/[email protected]/
--- 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(-)
drivers/dma-buf/dma-buf.c +20 -0
@@ -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
drivers/dma-buf/dma-heap.c +40 -40
124 lines not on this page · raw message
include/linux/dma-buf.h +1 -0
@@ -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);
Signature · 1 line
2.34.1
PATCH v5 2/4 misc: fastrpc: don't publish fd before copy_to_user() succeeds
In reply to Baineng Shoufastrpc_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.
--- drivers/misc/fastrpc.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-)
drivers/misc/fastrpc.c +6 -10
@@ -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 ; }
Signature · 1 line
2.34.1
PATCH v5 3/4 drm/prime: use dma_buf_fd_install() to preserve export tracing
In reply to Baineng Shoudrm_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/[email protected]/
--- drivers/gpu/drm/drm_prime.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/gpu/drm/drm_prime.c +1 -1
@@ -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 ; }
Signature · 1 line
2.34.1
[email protected]
AI reviewRe: [PATCH v5 3/4] drm/prime: use dma_buf_fd_install() to preserve export tracing
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] The file descriptor is published to userspace before the copy_to_user() operation succeeds, leading to a leaked fd and dma_buf reference if the copy fails. --
--- Patch [3]: [PATCH v5 3/4] drm/prime: use dma_buf_fd_install() to preserve export tracing --- commit 50a50a53aa7179af6880f9711b00fcaf6105ea33
Author: Baineng Shou <[email protected]>
drm/prime: use dma_buf_fd_install() to preserve export tracing
This commit replaces the open-coded fd reservation and installation in drm_gem_prime_handle_to_fd with dma_buf_fd_install. This restores the trace_dma_buf_fd tracepoint coverage that was bypassed by using fd_install directly.
Quoted 4 lines
Replace the bare
fd_install()withdma_buf_fd_install(), which wrapsfd_install()together withDMA_BUF_TRACE(), restoring full tracepoint coverage. No functional change; the fd lifecycle (get_unused_fd_flags→ work → install) is already correct.
[Severity: High]
This is a pre-existing issue, but is the statement about the fd lifecycle being already correct accurate?
In drm_gem_prime_handle_to_fd(), the file descriptor is published to the process's fd table before the ioctl data is copied back to user space:
drivers/gpu/drm/drm_prime.c:drm_gem_prime_handle_to_fd() {
...
dma_buf_fd_install(dmabuf, fd);
*prime_fd = fd;
return 0;
}The actual copy_to_user() happens later in the DRM ioctl wrapper:
drivers/gpu/drm/drm_ioctl.c:drm_ioctl() {
...
retcode = drm_ioctl_kernel(filp, func, kdata, ioctl->flags);
if (copy_to_user((void __user *)arg, kdata, out_size) != 0)
retcode = -EFAULT;
...
}If a user application allocates a page, maps it as read-only, constructs a valid drm_prime_handle payload within it, and invokes DRM_IOCTL_PRIME_HANDLE_TO_FD, the core drm_ioctl() successfully reads the input. The handler executes, allocates the dmabuf, and publishes the file descriptor.
When drm_ioctl() attempts to write the result back via copy_to_user(), it fails due to the read-only mapping, causing the ioctl to return -EFAULT. Since the fd is already published and the userspace application does not know the fd number to close it, does this result in a leak of the file descriptor and dmabuf references?
Quoted 13 lines
drivers/gpu/drm/drm_prime.c +1
-1@@ -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 ; }
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3
Re: [PATCH v5 3/4] drm/prime: use dma_buf_fd_install() to preserve export tracing
In reply to Baineng ShouQuoted 12 lines · Baineng Shou
drm_gem_prime_handle_to_fd()open-codes fd reservation and install usingget_unused_fd_flags()+fd_install()directly. This bypasses theDMA_BUF_TRACE()call thatdma_buf_fd()emits, so observability tools relying on thetrace_dma_buf_fdtracepoint silently miss all DRM PRIME exports.Replace the bare
fd_install()withdma_buf_fd_install(), which wrapsfd_install()together withDMA_BUF_TRACE(), restoring full tracepoint coverage. No functional change; the fd lifecycle (get_unused_fd_flags→ work → install) is already correct.
Quoted 8 lines
Note: this patch depends on
dma_buf_fd_install()introduced in "dma-buf: dma-heap: don't publish fd beforecopy_to_user()succeeds" [1].[1] https://lore.kernel.org/dri-devel/[email protected]/
Suggested-by Christian König <[email protected]>Signed-off-by Baineng Shou <[email protected]>
Quoted 20 lines
--- drivers/gpu/drm/drm_prime.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/gpu/drm/drm_prime.c +1
-2@@ -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
v6 cover dma-buf: fix fd leak when copy_to_user() fails after fd_install()
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.
Patch 3 replaces the bare fd_install() in drm_gem_prime_handle_to_fd() with dma_buf_fd_install() to restore tracepoint coverage for DRM PRIME exports (suggested by Christian König).
Patch 4 adds a selftest to tools/testing/selftests/dmabuf-heaps/ that reproduces the fd-leak scenario (mprotect flip before the ioctl) and verifies the fd count is unchanged after a failed ioctl (suggested by Sumit Semwal).
v1: https://lore.kernel.org/dri-devel/[email protected]/
v2: https://lore.kernel.org/dri-devel/[email protected]/
v3: https://lore.kernel.org/dri-devel/[email protected]/
Changes in v6:
- Rework the selftest (patch 4) per review: extract a count_open_fds() helper, fix the copy_from_user() comment, fail (not skip) when the ioctl does not return -1, drop the bogus mprotect-race mention, and reword the result message.
Changes in v5:
- Add selftest (patch 4) reproducing the fd-leak scenario (Sumit Semwal)
Changes in v4:
- Add patch 3: drm/prime: use dma_buf_fd_install() (Christian König)
- Add Acked-by: Christian König to patches 1 and 2
Changes in v3:
- Split into two patches (dma-heap + fastrpc separately)
- Add dma_buf_fd_install() to preserve trace_dma_buf_fd tracepoint
- Add fastrpc fix using the new helper (T.J. Mercier)
Baineng Shou (4):
dma-buf: dma-heap: don't publish fd before copy_to_user() succeeds
misc: fastrpc: don't publish fd before copy_to_user() succeeds drm/prime: use dma_buf_fd_install() to preserve export tracing
selftests: dmabuf-heaps: add fd-leak-on-EFAULT regression test
drivers/dma-buf/dma-buf.c | 20 ++++ drivers/dma-buf/dma-heap.c | 80 ++++++------- drivers/gpu/drm/drm_prime.c | 2 +- drivers/misc/fastrpc.c | 16 +-- include/linux/dma-buf.h | 1 + .../selftests/dmabuf-heaps/dmabuf-heap.c | 113 +++++++++++++++++- 6 files changed, 180 insertions(+), 52 deletions(-)
Signature · 1 line
2.34.1
v6 cover dma-buf: fix fd leak when copy_to_user() fails after fd_install()
In reply to T.J. MercierSeveral 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.
Patch 3 replaces the bare fd_install() in drm_gem_prime_handle_to_fd() with dma_buf_fd_install() to restore tracepoint coverage for DRM PRIME exports (suggested by Christian König).
Patch 4 adds a selftest to tools/testing/selftests/dmabuf-heaps/ that reproduces the fd-leak scenario (mprotect flip before the ioctl) and verifies the fd count is unchanged after a failed ioctl (suggested by Sumit Semwal).
v1: https://lore.kernel.org/dri-devel/[email protected]/
v2: https://lore.kernel.org/dri-devel/[email protected]/
v3: https://lore.kernel.org/dri-devel/[email protected]/
Changes in v6:
- Rework the selftest (patch 4) per review: extract a count_open_fds() helper, fix the copy_from_user() comment, fail (not skip) when the ioctl does not return -1, drop the bogus mprotect-race mention, and reword the result message.
Changes in v5:
- Add selftest (patch 4) reproducing the fd-leak scenario (Sumit Semwal)
Changes in v4:
- Add patch 3: drm/prime: use dma_buf_fd_install() (Christian König)
- Add Acked-by: Christian König to patches 1 and 2
Changes in v3:
- Split into two patches (dma-heap + fastrpc separately)
- Add dma_buf_fd_install() to preserve trace_dma_buf_fd tracepoint
- Add fastrpc fix using the new helper (T.J. Mercier)
Baineng Shou (4):
dma-buf: dma-heap: don't publish fd before copy_to_user() succeeds
misc: fastrpc: don't publish fd before copy_to_user() succeeds drm/prime: use dma_buf_fd_install() to preserve export tracing
selftests: dmabuf-heaps: add fd-leak-on-EFAULT regression test
drivers/dma-buf/dma-buf.c | 20 ++++ drivers/dma-buf/dma-heap.c | 80 ++++++------- drivers/gpu/drm/drm_prime.c | 2 +- drivers/misc/fastrpc.c | 16 +-- include/linux/dma-buf.h | 1 + .../selftests/dmabuf-heaps/dmabuf-heap.c | 113 +++++++++++++++++- 6 files changed, 180 insertions(+), 52 deletions(-)
Signature · 1 line
2.34.1
PATCH v6 1/4 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. 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/[email protected]/
--- 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(-)
drivers/dma-buf/dma-buf.c +20 -0
@@ -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
drivers/dma-buf/dma-heap.c +40 -40
124 lines not on this page · raw message
include/linux/dma-buf.h +1 -0
@@ -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);
Signature · 1 line
2.34.1
v6 cover dma-buf: fix fd leak when copy_to_user() fails after fd_install()
In reply to T.J. MercierSeveral 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.
Patch 3 replaces the bare fd_install() in drm_gem_prime_handle_to_fd() with dma_buf_fd_install() to restore tracepoint coverage for DRM PRIME exports (suggested by Christian König).
Patch 4 adds a selftest to tools/testing/selftests/dmabuf-heaps/ that reproduces the fd-leak scenario (mprotect flip before the ioctl) and verifies the fd count is unchanged after a failed ioctl (suggested by Sumit Semwal).
v1: https://lore.kernel.org/dri-devel/[email protected]/
v2: https://lore.kernel.org/dri-devel/[email protected]/
v3: https://lore.kernel.org/dri-devel/[email protected]/
Changes in v6:
- Rework the selftest (patch 4) per review: extract a count_open_fds() helper, fix the copy_from_user() comment, fail (not skip) when the ioctl does not return -1, drop the bogus mprotect-race mention, and reword the result message.
Changes in v5:
- Add selftest (patch 4) reproducing the fd-leak scenario (Sumit Semwal)
Changes in v4:
- Add patch 3: drm/prime: use dma_buf_fd_install() (Christian König)
- Add Acked-by: Christian König to patches 1 and 2
Changes in v3:
- Split into two patches (dma-heap + fastrpc separately)
- Add dma_buf_fd_install() to preserve trace_dma_buf_fd tracepoint
- Add fastrpc fix using the new helper (T.J. Mercier)
Baineng Shou (4):
dma-buf: dma-heap: don't publish fd before copy_to_user() succeeds
misc: fastrpc: don't publish fd before copy_to_user() succeeds drm/prime: use dma_buf_fd_install() to preserve export tracing
selftests: dmabuf-heaps: add fd-leak-on-EFAULT regression test
drivers/dma-buf/dma-buf.c | 20 ++++ drivers/dma-buf/dma-heap.c | 80 ++++++------- drivers/gpu/drm/drm_prime.c | 2 +- drivers/misc/fastrpc.c | 16 +-- include/linux/dma-buf.h | 1 + .../selftests/dmabuf-heaps/dmabuf-heap.c | 113 +++++++++++++++++- 6 files changed, 180 insertions(+), 52 deletions(-)
Signature · 1 line
2.34.1
v6 cover dma-buf: fix fd leak when copy_to_user() fails after fd_install()
In reply to T.J. MercierSeveral 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.
Patch 3 replaces the bare fd_install() in drm_gem_prime_handle_to_fd() with dma_buf_fd_install() to restore tracepoint coverage for DRM PRIME exports (suggested by Christian König).
Patch 4 adds a selftest to tools/testing/selftests/dmabuf-heaps/ that reproduces the fd-leak scenario (mprotect flip before the ioctl) and verifies the fd count is unchanged after a failed ioctl (suggested by Sumit Semwal).
v1: https://lore.kernel.org/dri-devel/[email protected]/
v2: https://lore.kernel.org/dri-devel/[email protected]/
v3: https://lore.kernel.org/dri-devel/[email protected]/
Changes in v6:
- Rework the selftest (patch 4) per review: extract a count_open_fds() helper, fix the copy_from_user() comment, fail (not skip) when the ioctl does not return -1, drop the bogus mprotect-race mention, and reword the result message.
Changes in v5:
- Add selftest (patch 4) reproducing the fd-leak scenario (Sumit Semwal)
Changes in v4:
- Add patch 3: drm/prime: use dma_buf_fd_install() (Christian König)
- Add Acked-by: Christian König to patches 1 and 2
Changes in v3:
- Split into two patches (dma-heap + fastrpc separately)
- Add dma_buf_fd_install() to preserve trace_dma_buf_fd tracepoint
- Add fastrpc fix using the new helper (T.J. Mercier)
Baineng Shou (4):
dma-buf: dma-heap: don't publish fd before copy_to_user() succeeds
misc: fastrpc: don't publish fd before copy_to_user() succeeds drm/prime: use dma_buf_fd_install() to preserve export tracing
selftests: dmabuf-heaps: add fd-leak-on-EFAULT regression test
drivers/dma-buf/dma-buf.c | 20 ++++ drivers/dma-buf/dma-heap.c | 80 ++++++------- drivers/gpu/drm/drm_prime.c | 2 +- drivers/misc/fastrpc.c | 16 +-- include/linux/dma-buf.h | 1 + .../selftests/dmabuf-heaps/dmabuf-heap.c | 113 +++++++++++++++++- 6 files changed, 180 insertions(+), 52 deletions(-)
Signature · 1 line
2.34.1
v6 cover dma-buf: fix fd leak when copy_to_user() fails after fd_install()
In reply to T.J. MercierSeveral 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.
Patch 3 replaces the bare fd_install() in drm_gem_prime_handle_to_fd() with dma_buf_fd_install() to restore tracepoint coverage for DRM PRIME exports (suggested by Christian König).
Patch 4 adds a selftest to tools/testing/selftests/dmabuf-heaps/ that reproduces the fd-leak scenario (mprotect flip before the ioctl) and verifies the fd count is unchanged after a failed ioctl (suggested by Sumit Semwal).
v1: https://lore.kernel.org/dri-devel/[email protected]/
v2: https://lore.kernel.org/dri-devel/[email protected]/
v3: https://lore.kernel.org/dri-devel/[email protected]/
Changes in v6:
- Rework the selftest (patch 4) per review: extract a count_open_fds() helper, fix the copy_from_user() comment, fail (not skip) when the ioctl does not return -1, drop the bogus mprotect-race mention, and reword the result message.
Changes in v5:
- Add selftest (patch 4) reproducing the fd-leak scenario (Sumit Semwal)
Changes in v4:
- Add patch 3: drm/prime: use dma_buf_fd_install() (Christian König)
- Add Acked-by: Christian König to patches 1 and 2
Changes in v3:
- Split into two patches (dma-heap + fastrpc separately)
- Add dma_buf_fd_install() to preserve trace_dma_buf_fd tracepoint
- Add fastrpc fix using the new helper (T.J. Mercier)
Baineng Shou (4):
dma-buf: dma-heap: don't publish fd before copy_to_user() succeeds
misc: fastrpc: don't publish fd before copy_to_user() succeeds drm/prime: use dma_buf_fd_install() to preserve export tracing
selftests: dmabuf-heaps: add fd-leak-on-EFAULT regression test
drivers/dma-buf/dma-buf.c | 20 ++++ drivers/dma-buf/dma-heap.c | 80 ++++++------- drivers/gpu/drm/drm_prime.c | 2 +- drivers/misc/fastrpc.c | 16 +-- include/linux/dma-buf.h | 1 + .../selftests/dmabuf-heaps/dmabuf-heap.c | 113 +++++++++++++++++- 6 files changed, 180 insertions(+), 52 deletions(-)
Signature · 1 line
2.34.1
PATCH v6 1/4 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. 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/[email protected]/
--- 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(-)
drivers/dma-buf/dma-buf.c +20 -0
@@ -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
drivers/dma-buf/dma-heap.c +40 -40
124 lines not on this page · raw message
include/linux/dma-buf.h +1 -0
@@ -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);
Signature · 1 line
2.34.1
PATCH v6 2/4 misc: fastrpc: don't publish fd before copy_to_user() succeeds
In reply to Baineng Shoufastrpc_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.
--- drivers/misc/fastrpc.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-)
drivers/misc/fastrpc.c +6 -10
@@ -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 ; }
Signature · 1 line
2.34.1
PATCH v6 3/4 drm/prime: use dma_buf_fd_install() to preserve export tracing
In reply to Baineng Shoudrm_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/[email protected]/
--- drivers/gpu/drm/drm_prime.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/gpu/drm/drm_prime.c +1 -1
@@ -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 ; }
Signature · 1 line
2.34.1
[email protected]
AI reviewRe: [PATCH v6 3/4] drm/prime: use dma_buf_fd_install() to preserve export tracing
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] Meaningful mismatch between the commit message's claim and the actual code behavior regarding…
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Meaningful mismatch between the commit message's claim and the actual code behavior regarding the file descriptor lifecycle. --
--- Patch [3]: [PATCH v6 3/4] drm/prime: use dma_buf_fd_install() to preserve export tracing --- commit 33826692d4a41b387d790330b8cd37a2f20fa524
Author: Baineng Shou <[email protected]>
drm/prime: use dma_buf_fd_install() to preserve export tracing
This commit replaces the open-coded fd reservation and installation in drm_gem_prime_handle_to_fd() with dma_buf_fd_install(). The change ensures that the DMA_BUF_TRACE() call is correctly emitted, restoring tracepoint coverage for DRM PRIME exports.
Quoted 4 lines
Replace the bare
fd_install()withdma_buf_fd_install(), which wrapsfd_install()together withDMA_BUF_TRACE(), restoring full tracepoint coverage. No functional change; the fd lifecycle (get_unused_fd_flags→ work → install) is already correct.
[Severity: Medium]
Is the statement about the fd lifecycle being already correct accurate?
This isn't a bug introduced by this patch, but rather a pre-existing issue. Looking at drm_gem_prime_handle_to_fd(), the fd is irrevocably published via dma_buf_fd_install() before returning:
drivers/gpu/drm/drm_prime.c:drm_gem_prime_handle_to_fd() {
...
dma_buf_fd_install(dmabuf, fd);
*prime_fd = fd;
return 0;
}After returning, the generic ioctl dispatcher performs a fallible copy_to_user():
drivers/gpu/drm/drm_ioctl.c:drm_ioctl() {
...
if (copy_to_user((void __user *)arg, kdata, out_size) != 0)
retcode = -EFAULT;
...
}If this copy_to_user() fails, does the fd remain permanently published, causing a leak of the fd and dmabuf reference?
If so, the code might actually need the same delayed publication fix that was applied to dma-heap, making the commit message's claim incorrect.
Quoted 13 lines
drivers/gpu/drm/drm_prime.c +1
-1@@ -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 ; }
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3
PATCH v6 4/4 selftests: dmabuf-heaps: add fd-leak-on-EFAULT regression test
In reply to Baineng ShouAdd a test case that verifies no file descriptor is leaked when DMA_HEAP_IOCTL_ALLOC succeeds internally but copy_to_user() fails to deliver the fd number back to userspace.
The failure is triggered by placing the ioctl argument in a private anonymous page and flipping it to PROT_READ (via mprotect) between the kernel's copy_from_user() and copy_to_user() calls. With the buggy kernel the ioctl returns -EFAULT but leaves an extra open fd in the process's fd table; with the fixed kernel the fd count is unchanged.
This serves as a regression test for:
"dma-buf: dma-heap: don't publish fd before copy_to_user() succeeds"
--- .../selftests/dmabuf-heaps/dmabuf-heap.c | 113 +++++++++++++++++- 1 file changed, 112 insertions(+), 1 deletion(-)
tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c +112 -1
@@ -390,6 +390,116 @@ static void test_alloc_errors(char *heap_name)close (heap_fd); } +/* +* count_open_fds - return the number of open file descriptors. +* +* The fd opened by opendir() itself is counted, but since it is opened +* and closed within each call, it cancels out when comparing two counts. +* Returns -1 on error. +*/ +static int count_open_fds (void ) +{ +DIR *d =opendir ("/proc/self/fd" ); +struct dirent *de; +int count =0 ; + +if (!d) +return -1 ; + +while ((de =readdir (d))) +if (de->d_name[0 ] !='.' ) + count++; +closedir (d); +return count; +} + +/* +* test_alloc_no_fd_leak_on_efault - verify no fd is leaked when +* copy_to_user() fails during DMA_HEAP_IOCTL_ALLOC. +* +* The bug: dma_buf_fd() called fd_install() before copy_to_user(). +* If copy_to_user() then failed (e.g. via mprotect), the fd was +* silently installed in the fd table but never returned to userspace. +* +* The fix: reserve the fd with get_unused_fd_flags() first, attempt +* copy_to_user(), and only call fd_install() on success. +* +* We trigger the failure by placing the ioctl argument in a private +* anonymous page and flipping it to PROT_READ before the ioctl. +* Inside the kernel, copy_from_user() reads from the page (reads are +* allowed under PROT_READ, so it succeeds), but copy_to_user() that +* writes the fd number back faults, returning -EFAULT. We then +* count open file descriptors before and after; with the bug an extra +* fd is left in the table. +*/ +static void test_alloc_no_fd_leak_on_efault (char *heap_name) +{ +int heap_fd =-1 ; +int fd_before, fd_after; +int ret; +long page_size; +struct dma_heap_allocation_data *req; + +ksft_print_msg ("Testing fd leak when copy_to_user() fails:\n" ); + + heap_fd =dmabuf_heap_open (heap_name); + + page_size =sysconf (_SC_PAGESIZE); + +/* +* Place the ioctl argument in its own private anonymous page so +* we can flip its protection independently. +*/ + req =mmap (NULL , page_size,PROT_READ |PROT_WRITE , +MAP_PRIVATE |MAP_ANONYMOUS ,-1 ,0 ); +if (req ==MAP_FAILED ) { +ksft_test_result_fail ("mmap failed: %s\n" ,strerror (errno)); + goto out; + } + +memset (req,0 ,sizeof (*req)); + req->len = page_size; + req->fd_flags =O_RDWR |O_CLOEXEC ; + + fd_before =count_open_fds (); +if (fd_before <0 ) { +ksft_test_result_fail ("count_open_fds: %s\n" ,strerror (errno)); +munmap (req, page_size); + goto out; + } + +/* +* Make the page read-only so copy_to_user() will fault. The +* ioctl must fail with -1; if it returns success the test setup +* is broken (mprotect is synchronous, so there is no race). +*/ +mprotect (req, page_size,PROT_READ ); + + ret =ioctl (heap_fd,DMA_HEAP_IOCTL_ALLOC , req); + +/* Re-allow writes so munmap can clean up */ +mprotect (req, page_size,PROT_READ |PROT_WRITE ); +munmap (req, page_size); + +if (ret !=-1 ) { +ksft_test_result_fail ("ioctl returned %d, expected -1 EFAULT\n" , + ret); + goto out; + } + + fd_after =count_open_fds (); +if (fd_after <0 ) { +ksft_test_result_fail ("count_open_fds: %s\n" ,strerror (errno)); + goto out; + } + +ksft_test_result (fd_before == fd_after, +"fd leak on EFAULT: before=%d after=%d\n" , + fd_before, fd_after); +out: +close (heap_fd); +} +static int numer_of_heaps (void ) {DIR *d =opendir (DEVPATH );
@@ -420,7 +530,7 @@ int main(void)return KSFT_SKIP ; } -ksft_set_plan (11 *numer_of_heaps ()); +ksft_set_plan (12 *numer_of_heaps ());while ((dir =readdir (d))) {if (!strncmp (dir->d_name,"." ,2 ))
@@ -435,6 +545,7 @@ int main(void)test_alloc_zeroed (dir->d_name,ONE_MEG );test_alloc_compat (dir->d_name);test_alloc_errors (dir->d_name); +test_alloc_no_fd_leak_on_efault (dir->d_name); }closedir (d);
Signature · 1 line
2.34.1
[email protected]
AI reviewRe: [PATCH v6 4/4] selftests: dmabuf-heaps: add fd-leak-on-EFAULT regression test
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] The regression test silently reports a false pass if the ioctl fails for a reason other than the…
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The regression test silently reports a false pass if the ioctl fails for a reason other than the expected -EFAULT.
- [Low] A file descriptor is leaked on the test's failure path if the ioctl unexpectedly succeeds.
--
--- Patch [4]: [PATCH v6 4/4] selftests: dmabuf-heaps: add fd-leak-on-EFAULT regression test --- commit 2ee174de4cce870677ff17aa1714b40304934392
Author: Baineng Shou <[email protected]>
selftests: dmabuf-heaps: add fd-leak-on-EFAULT regression test
This commit adds a test case that verifies no file descriptor is leaked when DMA_HEAP_IOCTL_ALLOC succeeds internally but copy_to_user() fails to deliver the fd number back to userspace. It triggers the failure by placing the ioctl argument in a private anonymous page and changing it to PROT_READ between the copy_from_user() and copy_to_user() calls.
Quoted 4 lines
tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c +0
-0
[ ... ]
tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c static void test_alloc_errors(char *heap_name) · line 483
Quoted 6 lines
tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c +6
-0+ ret =ioctl (heap_fd,DMA_HEAP_IOCTL_ALLOC , req); + +/* Re-allow writes so munmap can clean up */ +mprotect (req, page_size,PROT_READ |PROT_WRITE ); +munmap (req, page_size); +
[Severity: Low]
Does this code leak the newly allocated file descriptor if the ioctl unexpectedly succeeds?
Since the test unmaps the req structure immediately after the ioctl, the returned file descriptor stored in req->fd is lost. If the ioctl returned 0, the fd cannot be read and closed before the test jumps to the error path.
tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c static void test_alloc_errors(char *heap_name) · line 484
Quoted 1 line
tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c +1
-0+if (ret !=-1 ) {
[Severity: Medium]
Will this silently report a false pass if the ioctl fails for a reason other than the expected EFAULT?
The test checks that the return value is -1, but doesn't verify that errno == EFAULT. If the ioctl fails early (for example, returning -EINVAL or -ENOMEM) before reaching the copy_to_user() step, the fd count remains unchanged and the test passes, even though the EFAULT path was never tested.
Quoted 4 lines
tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c +4
-0+ksft_test_result_fail ("ioctl returned %d, expected -1 EFAULT\n" , + ret); + goto out; + }
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4
Re: [PATCH v6 4/4] selftests: dmabuf-heaps: add fd-leak-on-EFAULT regression test
In reply to Baineng ShouQuoted 12 lines · Baineng Shou
Add a test case that verifies no file descriptor is leaked when
DMA_HEAP_IOCTL_ALLOCsucceeds internally butcopy_to_user()fails to deliver the fd number back to userspace.The failure is triggered by placing the ioctl argument in a private anonymous page and flipping it to
PROT_READ(via mprotect) between the kernel'scopy_from_user()andcopy_to_user()calls. With the buggy kernel the ioctl returns-EFAULTbut leaves an extra open fd in the process's fd table; with the fixed kernel the fd count is unchanged.
Quoted 5 lines
This serves as a regression test for:
"dma-buf: dma-heap: don't publish fd beforecopy_to_user()succeeds"Suggested-by Sumit Semwal <[email protected]>Signed-off-by Baineng Shou <[email protected]>
Re: [PATCH v6 0/4] dma-buf: fix fd leak when copy_to_user() fails after fd_install()
In reply to Baineng ShouHello Baineng,
Quoted 23 lines · Baineng Shou
Several drivers call
dma_buf_fd()— which internally callsfd_install()— beforecopy_to_user()returns the fd number to userspace. Ifcopy_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 betweenfd_install()andcopy_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), docopy_to_user(), and only then publish the fd withfd_install()via the newdma_buf_fd_install()helper. Oncopy_to_user()failure,put_unused_fd()+dma_buf_put()cleanly unwind with no user-visible side effects.Patch 1 introduces
dma_buf_fd_install()indma-buf.c(wrappingfd_install()together with theDMA_BUF_TRACEcall 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.
Quoted 8 lines
Patch 3 replaces the bare
fd_install()indrm_gem_prime_handle_to_fd()withdma_buf_fd_install()to restore tracepoint coverage for DRM PRIME exports (suggested by Christian König).Patch 4 adds a selftest to
tools/testing/selftests/dmabuf-heaps/that reproduces the fd-leak scenario (mprotect flip before the ioctl) and verifies the fd count is unchanged after a failed ioctl (suggested by Sumit Semwal).
Thank you for the series - please feel free to add my
Quoted 37 lines
v1: https://lore.kernel.org/dri-devel/[email protected]/
v2: https://lore.kernel.org/dri-devel/[email protected]/
v3: https://lore.kernel.org/dri-devel/[email protected]/Changes in v6:
- Rework the selftest (patch 4) per review: extract acount_open_fds()helper, fix thecopy_from_user()comment, fail (not skip) when the ioctl does not return -1, drop the bogus mprotect-race mention, and reword the result message.Changes in v5:
- Add selftest (patch 4) reproducing the fd-leak scenario (Sumit Semwal)Changes in v4:
- Add patch 3: drm/prime: usedma_buf_fd_install()(Christian König)
- Add Acked-by: Christian König to patches 1 and 2Changes in v3:
- Split into two patches (dma-heap + fastrpc separately)
- Adddma_buf_fd_install()to preservetrace_dma_buf_fdtracepoint
- Add fastrpc fix using the new helper (T.J. Mercier)Baineng Shou (4):
dma-buf: dma-heap: don't publish fd beforecopy_to_user()succeeds
misc: fastrpc: don't publish fd beforecopy_to_user()succeeds drm/prime: usedma_buf_fd_install()to preserve export tracing
selftests: dmabuf-heaps: add fd-leak-on-EFAULT regression testdrivers/dma-buf/dma-buf.c | 20 ++++ drivers/dma-buf/dma-heap.c | 80 ++++++------- drivers/gpu/drm/drm_prime.c | 2 +- drivers/misc/fastrpc.c | 16 +-- include/linux/dma-buf.h | 1 + .../selftests/dmabuf-heaps/dmabuf-heap.c | 113 +++++++++++++++++- 6 files changed, 180 insertions(+), 52 deletions(-)
Quoted 3 lines
--
2.34.1
Best,
Sumit.
v7 cover dma-buf: fix fd leak when copy_to_user() fails after fd_install()
In reply to T.J. MercierSeveral 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.
Patch 3 replaces the bare fd_install() in drm_gem_prime_handle_to_fd() with dma_buf_fd_install() to restore tracepoint coverage for DRM PRIME exports (suggested by Christian König).
Patch 4 adds a selftest to tools/testing/selftests/dmabuf-heaps/ that reproduces the fd-leak scenario (mprotect flip before the ioctl) and verifies the fd count is unchanged after a failed ioctl (suggested by Sumit Semwal).
v1: https://lore.kernel.org/dri-devel/[email protected]/
v2: https://lore.kernel.org/dri-devel/[email protected]/
v3: https://lore.kernel.org/dri-devel/[email protected]/
v5: https://lore.kernel.org/dri-devel/[email protected]/
v6: https://lore.kernel.org/dri-devel/[email protected]/
Changes in v7:
- Add Reviewed-by: T.J. Mercier to patch 4 (selftest).
- Add Acked-by: Sumit Semwal to the whole series.
Changes in v6:
- Rework the selftest (patch 4) per review: extract a count_open_fds() helper, fix the copy_from_user() comment, fail (not skip) when the ioctl does not return -1, drop the bogus mprotect-race mention, and reword the result message.
Changes in v5:
- Add selftest (patch 4) reproducing the fd-leak scenario (Sumit Semwal)
Changes in v4:
- Add patch 3: drm/prime: use dma_buf_fd_install() (Christian König)
- Add Acked-by: Christian König to patches 1 and 2
Changes in v3:
- Split into two patches (dma-heap + fastrpc separately)
- Add dma_buf_fd_install() to preserve trace_dma_buf_fd tracepoint
- Add fastrpc fix using the new helper (T.J. Mercier)
Baineng Shou (4):
dma-buf: dma-heap: don't publish fd before copy_to_user() succeeds
misc: fastrpc: don't publish fd before copy_to_user() succeeds drm/prime: use dma_buf_fd_install() to preserve export tracing
selftests: dmabuf-heaps: add fd-leak-on-EFAULT regression test
drivers/dma-buf/dma-buf.c | 20 ++++ drivers/dma-buf/dma-heap.c | 80 ++++++------- drivers/gpu/drm/drm_prime.c | 2 +- drivers/misc/fastrpc.c | 16 +-- include/linux/dma-buf.h | 1 + .../selftests/dmabuf-heaps/dmabuf-heap.c | 113 +++++++++++++++++- 6 files changed, 180 insertions(+), 52 deletions(-)
Signature · 1 line
2.34.1
PATCH v7 1/4 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. 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/[email protected]/
--- 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(-)
drivers/dma-buf/dma-buf.c +20 -0
@@ -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
drivers/dma-buf/dma-heap.c +40 -40
124 lines not on this page · raw message
include/linux/dma-buf.h +1 -0
@@ -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);
Signature · 1 line
2.34.1
PATCH v7 2/4 misc: fastrpc: don't publish fd before copy_to_user() succeeds
In reply to Baineng Shoufastrpc_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.
--- drivers/misc/fastrpc.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-)
drivers/misc/fastrpc.c +6 -10
@@ -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 ; }
Signature · 1 line
2.34.1
[email protected]
AI reviewRe: [PATCH v7 2/4] misc: fastrpc: don't publish fd before copy_to_user() succeeds
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [Critical] Exported DMA buffers maintain a raw pointer to fastrpc_user (buf->fl) without holding a reference, leading to a Use-After-Free if the DMA buffer outlives the fastrpc device descriptor. --
--- Patch [2]: [PATCH v7 2/4] misc: fastrpc: don't publish fd before copy_to_user() succeeds --- commit f5005f3a9100abf2f83ab45f3bc5e948d9e97e67
Author: Baineng Shou <[email protected]>
misc: fastrpc: don't publish fd before copy_to_user() succeeds
This commit fixes an issue where a DMA buffer file descriptor is installed into the fd table before successfully copying the fd number back to userspace. It updates the allocation path to reserve the fd first, attempt the user copy, and only publish the fd upon success to prevent leaking access to the fd on failure.
Quoted 20 lines
drivers/misc/fastrpc.c +2
-6@@ -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
drivers/misc/fastrpc.c static int fastrpc_dmabuf_alloc(struct fastrpc_user *fl, char __user *argp) · line 1724
Quoted 10 lines
drivers/misc/fastrpc.c +3
-4-* 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);
[Severity: Critical]
This is a pre-existing issue, but does exporting this dma-buf introduce a use-after-free risk if the dma-buf fd outlives the fastrpc device fd?
When the fastrpc device fd is closed, the fastrpc_user struct (fl) is released. However, the exported dma-buf maintains a raw pointer to it via buf->fl without holding a reference.
If an unprivileged user allocates a dma-buf here, closes the fastrpc device fd, and then calls close() or mmap() on the dma-buf fd, it triggers dma_buf_put() or mmap(). Those paths call into fastrpc_buf_free() or fastrpc_mmap(), which dereference the dangling buf->fl pointer:
fastrpc_buf_free()
fastrpc_ipa_to_dma_addr(buf->fl->cctx, buf->dma_addr)
Is there a missing reference count increment on fastrpc_user when exporting the buffer, or should the dma-buf release operations handle the device teardown differently?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
PATCH v7 3/4 drm/prime: use dma_buf_fd_install() to preserve export tracing
In reply to Baineng Shoudrm_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/[email protected]/
--- drivers/gpu/drm/drm_prime.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/gpu/drm/drm_prime.c +1 -1
@@ -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 ; }
Signature · 1 line
2.34.1
[email protected]
AI reviewRe: [PATCH v7 3/4] drm/prime: use dma_buf_fd_install() to preserve export tracing
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] The file descriptor is published to userspace before copy_to_user() succeeds, violating the dma_buf_fd_install() API contract and causing a resource leak on error. --
--- Patch [3]: [PATCH v7 3/4] drm/prime: use dma_buf_fd_install() to preserve export tracing --- commit edfa3fdf8153dcc15c693c10359892f042189a1c
Author: Baineng Shou <[email protected]>
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 restore tracepoint coverage for DRM PRIME exports. It asserts that the file descriptor lifecycle is already correct.
Quoted 2 lines
No functional change; the fd lifecycle (
get_unused_fd_flags-> work -> install) is already correct.
[Severity: High]
This isn't a bug introduced by this patch, but is this claim about the fd lifecycle being correct accurate?
The new dma_buf_fd_install() API requires that it must only be called after all fallible work (such as copy_to_user) has succeeded.
Quoted 4 lines
drivers/gpu/drm/drm_prime.c +0
-0
[ ... ]
drivers/gpu/drm/drm_prime.c int drm_gem_prime_handle_to_fd(struct drm_device *dev, · line 527
Quoted 6 lines
drivers/gpu/drm/drm_prime.c +1
-1@@ -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]
This is a pre-existing issue, but by adopting the new dma_buf_fd_install() API, doesn't this code path violate its documented contract?
Can this lead to a file descriptor leak if a userspace application calls the DRM_IOCTL_PRIME_HANDLE_TO_FD ioctl with an invalid userspace pointer?
If drm_gem_prime_handle_to_fd() publishes the file descriptor to the userspace fdtable here:
dma_buf_fd_install(dmabuf, fd);*prime_fd = fd;
return 0;
Then the DRM core ioctl dispatcher attempts to copy the result to userspace after the handler returns, which can fail:
drivers/gpu/drm/drm_ioctl.c:drm_ioctl() {
...
retcode = func(dev, kdata, file_priv);
...
if (copy_to_user((void __user *)arg, kdata, out_size) != 0)
retcode = -EFAULT;
...
}Wouldn't the failed copy_to_user() return -EFAULT and leave the already installed fd orphaned in the process's fdtable, without the application ever receiving the fd number to close it?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3
PATCH v7 4/4 selftests: dmabuf-heaps: add fd-leak-on-EFAULT regression test
In reply to Baineng ShouAdd a test case that verifies no file descriptor is leaked when DMA_HEAP_IOCTL_ALLOC succeeds internally but copy_to_user() fails to deliver the fd number back to userspace.
The failure is triggered by placing the ioctl argument in a private anonymous page and flipping it to PROT_READ (via mprotect) between the kernel's copy_from_user() and copy_to_user() calls. With the buggy kernel the ioctl returns -EFAULT but leaves an extra open fd in the process's fd table; with the fixed kernel the fd count is unchanged.
This serves as a regression test for:
"dma-buf: dma-heap: don't publish fd before copy_to_user() succeeds"
--- .../selftests/dmabuf-heaps/dmabuf-heap.c | 113 +++++++++++++++++- 1 file changed, 112 insertions(+), 1 deletion(-)
tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c +112 -1
@@ -390,6 +390,116 @@ static void test_alloc_errors(char *heap_name)close (heap_fd); } +/* +* count_open_fds - return the number of open file descriptors. +* +* The fd opened by opendir() itself is counted, but since it is opened +* and closed within each call, it cancels out when comparing two counts. +* Returns -1 on error. +*/ +static int count_open_fds (void ) +{ +DIR *d =opendir ("/proc/self/fd" ); +struct dirent *de; +int count =0 ; + +if (!d) +return -1 ; + +while ((de =readdir (d))) +if (de->d_name[0 ] !='.' ) + count++; +closedir (d); +return count; +} + +/* +* test_alloc_no_fd_leak_on_efault - verify no fd is leaked when +* copy_to_user() fails during DMA_HEAP_IOCTL_ALLOC. +* +* The bug: dma_buf_fd() called fd_install() before copy_to_user(). +* If copy_to_user() then failed (e.g. via mprotect), the fd was +* silently installed in the fd table but never returned to userspace. +* +* The fix: reserve the fd with get_unused_fd_flags() first, attempt +* copy_to_user(), and only call fd_install() on success. +* +* We trigger the failure by placing the ioctl argument in a private +* anonymous page and flipping it to PROT_READ before the ioctl. +* Inside the kernel, copy_from_user() reads from the page (reads are +* allowed under PROT_READ, so it succeeds), but copy_to_user() that +* writes the fd number back faults, returning -EFAULT. We then +* count open file descriptors before and after; with the bug an extra +* fd is left in the table. +*/ +static void test_alloc_no_fd_leak_on_efault (char *heap_name) +{ +int heap_fd =-1 ; +int fd_before, fd_after; +int ret; +long page_size; +struct dma_heap_allocation_data *req; + +ksft_print_msg ("Testing fd leak when copy_to_user() fails:\n" ); + + heap_fd =dmabuf_heap_open (heap_name); + + page_size =sysconf (_SC_PAGESIZE); + +/* +* Place the ioctl argument in its own private anonymous page so +* we can flip its protection independently. +*/ + req =mmap (NULL , page_size,PROT_READ |PROT_WRITE , +MAP_PRIVATE |MAP_ANONYMOUS ,-1 ,0 ); +if (req ==MAP_FAILED ) { +ksft_test_result_fail ("mmap failed: %s\n" ,strerror (errno)); + goto out; + } + +memset (req,0 ,sizeof (*req)); + req->len = page_size; + req->fd_flags =O_RDWR |O_CLOEXEC ; + + fd_before =count_open_fds (); +if (fd_before <0 ) { +ksft_test_result_fail ("count_open_fds: %s\n" ,strerror (errno)); +munmap (req, page_size); + goto out; + } + +/* +* Make the page read-only so copy_to_user() will fault. The +* ioctl must fail with -1; if it returns success the test setup +* is broken (mprotect is synchronous, so there is no race). +*/ +mprotect (req, page_size,PROT_READ ); + + ret =ioctl (heap_fd,DMA_HEAP_IOCTL_ALLOC , req); + +/* Re-allow writes so munmap can clean up */ +mprotect (req, page_size,PROT_READ |PROT_WRITE ); +munmap (req, page_size); + +if (ret !=-1 ) { +ksft_test_result_fail ("ioctl returned %d, expected -1 EFAULT\n" , + ret); + goto out; + } + + fd_after =count_open_fds (); +if (fd_after <0 ) { +ksft_test_result_fail ("count_open_fds: %s\n" ,strerror (errno)); + goto out; + } + +ksft_test_result (fd_before == fd_after, +"fd leak on EFAULT: before=%d after=%d\n" , + fd_before, fd_after); +out: +close (heap_fd); +} +static int numer_of_heaps (void ) {DIR *d =opendir (DEVPATH );
@@ -420,7 +530,7 @@ int main(void)return KSFT_SKIP ; } -ksft_set_plan (11 *numer_of_heaps ()); +ksft_set_plan (12 *numer_of_heaps ());while ((dir =readdir (d))) {if (!strncmp (dir->d_name,"." ,2 ))
@@ -435,6 +545,7 @@ int main(void)test_alloc_zeroed (dir->d_name,ONE_MEG );test_alloc_compat (dir->d_name);test_alloc_errors (dir->d_name); +test_alloc_no_fd_leak_on_efault (dir->d_name); }closedir (d);
Signature · 1 line
2.34.1
[email protected]
AI reviewRe: [PATCH v7 4/4] selftests: dmabuf-heaps: add fd-leak-on-EFAULT regression test
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [Medium] The test does not verify that `errno == EFAULT` when `ioctl` fails, which can lead to false…
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] The test does not verify that errno == EFAULT when ioctl fails, which can lead to false positives if the allocation fails for other reasons.
- [Low] The return value of the first mprotect call is ignored.
- [Low] An unnecessary mprotect call is made to restore PROT_WRITE before munmap, accompanied by an inaccurate comment.
- [Low] If the ioctl unexpectedly succeeds, the newly allocated file descriptor is leaked.
--
--- Patch [4]: [PATCH v7 4/4] selftests: dmabuf-heaps: add fd-leak-on-EFAULT regression test --- commit 1be53f13852770ff6e3074adac5bd4f146ea6110
Author: Baineng Shou <[email protected]>
selftests: dmabuf-heaps: add fd-leak-on-EFAULT regression test
Add a test case that verifies no file descriptor is leaked when DMA_HEAP_IOCTL_ALLOC succeeds internally but copy_to_user() fails to deliver the fd number back to userspace.
Quoted 4 lines
tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c +0
-0
[ ... ]
Quoted 1 line
tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c +0
-0@@ -390,6 +390,116 @@ static void test_alloc_errors(char *heap_name)
[ ... ]
Quoted 2 lines
tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c +2
-0+static void test_alloc_no_fd_leak_on_efault (char *heap_name) +{
[ ... ]
tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c static void test_alloc_errors(char *heap_name) · line 476
Quoted 6 lines
tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c +6
-0+/* +* Make the page read-only so copy_to_user() will fault. The +* ioctl must fail with -1; if it returns success the test setup +* is broken (mprotect is synchronous, so there is no race). +*/ +mprotect (req, page_size,PROT_READ );
[Severity: Low]
Does this code handle the case where mprotect() fails? If it fails, the memory would remain writable, the ioctl would succeed, and the test would fail confusingly instead of reporting a setup failure.
tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c static void test_alloc_errors(char *heap_name) · line 481
Quoted 5 lines
tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c +5
-0+ + ret =ioctl (heap_fd,DMA_HEAP_IOCTL_ALLOC , req); + +/* Re-allow writes so munmap can clean up */ +mprotect (req, page_size,PROT_READ |PROT_WRITE );
[Severity: Low]
Is this mprotect() call necessary? munmap() manipulates the kernel's memory map directly and doesn't require the underlying memory pages to have write permissions to be unmapped.
tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c static void test_alloc_errors(char *heap_name) · line 484
Quoted 3 lines
tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c +3
-0+munmap (req, page_size); + +if (ret !=-1 ) {
[Severity: Low]
If the ioctl unexpectedly succeeds (for example if mprotect failed), does this code leak the newly allocated file descriptor? By the time success is detected, req has been unmapped, making it impossible to read req->fd to close it.
[Severity: Medium]
Does this code verify that the failure was actually caused by the expected EFAULT? If the ioctl fails for unrelated reasons like -ENOMEM, ret will still be -1, fd_before will equal fd_after, and the test will erroneously report PASS even though the copy_to_user() fault path was never tested.
Quoted 4 lines
tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c +4
-0+ksft_test_result_fail ("ioctl returned %d, expected -1 EFAULT\n" , + ret); + goto out; + }
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4
Re: [PATCH v7 0/4] dma-buf: fix fd leak when copy_to_user() fails after fd_install()
In reply to Baineng ShouI've gone ahead, added one more rb and pushed the result to drm-misc-fixes.
Thanks for the help,
Christian.
Quoted 73 lines · Baineng Shou
Several drivers call
dma_buf_fd()— which internally callsfd_install()— beforecopy_to_user()returns the fd number to userspace. Ifcopy_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 betweenfd_install()andcopy_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), docopy_to_user(), and only then publish the fd withfd_install()via the newdma_buf_fd_install()helper. Oncopy_to_user()failure,put_unused_fd()+dma_buf_put()cleanly unwind with no user-visible side effects.Patch 1 introduces
dma_buf_fd_install()indma-buf.c(wrappingfd_install()together with theDMA_BUF_TRACEcall 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.
Patch 3 replaces the bare
fd_install()indrm_gem_prime_handle_to_fd()withdma_buf_fd_install()to restore tracepoint coverage for DRM PRIME exports (suggested by Christian König).Patch 4 adds a selftest to
tools/testing/selftests/dmabuf-heaps/that reproduces the fd-leak scenario (mprotect flip before the ioctl) and verifies the fd count is unchanged after a failed ioctl (suggested by Sumit Semwal).v1: https://lore.kernel.org/dri-devel/[email protected]/
v2: https://lore.kernel.org/dri-devel/[email protected]/
v3: https://lore.kernel.org/dri-devel/[email protected]/
v5: https://lore.kernel.org/dri-devel/[email protected]/
v6: https://lore.kernel.org/dri-devel/[email protected]/Changes in v7:
- Add Reviewed-by: T.J. Mercier to patch 4 (selftest).
- Add Acked-by: Sumit Semwal to the whole series.Changes in v6:
- Rework the selftest (patch 4) per review: extract acount_open_fds()helper, fix thecopy_from_user()comment, fail (not skip) when the ioctl does not return -1, drop the bogus mprotect-race mention, and reword the result message.Changes in v5:
- Add selftest (patch 4) reproducing the fd-leak scenario (Sumit Semwal)Changes in v4:
- Add patch 3: drm/prime: usedma_buf_fd_install()(Christian König)
- Add Acked-by: Christian König to patches 1 and 2Changes in v3:
- Split into two patches (dma-heap + fastrpc separately)
- Adddma_buf_fd_install()to preservetrace_dma_buf_fdtracepoint
- Add fastrpc fix using the new helper (T.J. Mercier)Baineng Shou (4):
dma-buf: dma-heap: don't publish fd beforecopy_to_user()succeeds
misc: fastrpc: don't publish fd beforecopy_to_user()succeeds drm/prime: usedma_buf_fd_install()to preserve export tracing
selftests: dmabuf-heaps: add fd-leak-on-EFAULT regression testdrivers/dma-buf/dma-buf.c | 20 ++++ drivers/dma-buf/dma-heap.c | 80 ++++++------- drivers/gpu/drm/drm_prime.c | 2 +- drivers/misc/fastrpc.c | 16 +-- include/linux/dma-buf.h | 1 + .../selftests/dmabuf-heaps/dmabuf-heap.c | 113 +++++++++++++++++- 6 files changed, 180 insertions(+), 52 deletions(-)
PATCH v5 4/4 selftests: dmabuf-heaps: add fd-leak-on-EFAULT regression test
In reply to Baineng ShouAdd a test case that verifies no file descriptor is leaked when DMA_HEAP_IOCTL_ALLOC succeeds internally but copy_to_user() fails to deliver the fd number back to userspace.
The failure is triggered by placing the ioctl argument in a private anonymous page and flipping it to PROT_READ (via mprotect) between the kernel's copy_from_user() and copy_to_user() calls. With the buggy kernel the ioctl returns -EFAULT but leaves an extra open fd in the process's fd table; with the fixed kernel the fd count is unchanged.
This serves as a regression test for:
"dma-buf: dma-heap: don't publish fd before copy_to_user() succeeds"
--- .../selftests/dmabuf-heaps/dmabuf-heap.c | 115 +++++++++++++++++- 1 file changed, 114 insertions(+), 1 deletion(-)
tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c +114 -1
@@ -390,6 +390,118 @@ static void test_alloc_errors(char *heap_name)close (heap_fd); } +/* +* test_alloc_no_fd_leak_on_efault - verify no fd is leaked when +* copy_to_user() fails during DMA_HEAP_IOCTL_ALLOC. +* +* The bug: dma_buf_fd() called fd_install() before copy_to_user(). +* If copy_to_user() then failed (e.g. via mprotect), the fd was +* silently installed in the fd table but never returned to userspace. +* +* The fix: reserve the fd with get_unused_fd_flags() first, attempt +* copy_to_user(), and only call fd_install() on success. +* +* We trigger the failure by placing the ioctl argument in a page, +* flipping it to PROT_READ between copy_from_user and copy_to_user, +* and counting open file descriptors before and after. +*/ +static void test_alloc_no_fd_leak_on_efault (char *heap_name) +{ +int heap_fd =-1 ; +int fd_before, fd_after; +int ret; +long page_size; +struct dma_heap_allocation_data *req; + +ksft_print_msg ("Testing no fd leak when copy_to_user() fails:\n" ); + + heap_fd =dmabuf_heap_open (heap_name); + + page_size =sysconf (_SC_PAGESIZE); + +/* +* Place the ioctl argument in its own private anonymous page so +* we can flip its protection independently. +*/ + req =mmap (NULL , page_size,PROT_READ |PROT_WRITE , +MAP_PRIVATE |MAP_ANONYMOUS ,-1 ,0 ); +if (req ==MAP_FAILED ) { +ksft_test_result_fail ("mmap failed: %s\n" ,strerror (errno)); + goto out; + } + +memset (req,0 ,sizeof (*req)); + req->len = page_size; + req->fd_flags =O_RDWR |O_CLOEXEC ; + +/* Count open fds before the ioctl */ + fd_before =0 ; + { +DIR *d =opendir ("/proc/self/fd" ); +struct dirent *de; + +if (!d) { +ksft_test_result_fail ("opendir /proc/self/fd: %s\n" , +strerror (errno)); +munmap (req, page_size); + goto out; + } +while ((de =readdir (d))) +if (de->d_name[0 ] !='.' ) + fd_before++; +closedir (d); +/* subtract the fd opened by opendir itself */ + } + +/* +* Make the page read-only: copy_from_user() in the kernel will +* still succeed (it already ran), but copy_to_user() that writes +* the fd number back will fault. +*/ +mprotect (req, page_size,PROT_READ ); + + ret =ioctl (heap_fd,DMA_HEAP_IOCTL_ALLOC , req); + +/* Re-allow writes so munmap can clean up */ +mprotect (req, page_size,PROT_READ |PROT_WRITE ); +munmap (req, page_size); + +if (ret !=-1 || errno !=EFAULT ) { +/* +* If the ioctl didn't fail with EFAULT, either the kernel +* handled it differently or mprotect raced. Skip rather +* than giving a false pass/fail. +*/ +ksft_test_result_skip ( +"ioctl did not return EFAULT (ret=%d errno=%d), skipping\n" , + ret, errno); + goto out; + } + +/* Count open fds after the failed ioctl */ + fd_after =0 ; + { +DIR *d =opendir ("/proc/self/fd" ); +struct dirent *de; + +if (!d) { +ksft_test_result_fail ("opendir /proc/self/fd: %s\n" , +strerror (errno)); + goto out; + } +while ((de =readdir (d))) +if (de->d_name[0 ] !='.' ) + fd_after++; +closedir (d); + } + +ksft_test_result (fd_before == fd_after, +"no fd leak on EFAULT: before=%d after=%d\n" , + fd_before, fd_after); +out: +close (heap_fd); +} +static int numer_of_heaps (void ) {DIR *d =opendir (DEVPATH );
@@ -420,7 +532,7 @@ int main(void)return KSFT_SKIP ; } -ksft_set_plan (11 *numer_of_heaps ()); +ksft_set_plan (12 *numer_of_heaps ());while ((dir =readdir (d))) {if (!strncmp (dir->d_name,"." ,2 ))
@@ -435,6 +547,7 @@ int main(void)test_alloc_zeroed (dir->d_name,ONE_MEG );test_alloc_compat (dir->d_name);test_alloc_errors (dir->d_name); +test_alloc_no_fd_leak_on_efault (dir->d_name); }closedir (d);
Signature · 1 line
2.34.1
[email protected]
AI reviewRe: [PATCH v5 4/4] selftests: dmabuf-heaps: add fd-leak-on-EFAULT regression test
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] Missing `errno` preservation leads to potential clobbering before evaluation. - [Medium]…
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Missing errno preservation leads to potential clobbering before evaluation.
- [Medium] Unconditional unmapping of req causes a file descriptor leak if ioctl unexpectedly succeeds. --
--- Patch [4]: [PATCH v5 4/4] selftests: dmabuf-heaps: add fd-leak-on-EFAULT regression test --- commit 43e605bb1fee2426e027813c0311f1910f7ea15e
Author: Baineng Shou <[email protected]>
selftests: dmabuf-heaps: add fd-leak-on-EFAULT regression test
Add a test case that verifies no file descriptor is leaked when DMA_HEAP_IOCTL_ALLOC succeeds internally but copy_to_user() fails to deliver the fd number back to userspace.
Quoted 4 lines
tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c +0
-0
[ ... ]
Quoted 2 lines
tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c +2
-0+static void test_alloc_no_fd_leak_on_efault (char *heap_name) +{
[ ... ]
Quoted 4 lines
tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c +4
-0+/* +* Make the page read-only: copy_from_user() in the kernel will +* still succeed (it already ran), but copy_to_user() that writes +* the fd number back will fault.
tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c static void test_alloc_errors(char *heap_name) · line 469
Quoted 10 lines
tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c +10
-0+*/ +mprotect (req, page_size,PROT_READ ); + + ret =ioctl (heap_fd,DMA_HEAP_IOCTL_ALLOC , req); + +/* Re-allow writes so munmap can clean up */ +mprotect (req, page_size,PROT_READ |PROT_WRITE ); +munmap (req, page_size); + +if (ret !=-1 || errno !=EFAULT ) {
[Severity: Medium]
Will this evaluate the correct errno from the ioctl call?
The errno variable is checked after executing mprotect and munmap. If either of these functions modify errno (for example, if they fail), the test will evaluate the wrong errno and incorrectly skip with a misleading message instead of reporting a failure or success properly.
[Severity: Medium]
Also, could this leak the newly allocated file descriptor in the test process if the ioctl call unexpectedly succeeds?
If the ioctl call successfully allocates a file descriptor (for instance, if the initial mprotect fails to make the page read-only or due to a kernel bug), the new file descriptor is written to req->fd.
Since the test unconditionally calls munmap(req) immediately after, it destroys the only reference to the allocated file descriptor. The test then detects unexpected success, skips, and leaks the file descriptor in the test process.
Quoted 10 lines
tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c +10
-0+/* +* If the ioctl didn't fail with EFAULT, either the kernel +* handled it differently or mprotect raced. Skip rather +* than giving a false pass/fail. +*/ +ksft_test_result_skip ( +"ioctl did not return EFAULT (ret=%d errno=%d), skipping\n" , + ret, errno); + goto out; + }
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4
T.J. Mercier
reviewerRe: [PATCH v5 4/4] selftests: dmabuf-heaps: add fd-leak-on-EFAULT regression test
In reply to Baineng ShouQuoted 80 lines · Baineng Shou
Add a test case that verifies no file descriptor is leaked when
DMA_HEAP_IOCTL_ALLOCsucceeds internally butcopy_to_user()fails to deliver the fd number back to userspace.The failure is triggered by placing the ioctl argument in a private anonymous page and flipping it to
PROT_READ(via mprotect) between the kernel'scopy_from_user()andcopy_to_user()calls. With the buggy kernel the ioctl returns-EFAULTbut leaves an extra open fd in the process's fd table; with the fixed kernel the fd count is unchanged.This serves as a regression test for:
"dma-buf: dma-heap: don't publish fd beforecopy_to_user()succeeds"Suggested-by Sumit Semwal <[email protected]>Signed-off-by Baineng Shou <[email protected]>--- .../selftests/dmabuf-heaps/dmabuf-heap.c | 115 +++++++++++++++++- 1 file changed, 114 insertions(+), 1 deletion(-)tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c +51
-0@@ -390,6 +390,118 @@ static void test_alloc_errors(char *heap_name)close (heap_fd); } +/* +* test_alloc_no_fd_leak_on_efault - verify no fd is leaked when +* copy_to_user() fails during DMA_HEAP_IOCTL_ALLOC. +* +* The bug: dma_buf_fd() called fd_install() before copy_to_user(). +* If copy_to_user() then failed (e.g. via mprotect), the fd was +* silently installed in the fd table but never returned to userspace. +* +* The fix: reserve the fd with get_unused_fd_flags() first, attempt +* copy_to_user(), and only call fd_install() on success. +* +* We trigger the failure by placing the ioctl argument in a page, +* flipping it to PROT_READ between copy_from_user and copy_to_user, +* and counting open file descriptors before and after. +*/ +static void test_alloc_no_fd_leak_on_efault (char *heap_name) +{ +int heap_fd =-1 ; +int fd_before, fd_after; +int ret; +long page_size; +struct dma_heap_allocation_data *req; + +ksft_print_msg ("Testing no fd leak when copy_to_user() fails:\n" ); + + heap_fd =dmabuf_heap_open (heap_name); + + page_size =sysconf (_SC_PAGESIZE); + +/* +* Place the ioctl argument in its own private anonymous page so +* we can flip its protection independently. +*/ + req =mmap (NULL , page_size,PROT_READ |PROT_WRITE , +MAP_PRIVATE |MAP_ANONYMOUS ,-1 ,0 ); +if (req ==MAP_FAILED ) { +ksft_test_result_fail ("mmap failed: %s\n" ,strerror (errno)); + goto out; + } + +memset (req,0 ,sizeof (*req)); + req->len = page_size; + req->fd_flags =O_RDWR |O_CLOEXEC ; + +/* Count open fds before the ioctl */ + fd_before =0 ; + { +DIR *d =opendir ("/proc/self/fd" ); +struct dirent *de; + +if (!d) {
tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c static void test_alloc_errors(char *heap_name) · line 453
Quoted 10 lines
tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c +10
-0+ksft_test_result_fail ("opendir /proc/self/fd: %s\n" , +strerror (errno)); +munmap (req, page_size); + goto out; + } +while ((de =readdir (d))) +if (de->d_name[0 ] !='.' ) + fd_before++; +closedir (d); +/* subtract the fd opened by opendir itself */
But no actual subtraction?
tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c static void test_alloc_errors(char *heap_name)
Quoted 5 lines
tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c +5
-0+ } + +/* +* Make the page read-only: copy_from_user() in the kernel will +* still succeed (it already ran),
Huh? copy_from_user hasn't run yet. That happens inside the ioctl().
tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c static void test_alloc_errors(char *heap_name) · line 469
Quoted 12 lines
tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c +11
-0but copy_to_user() that writes +* the fd number back will fault. +*/ +mprotect (req, page_size,PROT_READ ); + + ret =ioctl (heap_fd,DMA_HEAP_IOCTL_ALLOC , req); + +/* Re-allow writes so munmap can clean up */ +mprotect (req, page_size,PROT_READ |PROT_WRITE ); +munmap (req, page_size); + +if (ret !=-1 || errno !=EFAULT ) {
This looks like you meant &&, but I think we should just fail if ret != -1. Either the mprotect is broken, or dma-heap didn't actually try to copy_to_user.
tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c static void test_alloc_errors(char *heap_name)
Quoted 3 lines
tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c +3
-0+/* +* If the ioctl didn't fail with EFAULT, either the kernel +* handled it differently or mprotect raced.
mprotect is synchronous, how could it race with anything here?
Quoted 18 lines
tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c +17
-0Skip rather +* than giving a false pass/fail. +*/ +ksft_test_result_skip ( +"ioctl did not return EFAULT (ret=%d errno=%d), skipping\n" , + ret, errno); + goto out; + } + +/* Count open fds after the failed ioctl */ + fd_after =0 ; + { +DIR *d =opendir ("/proc/self/fd" ); +struct dirent *de; + +if (!d) { +ksft_test_result_fail ("opendir /proc/self/fd: %s\n" , +strerror (errno));
tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c static void test_alloc_errors(char *heap_name) · line 499
Quoted 10 lines
tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c +10
-0+ goto out; + } +while ((de =readdir (d))) +if (de->d_name[0 ] !='.' ) + fd_after++; +closedir (d); + } + + ksft_test_result(fd_before == fd_after, + "no fd leak on EFAULT: before=%d after=%d\n",
This is for the failure case, so I don't think the "no" should be in the string.
Quoted 28 lines
tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c +5
-0+ fd_before, fd_after); +out: +close (heap_fd); +} +static int numer_of_heaps (void ){ DIR *d = opendir(DEVPATH);tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c +2
-2@@ -420,7 +532,7 @@ int main(void)return KSFT_SKIP ; } -ksft_set_plan (11 *numer_of_heaps ()); +ksft_set_plan (12 *numer_of_heaps ());while ((dir =readdir (d))) {if (!strncmp (dir->d_name,"." ,2 ))@@ -435,6 +547,7 @@ int main(void)test_alloc_zeroed (dir->d_name,ONE_MEG );test_alloc_compat (dir->d_name);test_alloc_errors (dir->d_name); +test_alloc_no_fd_leak_on_efault (dir->d_name); }closedir (d); --2.34.1
PATCH v3 2/2 misc: fastrpc: don't publish fd before copy_to_user() succeeds
In reply to Baineng Shoufastrpc_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.
--- drivers/misc/fastrpc.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-)
drivers/misc/fastrpc.c +6 -10
@@ -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 ; }
Signature · 1 line
2.34.1
[email protected]
AI reviewRe: [PATCH v3 2/2] misc: fastrpc: don't publish fd before copy_to_user() succeeds
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
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 <[email protected]>
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.
Quoted 4 lines
drivers/misc/fastrpc.c +0
-0
[ ... ]
drivers/misc/fastrpc.c static int fastrpc_dmabuf_alloc(struct fastrpc_user *fl, char __user *argp) · line 1712
Quoted 6 lines
drivers/misc/fastrpc.c +1
-1@@ -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.
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
Re: [PATCH v3 2/2] misc: fastrpc: don't publish fd before copy_to_user() succeeds
In reply to Baineng ShouQuoted 17 lines · Baineng Shou
fastrpc_ioctl_alloc_dmabuf()callsdma_buf_fd()which installs the fd into the caller's fd table beforecopy_to_user()copies the fd number back to userspace. Ifcopy_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 calledfd_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 withget_unused_fd_flags(), attemptcopy_to_user(), and only on success calldma_buf_fd_install()to publish it atomically with the tracepoint. Oncopy_to_user()failure,put_unused_fd()anddma_buf_put()cleanly unwind without any user-visible side effects.
Quoted 3 lines
Fixes 6cffd79504ce ("misc: fastrpc: Add support for dmabuf exporter")Signed-off-by Baineng Shou <[email protected]>
Christian König
maintainerRe: [PATCH v3 0/2] dma-buf: fix fd leak when copy_to_user() fails after fd_install()
In reply to Baineng ShouQuoted 15 lines · Baineng Shou
Several drivers call
dma_buf_fd()— which internally callsfd_install()— beforecopy_to_user()returns the fd number to userspace. Ifcopy_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 betweenfd_install()andcopy_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), docopy_to_user(), and only then publish the fd withfd_install()via the newdma_buf_fd_install()helper. Oncopy_to_user()failure,put_unused_fd()+dma_buf_put()cleanly unwind with no user-visible side effects.
Quoted 6 lines
Patch 1 introduces
dma_buf_fd_install()indma-buf.c(wrappingfd_install()together with theDMA_BUF_TRACEcall 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 <[email protected]> to the whole series.
Regards,
Christian.
Quoted 20 lines
v1: https://lore.kernel.org/dri-devel/[email protected]/
v2: https://lore.kernel.org/dri-devel/[email protected]/Changes in v3:
- Split into two patches (dma-heap + fastrpc separately)
- Adddma_buf_fd_install()to preservetrace_dma_buf_fdtracepoint (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 beforecopy_to_user()succeeds
misc: fastrpc: don't publish fd beforecopy_to_user()succeedsdrivers/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 1/1 drm/prime: use dma_buf_fd_install() to preserve export tracing
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/[email protected]/
--- drivers/gpu/drm/drm_prime.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/gpu/drm/drm_prime.c +1 -1
@@ -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 ; }
Signature · 1 line
2.34.1
[email protected]
AI reviewRe: [PATCH v3] drm/prime: use dma_buf_fd_install() to preserve export tracing
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
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 <[email protected]>
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.
drivers/gpu/drm/drm_prime.c int drm_gem_prime_handle_to_fd(struct drm_device *dev, · line 527
Quoted 10 lines
drivers/gpu/drm/drm_prime.c +1
-1@@ -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?
Quoted 3 lines
drivers/gpu/drm/drm_prime.c +0
-0*prime_fd = fd;return 0 ; }
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
Re: [PATCH v2] dma-buf: dma-heap: don't publish fd before copy_to_user() succeeds
In reply to Baineng ShouQuoted 8 lines · Baineng Shou
DMA_HEAP_IOCTL_ALLOCallocates a dma-buf and installs an fd into the caller's fd table viadma_buf_fd()->fd_install()beforedma_heap_ioctl()copies the result back to userspace. If the trailingcopy_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.
Quoted 5 lines
The obvious "close it on the failure path" fix is unsafe: once
fd_install()has run, another thread can alreadydup()the fd, send it viaSCM_RIGHTS, orclose()it and let its number be reused, so a subsequentclose_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.
Quoted 30 lines
Restructure the allocation path so that
fd_install()is the last, unfailable step of a successful ioctl:1.
heap->ops->allocate()creates thedma_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 withput_unused_fd()and thedma_bufreference is dropped withdma_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 structdma_buf* directly (returningERR_PTRon failure) so the caller holds the dmabuf reference across steps 3 and 4. The fd is written into the kdata buffer beforecopy_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_datathat lives in a page whose protection is flipped toPROT_READbetweencopy_from_user()andcopy_to_user()(e.g. viamprotect()). 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.
Quoted 7 lines
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")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.
Quoted 136 lines
--- drivers/dma-buf/dma-heap.c | 80 +++++++++++++++++++------------------- 1 file changed, 40 insertions(+), 40 deletions(-)drivers/dma-buf/dma-heap.c +40
-40@@ -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 1/1 dma-buf: dma-heap: don't publish fd before copy_to_user() succeeds
In reply to Baineng ShouDMA_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/[email protected]/
--- 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(-)
drivers/dma-buf/dma-buf.c +20 -0
@@ -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
drivers/dma-buf/dma-heap.c +40 -40
124 lines not on this page · raw message
include/linux/dma-buf.h +1 -0
@@ -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);
Signature · 1 line
2.34.1
[email protected]
AI reviewRe: [PATCH v2] dma-buf: dma-heap: don't publish fd before copy_to_user() succeeds
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
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 <[email protected]>
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.
drivers/dma-buf/dma-buf.c int dma_buf_fd(struct dma_buf *dmabuf, int flags) · line 800
Quoted 7 lines
drivers/dma-buf/dma-buf.c +0
-0@@ -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?
[ ... ]
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1