Re: [PATCH v2 2/2] dma-buf: Annul dmabuf->file on file release

[sashiko-bot@kernel.org]

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 <matt@ozlabs.org>

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/20261009123601.94079-1-matt@ozlabs.org?part=2