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. Regards, Christian. > > > Thanks, > > > Matt
