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; }