History

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/175e98de-f414-47d7-81c1-c0fe0a8f7f62@amd.com/

Baineng Shou committed 2 months ago

Commit 30d0aff1 parent cb732d03 files · +61 −40Browse files

Unified · Split

3 files
  1. drivers/dma-buf/dma-buf.c +20 −0
  2. drivers/dma-buf/dma-heap.c +40 −40
  3. include/linux/dma-buf.h +1 −0
drivers/dma-buf/dma-buf.c+20−0
@@ -803,6 +803,26 @@ int dma_buf_fd(struct dma_buf *dmabuf, int flags) · 802 unmodified lines
803}803}
804EXPORT_SYMBOL_NS_GPL(dma_buf_fd, "DMA_BUF");804EXPORT_SYMBOL_NS_GPL(dma_buf_fd, "DMA_BUF");
805 805
806/**
807 * dma_buf_fd_install - install a reserved fd for a dma-buf
808 * @dmabuf: [in] pointer to dma_buf
809 * @fd: [in] fd reserved with get_unused_fd_flags()
810 *
811 * Publishes a previously reserved fd into the caller's fd table.
812 * Must only be called after all fallible work (e.g. copy_to_user)
813 * has succeeded, as it cannot be undone safely once called.
814 *
815 * The caller is responsible for having emitted the trace event
816 * (via dma_buf_fd() or get_unused_fd_flags() + this function)
817 * before calling this.
818 */
819void dma_buf_fd_install(struct dma_buf *dmabuf, int fd)
820{
821 DMA_BUF_TRACE(trace_dma_buf_fd, dmabuf, fd);
822 fd_install(fd, dmabuf->file);
823}
824EXPORT_SYMBOL_NS_GPL(dma_buf_fd_install, "DMA_BUF");
825
806/**826/**
807 * dma_buf_get - returns the struct dma_buf related to an fd827 * dma_buf_get - returns the struct dma_buf related to an fd
808 * @fd: [in] fd associated with the struct dma_buf to be returned828 * @fd: [in] fd associated with the struct dma_buf to be returned
drivers/dma-buf/dma-heap.c+40−40
@@ -55,33 +55,6 @@ MODULE_PARM_DESC(mem_accounting, · 54 unmodified lines
55 "Enable cgroup-based memory accounting for dma-buf heap allocations (default=false).");55 "Enable cgroup-based memory accounting for dma-buf heap allocations (default=false).");
56EXPORT_SYMBOL_NS_GPL(mem_accounting, "DMA_BUF_HEAP");56EXPORT_SYMBOL_NS_GPL(mem_accounting, "DMA_BUF_HEAP");
57 57
58static int dma_heap_buffer_alloc(struct dma_heap *heap, size_t len,
59 u32 fd_flags,
60 u64 heap_flags)
61{
62 struct dma_buf *dmabuf;
63 int fd;
64
65 /*
66 * Allocations from all heaps have to begin
67 * and end on page boundaries.
68 */
69 len = PAGE_ALIGN(len);
70 if (!len)
71 return -EINVAL;
72
73 dmabuf = heap->ops->allocate(heap, len, fd_flags, heap_flags);
74 if (IS_ERR(dmabuf))
75 return PTR_ERR(dmabuf);
76
77 fd = dma_buf_fd(dmabuf, fd_flags);
78 if (fd < 0) {
79 dma_buf_put(dmabuf);
80 /* just return, as put will call release and that will free */
81 }
82 return fd;
83}
84
85static int dma_heap_open(struct inode *inode, struct file *file)58static int dma_heap_open(struct inode *inode, struct file *file)
86{59{
87 struct dma_heap *heap;60 struct dma_heap *heap;
@@ -99,30 +72,42 @@ static int dma_heap_open(struct inode *inode, struct file *file) · 11 unmodified lines
99 return 0;72 return 0;
100}73}
101 74
102static long dma_heap_ioctl_allocate(struct file *file, void *data)75static struct dma_buf *dma_heap_ioctl_allocate(struct file *file, void *data)
103{76{
104 struct dma_heap_allocation_data *heap_allocation = data;77 struct dma_heap_allocation_data *heap_allocation = data;
105 struct dma_heap *heap = file->private_data;78 struct dma_heap *heap = file->private_data;
79 struct dma_buf *dmabuf;
106 int fd;80 int fd;
81 size_t len;
107 82
108 if (heap_allocation->fd)83 if (heap_allocation->fd)
109 return -EINVAL;84 return ERR_PTR(-EINVAL);
110 85
111 if (heap_allocation->fd_flags & ~DMA_HEAP_VALID_FD_FLAGS)86 if (heap_allocation->fd_flags & ~DMA_HEAP_VALID_FD_FLAGS)
112 return -EINVAL;87 return ERR_PTR(-EINVAL);
113 88
114 if (heap_allocation->heap_flags & ~DMA_HEAP_VALID_HEAP_FLAGS)89 if (heap_allocation->heap_flags & ~DMA_HEAP_VALID_HEAP_FLAGS)
115 return -EINVAL;90 return ERR_PTR(-EINVAL);
91
92 len = PAGE_ALIGN(heap_allocation->len);
93 if (!len)
94 return ERR_PTR(-EINVAL);
95
96 dmabuf = heap->ops->allocate(heap, len, heap_allocation->fd_flags,
97 heap_allocation->heap_flags);
116 98
117 fd = dma_heap_buffer_alloc(heap, heap_allocation->len,99 if (IS_ERR(dmabuf))
118 heap_allocation->fd_flags,100 return dmabuf;
119 heap_allocation->heap_flags);101
120 if (fd < 0)102 fd = get_unused_fd_flags(heap_allocation->fd_flags);
121 return fd;103 if (fd < 0) {
104 dma_buf_put(dmabuf);
105 return ERR_PTR(fd);
106 }
122 107
123 heap_allocation->fd = fd;108 heap_allocation->fd = fd;
124 109
125 return 0;110 return dmabuf;
126}111}
127 112
128static unsigned int dma_heap_ioctl_cmds[] = {113static unsigned int dma_heap_ioctl_cmds[] = {
@@ -138,6 +123,8 @@ static long dma_heap_ioctl(struct file *file, unsigned int ucmd, · 9 unmodified lines
138 unsigned int in_size, out_size, drv_size, ksize;123 unsigned int in_size, out_size, drv_size, ksize;
139 int nr = _IOC_NR(ucmd);124 int nr = _IOC_NR(ucmd);
140 int ret = 0;125 int ret = 0;
126 int fd;
127 struct dma_buf *dmabuf;
141 128
142 if (nr >= ARRAY_SIZE(dma_heap_ioctl_cmds))129 if (nr >= ARRAY_SIZE(dma_heap_ioctl_cmds))
143 return -EINVAL;130 return -EINVAL;
@@ -174,15 +161,28 @@ static long dma_heap_ioctl(struct file *file, unsigned int ucmd, · 30 unmodified lines
174 161
175 switch (kcmd) {162 switch (kcmd) {
176 case DMA_HEAP_IOCTL_ALLOC:163 case DMA_HEAP_IOCTL_ALLOC:
177 ret = dma_heap_ioctl_allocate(file, kdata);164 dmabuf = dma_heap_ioctl_allocate(file, kdata);
165
166 if (IS_ERR(dmabuf)) {
167 ret = PTR_ERR(dmabuf);
168 break;
169 }
170
171 fd = ((struct dma_heap_allocation_data *)kdata)->fd;
172 if (copy_to_user((void __user *)arg, kdata, out_size) != 0) {
173 put_unused_fd(fd);
174 dma_buf_put(dmabuf);
175 ret = -EFAULT;
176 } else {
177 dma_buf_fd_install(dmabuf, fd);
178 }
179
178 break;180 break;
179 default:181 default:
180 ret = -ENOTTY;182 ret = -ENOTTY;
181 goto err;183 goto err;
182 }184 }
183 185
184 if (copy_to_user((void __user *)arg, kdata, out_size) != 0)
185 ret = -EFAULT;
186err:186err:
187 if (kdata != stack_kdata)187 if (kdata != stack_kdata)
188 kfree(kdata);188 kfree(kdata);
include/linux/dma-buf.h+1−0
@@ -567,6 +567,7 @@ void dma_buf_unpin(struct dma_buf_attachment *attach); · 566 unmodified lines
567struct dma_buf *dma_buf_export(const struct dma_buf_export_info *exp_info);567struct dma_buf *dma_buf_export(const struct dma_buf_export_info *exp_info);
568 568
569int dma_buf_fd(struct dma_buf *dmabuf, int flags);569int dma_buf_fd(struct dma_buf *dmabuf, int flags);
570void dma_buf_fd_install(struct dma_buf *dmabuf, int fd);
570struct dma_buf *dma_buf_get(int fd);571struct dma_buf *dma_buf_get(int fd);
571void dma_buf_put(struct dma_buf *dmabuf);572void dma_buf_put(struct dma_buf *dmabuf);
572 573