Re: [PATCH] dma-buf: Add DMA_BUF_IOCTL_SET_RELEASE_EVENTFD

[sashiko-bot@kernel.org]

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 <val@invisiblethingslab.com>

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/20261021093533.1990028-1-val@invisiblethingslab.com?part=1