[PATCH] dma-buf: Annul dmabuf->file on file release

[Matt Evans <matt@ozlabs.org>]

The dmabuf release path is split between file and dentry release:
dma_buf_file_release() is called shortly before the file is freed, and
dma_buf_release() calls an exporter's dmabuf->ops->release when the
dentry is freed.  However, the dentry can outlive the file (for
example, if opened with O_PATH), meaning .release might be called some
time after the file is freed.

This presents a window, when closing a DMABUF file, in which
dmabuf->file points to freed memory yet .release op has not yet been
called.

For VFIO, if a buffer's .release has not been called it's considered
still active and subject to move/cleanup.  If so, it attempts to
get_file_active() on dmabuf->file: a file close with dentry held open
will call this function with a stale pointer, a UAF.

To make this pattern safe, set dmabuf->file to NULL in
dma_buf_file_release() to reflect that the associated file is now dead
even if the DMABUF is not yet gone.  A get_file_active() ... fput()
sequence concurrent with a file close will only execute as one of:

  - Gets the file before file_ref_put() (dmabuf->file valid)
  - Observes dmabuf->file pointing to a file, but it's DEAD (no file)
  - Observes dmabuf->file = NULL (no file)

Originally, drivers could assume dmabuf->file was valid until .release
was called from fops->release.  This assumption was no longer valid
after 4ab59c3c638c6 ("dma-buf: Move dma_buf_release() from fops to
dentry_ops"), which moved the callback to the dentry release (by which
point the file might have been freed).  With this commit, drivers must
still consider that dmabuf->file could be NULL before .release.

Fixes: 4ab59c3c638c6 ("dma-buf: Move dma_buf_release() from fops to dentry_ops")
Signed-off-by: Matt Evans <matt@ozlabs.org>
---

Hi,

This issue was found (by Claude Opus 5.5) in the context of VFIO's
DMABUF export path.  VFIO iterates live DMABUFs with a
get_file_active()/fput() block, which now becomes safe if the file is
closed (and memory freed!) yet DMABUF .release hasn't yet occurred.

However, there are a couple of other places that directly use
dmabuf->file and seem able to race a closing file (i.e. without holding
the file reference)?  If this is so, they'd be a UAF today; with this
patch that goes away, but instead of a stale pointer dmabuf->file could
be NULL:

1. drivers/gpu/drm/vmwgfx/ttm_object.c:get_dma_buf_unless_doomed()

  file_ref_get(&dmabuf->file->f_ref); on a non-refcounted DMABUF

The commit message of 90ee6ed776c0 ("fs: port files to file_ref") hints
this might be more subtle than replacing it with a get_file() variant
(so as to accept a NULL file *).

2. drivers/gpu/drm/i915/gvt/dmabuf.c:intel_vgpu_get_dmabuf()

  gvt_dbg_dpy(... file_count(dmabuf->file) ...);


Respective vmwgfx & i915 maintainers, what is your view?
Or, indeed, if anyone sees any other questionable uses of dmabuf->file.

Thanks,


Matt


  drivers/dma-buf/dma-buf.c | 6 +++++-
  1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index 4c9add51f9ef..726639130477 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -193,11 +193,15 @@ static void dma_buf_release(struct dentry *dentry)
  
  static int dma_buf_file_release(struct inode *inode, struct file *file)
  {
+	struct dma_buf *dmabuf = file->private_data;
+
  	if (!is_dma_buf_file(file))
  		return -EINVAL;
  
-	__dma_buf_list_del(file->private_data);
+	__dma_buf_list_del(dmabuf);
  
+	/* Must be observed by __get_file_rcu() before file_free() */
+	smp_store_mb(dmabuf->file, NULL);
  	return 0;
  }
  
-- 
2.47.3