Hi Christian,
On 12/10/2026 18:48, Christian König wrote:
On 10/12/26 17:27, Matt Evans wrote:
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?
The Sashiko comment is nonsense. Each GEM object which has a DMA-buf (either
through import or export) holds a reference to the DMA-buf file.
The only case where it is temporarily not holding the reference is during the
destruction dance, e.g. handle ref goes to zero, DMA-buf is released and freed,
GEM object is released and freed.
Additional to that this is for debugfs which is meant to be used for debugging
only and is not really stable. We literally have tons of debugfs files which do
very questionable things with locks and/or RCU and sometimes just returns
random data because the object in question is about to be destroyed.
A NULL pointer deref is a bit more problematic. But in general most people
don't care much, it's debugfs.
That makes sense, and I really appreciate the explanation. Thanks for
the help.
Matt
