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? > return 0; > } -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
