Hi Christian, Alex,
I'd be very grateful for your help on this:
On 09/10/2026 14:11, [email protected] wrote:
Thank you for your contribution! Sashiko AI review found 1 potential issue(s)
to consider:
- [High] dma-buf: NULL pointer dereference of dmabuf->file in exporter drivers
--
commit b5be810d5fa3244638a9bad8503cacb8ca86027e
Author: Matt Evans <[email protected]>
dma-buf: Annul dmabuf->file on file release
This patch sets dmabuf->file to NULL in dma_buf_file_release() to prevent a
use-after-free window if the dentry outlives the file. By clearing the
pointer, exporters using get_file_active() can safely observe that the file
is closed.
diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index 36cdc342627ee..d0f8b766e8fd1 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -193,11 +193,26 @@ static void dma_buf_release(struct dentry *dentry)
static int dma_buf_file_release(struct inode *inode, struct file *file)
{
- if (!is_dma_buf_file(file))
- return -EINVAL;
+ struct dma_buf *dmabuf = file->private_data;
+
+ if (!dmabuf)
+ return 0;
- __dma_buf_list_del(file->private_data);
+ __dma_buf_list_del(dmabuf);
+ /*
+ * As the file is being closed, dmabuf->file is no longer
+ * valid and NULL replaces an otherwise stale pointer. If a
+ * reference to the file isn't already held, the file can be
+ * closed at any time, including before dmabuf->ops->release()
+ * happens: dmabuf->file _must not_ be accessed directly. An
+ * exporter should use get_file_active(&dmabuf->file) which
+ * atomically tests and acquires a reference to the file (or
+ * returns NULL if the file has closed). The barrier ensures
+ * get_file_active() observes the store before file_free()
+ * frees the memory.
+ */
+ smp_store_mb(dmabuf->file, NULL);
[Severity: High]
Does setting dmabuf->file to NULL here cause a NULL pointer dereference in
exporter drivers that still hold a reference to the dma_buf?
For example, if a GEM object is exported to a DMA-BUF and the file descriptor
is closed, the dma_buf is kept alive by the GEM object. If a user then reads
the debugfs file, amdgpu_bo_print_info() in
drivers/gpu/drm/amd/amdgpu/amdgpu_object.c dereferences dmabuf->file:
seq_printf(m, " imported from ino:%llu",
file_inode(dma_buf->file)->i_ino);
else if (dma_buf)
seq_printf(m, " exported as ino:%llu",
file_inode(dma_buf->file)->i_ino);
Similarly, during cleanup, amdgpu_ualink_cleanup_imp_xa_node() in
drivers/gpu/drm/amd/amdgpu/amdgpu_ualink.c attempts to read the file count:
dev_dbg(adev->dev,
"IMP-CLEANUP: dmabuf free for NPA:%llx handle:%llx:%llx,
fc:%lu\n",
imp_xa_node->npa_addr, imp_xa_node->handle.handle_hi,
imp_xa_node->handle.handle_lo,
file_count(imp_xa_node->dmabuf->file));
Could these routines trigger a kernel panic when dmabuf->file is NULL?
I am out on a limb here, being the first time I've read this driver. :-I
It _looks_ like there's a live dmabuf->file because a
get_dma_buf()-style reference is held here (so a false positive from
Sashiko). But I'm quite uncertain.
Christian, that you hadn't already mentioned this hints to me that this
is safe, is that fair?
Thanks,
Matt
