Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [Critical] dma-buf: race in dma_buf_set_release_eventfd() causes eventfd UAF and leak - [Medium] dma-buf: ioctl argument type mismatch in dma_buf_ioctl() - [Medium] dma-buf: single overwritable release_eventfd allows disarming notifications --
commit 006342891462f158abfae40fb1d0a9db415351f4 Author: Val Packett <[email protected]> dma-buf: Add DMA_BUF_IOCTL_SET_RELEASE_EVENTFD This commit introduces the DMA_BUF_IOCTL_SET_RELEASE_EVENTFD ioctl so userspace can register an eventfd context to be signaled when a dma-buf is released. > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > index 36cdc342627ee..f6437a4e9284b 100644 > --- a/drivers/dma-buf/dma-buf.c > +++ b/drivers/dma-buf/dma-buf.c [ ... ] > @@ -185,6 +186,11 @@ static void dma_buf_release(struct dentry *dentry) > if (dmabuf->resv == (struct dma_resv *)&dmabuf[1]) > dma_resv_fini(dmabuf->resv); > > + if (dmabuf->release_eventfd) { > + eventfd_signal(dmabuf->release_eventfd); > + eventfd_ctx_put(dmabuf->release_eventfd); > + } > + > WARN_ON(!list_empty(&dmabuf->attachments)); > module_put(dmabuf->owner); > kfree(dmabuf->name); > @@ -537,6 +543,21 @@ static long dma_buf_import_sync_file(struct dma_buf > *dmabuf, > } > #endif > > +static long dma_buf_set_release_eventfd(struct dma_buf *dmabuf, > + u32 fd) > +{ > + struct eventfd_ctx *ev_fd_ctx = eventfd_ctx_fdget(fd); > + > + if (IS_ERR(ev_fd_ctx)) > + return PTR_ERR(ev_fd_ctx); > + > + if (dmabuf->release_eventfd) > + eventfd_ctx_put(dmabuf->release_eventfd); > + > + dmabuf->release_eventfd = ev_fd_ctx; > + return 0; > +} [Severity: Critical] Can concurrent DMA_BUF_IOCTL_SET_RELEASE_EVENTFD calls on the same dma-buf race in dma_buf_set_release_eventfd() in drivers/dma-buf/dma-buf.c and cause a use-after-free or leak of struct eventfd_ctx? dma_buf_ioctl() runs from .unlocked_ioctl without holding a lock and calls dma_buf_set_release_eventfd(). Unlike dma_buf_set_name(), which serializes updates with dmabuf->name_lock, dma_buf_set_release_eventfd() reads, puts, and overwrites dmabuf->release_eventfd without locking or an atomic exchange: Thread 1 Thread 2 dma_buf_ioctl() dma_buf_ioctl() dma_buf_set_release_eventfd() dma_buf_set_release_eventfd() if (dmabuf->release_eventfd) if (dmabuf->release_eventfd) eventfd_ctx_put(release_eventfd); eventfd_ctx_put(release_eventfd); dmabuf->release_eventfd = ctx1; dmabuf->release_eventfd = ctx2; If dmabuf->release_eventfd is already set, both threads can observe the same old pointer and call eventfd_ctx_put() on it twice, underflowing its reference count and leading to a use-after-free when the eventfd is later signaled in dma_buf_release() or closed. Similarly, if Thread 1 assigns dmabuf->release_eventfd = ctx1 after Thread 2 has already checked dmabuf->release_eventfd, Thread 2 will overwrite ctx1 with ctx2 without calling eventfd_ctx_put(ctx1), leaking the ctx1 reference acquired by eventfd_ctx_fdget(). Should dmabuf->release_eventfd updates be synchronized with a lock or xchg()? > + > static long dma_buf_ioctl(struct file *file, > unsigned int cmd, unsigned long arg) > { > @@ -587,6 +608,9 @@ static long dma_buf_ioctl(struct file *file, > return dma_buf_import_sync_file(dmabuf, (const void __user > *)arg); > #endif > > + case DMA_BUF_IOCTL_SET_RELEASE_EVENTFD: > + return dma_buf_set_release_eventfd(dmabuf, (u32)arg); > + > default: > return -ENOTTY; > } > diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h > index d15b2b31d3c91..07b4a589a7935 100644 > --- a/include/linux/dma-buf.h > +++ b/include/linux/dma-buf.h > @@ -429,6 +429,9 @@ struct dma_buf { > > __poll_t active; > } cb_in, cb_out; > + > + /** @release_eventfd: for userspace release notification */ > + struct eventfd_ctx *release_eventfd; > }; [Severity: Medium] What happens if more than one process registers a release eventfd on the same struct dma_buf, or if a recipient of the dma-buf fd calls DMA_BUF_IOCTL_SET_RELEASE_EVENTFD? Because struct dma_buf in include/linux/dma-buf.h stores a single release_eventfd pointer and dma_buf_set_release_eventfd() in drivers/dma-buf/dma-buf.c drops and replaces any existing context: if (dmabuf->release_eventfd) eventfd_ctx_put(dmabuf->release_eventfd); dmabuf->release_eventfd = ev_fd_ctx; if a host backend registers an eventfd and then passes the dma-buf file descriptor over a socket to another process (such as a compositor or client) that also invokes DMA_BUF_IOCTL_SET_RELEASE_EVENTFD, the first eventfd_ctx is released via eventfd_ctx_put() without ever being signaled by dma_buf_release(). Could that leave the original waiter blocked indefinitely waiting for buffer release, and would a list of registered eventfd contexts (or rejecting re-registration) avoid allowing one holder to disarm another's notification? > > /** > diff --git a/include/uapi/linux/dma-buf.h b/include/uapi/linux/dma-buf.h > index e827c9d20c5d3..566dea778ad06 100644 > --- a/include/uapi/linux/dma-buf.h > +++ b/include/uapi/linux/dma-buf.h > @@ -179,5 +179,6 @@ struct dma_buf_import_sync_file { > #define DMA_BUF_SET_NAME_B _IOW(DMA_BUF_BASE, 1, __u64) > #define DMA_BUF_IOCTL_EXPORT_SYNC_FILE _IOWR(DMA_BUF_BASE, 2, struct > dma_buf_export_sync_file) > #define DMA_BUF_IOCTL_IMPORT_SYNC_FILE _IOW(DMA_BUF_BASE, 3, struct > dma_buf_import_sync_file) > +#define DMA_BUF_IOCTL_SET_RELEASE_EVENTFD _IOW(DMA_BUF_BASE, 4, __u32) [Severity: Medium] Is DMA_BUF_IOCTL_SET_RELEASE_EVENTFD in include/uapi/linux/dma-buf.h intended to take a user pointer to __u32 or a file descriptor value directly? Here DMA_BUF_IOCTL_SET_RELEASE_EVENTFD is defined with _IOW(DMA_BUF_BASE, 4, __u32), which indicates a user pointer argument, and dma_buf_fops in drivers/dma-buf/dma-buf.c uses compat_ptr_ioctl: static const struct file_operations dma_buf_fops = { ... .unlocked_ioctl = dma_buf_ioctl, .compat_ioctl = compat_ptr_ioctl, ... }; However, dma_buf_ioctl() in drivers/dma-buf/dma-buf.c casts arg directly by value instead of copying from a user pointer: case DMA_BUF_IOCTL_SET_RELEASE_EVENTFD: return dma_buf_set_release_eventfd(dmabuf, (u32)arg); Callers or language bindings following the _IOW(..., __u32) definition and passing a pointer to a __u32 will have the truncated pointer address treated as the file descriptor, while passing an integer by value conflicts with compat_ptr_ioctl(). Should dma_buf_ioctl() read the descriptor via get_user() / copy_from_user() to match the _IOW(..., __u32) definition? > > #endif -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
