Message-ID: <aIEiJL83pOYO8lUJ@sultan-box>
Date: Wed, 23 Jul 2025 10:55:48 -0700
From: Sultan Alsawaf <sultan@...neltoast.com>
To: Bin Du <Bin.Du@....com>
Cc: mchehab@...nel.org, hverkuil@...all.nl,
	laurent.pinchart+renesas@...asonboard.com,
	bryan.odonoghue@...aro.org, sakari.ailus@...ux.intel.com,
	prabhakar.mahadev-lad.rj@...renesas.com,
	linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
	pratap.nirujogi@....com, benjamin.chan@....com, king.li@....com,
	gjorgji.rosikopulos@....com, Phil.Jawich@....com,
	Dominic.Antony@....com, Svetoslav.Stoilov@....com
Subject: Re: [PATCH v2 6/8] media: platform: amd: isp4 video node and buffers
 handling added

On Wed, Jun 18, 2025 at 05:19:57PM +0800, Bin Du wrote:
> +static void isp4vid_vb2_detach_dmabuf(void *mem_priv)
> +{
> +	struct isp4vid_vb2_buf *buf = mem_priv;
> +
> +	if (!buf) {
> +		pr_err("fail invalid buf handle\n");
> +		return;
> +	}
> +
> +	struct iosys_map map = IOSYS_MAP_INIT_VADDR(buf->vaddr);

Variable declaration mixed with code, move the variable declaration to the top.

> +
> +	dev_dbg(buf->dev, "detach dmabuf of isp user bo 0x%llx size %ld",
> +		buf->gpu_addr, buf->size);
> +
> +	if (buf->vaddr)
> +		dma_buf_vunmap_unlocked(buf->dbuf, &map);
> +
> +	// put dmabuf for exported ones
> +	dma_buf_put(buf->dbuf);

The dmabuf shouldn't be put from the detach_dmabuf memop. Remove this.

> +
> +	kfree(buf);
> +}

[snip]

> +static void isp4vid_vb2_dmabuf_ops_release(struct dma_buf *dbuf)
> +{
> +	struct isp4vid_vb2_buf *buf = dbuf->priv;
> +
> +	/* drop reference obtained in vb2_isp4vid_get_dmabuf */

s/vb2_isp4vid_get_dmabuf/isp4vid_vb2_get_dmabuf/

> +	if (buf->is_expbuf)
> +		isp4vid_vb2_put(dbuf->priv);
> +	else
> +		dev_dbg(buf->dev, "ignore buf release for implicit case");
> +}

[snip]

> +static struct dma_buf *isp4vid_vb2_get_dmabuf(struct vb2_buffer *vb,
> +					      void *buf_priv,
> +					      unsigned long flags)
> +{
> +	struct isp4vid_vb2_buf *buf = buf_priv;
> +	struct dma_buf *dbuf;
> +
> +	if (buf->dbuf) {
> +		dev_dbg(buf->dev,
> +			"dbuf already created, reuse implicit dbuf\n");
> +		dbuf = buf->dbuf;

The dmabuf is reused here without taking a reference to it. When the get_dmabuf
memop is called by vb2_core_expbuf(), it assumes that a reference to the dmabuf
is acquired. So you need to add `get_dma_buf(dbuf)` here.

> +	} else {
> +		dbuf = isp4vid_get_dmabuf(vb, buf_priv, flags);
> +		dev_dbg(buf->dev, "created new dbuf\n");
> +	}
> +	buf->is_expbuf = true;
> +	refcount_inc(&buf->refcount);
> +
> +	dev_dbg(buf->dev, "buf exported, refcount %d\n",
> +		buf->refcount.refs.counter);
> +
> +	return dbuf;
> +}

[snip]

> +static void *isp4vid_vb2_get_userptr(struct vb2_buffer *vb, struct device *dev,
> +				     unsigned long vaddr, unsigned long size)
> +{
> +	struct isp4vid_vb2_buf *buf;
> +	struct frame_vector *vec;
> +	int n_pages, offset, i;
> +	int ret = -ENOMEM;
> +
> +	buf = kzalloc(sizeof(*buf), GFP_KERNEL);
> +	if (!buf)
> +		return ERR_PTR(-ENOMEM);
> +
> +	buf->dev = dev;
> +	buf->dma_dir = vb->vb2_queue->dma_dir;
> +	offset = vaddr & ~PAGE_MASK;
> +	buf->size = size;
> +	vec = vb2_create_framevec(vaddr, size,
> +				  buf->dma_dir == DMA_FROM_DEVICE ||
> +				  buf->dma_dir == DMA_BIDIRECTIONAL);
> +	if (IS_ERR(vec)) {
> +		kfree(buf);
> +		return vec;
> +	}

You can combine the error handling here with the error path at the bottom of the
function instead of duplicating the `kfree(buf)`.

> +	buf->vec = vec;
> +	n_pages = frame_vector_count(vec);
> +	if (frame_vector_to_pages(vec) < 0) {
> +		unsigned long *nums = frame_vector_pfns(vec);
> +
> +		/*
> +		 * We cannot get page pointers for these pfns. Check memory is
> +		 * physically contiguous and use direct mapping.
> +		 */
> +		for (i = 1; i < n_pages; i++)
> +			if (nums[i - 1] + 1 != nums[i])
> +				goto err_destroy_free;
> +		buf->vaddr = (__force void *)
> +			     ioremap(__pfn_to_phys(nums[0]), size + offset);
> +	} else {
> +		buf->vaddr = vm_map_ram(frame_vector_pages(vec), n_pages, -1);
> +	}
> +
> +	if (!buf->vaddr)
> +		goto err_destroy_free;
> +
> +	buf->vaddr = ((char *)buf->vaddr) + offset;
> +	return buf;
> +
> +err_destroy_free:
> +	vb2_destroy_framevec(vec);
> +	kfree(buf);
> +	return ERR_PTR(ret);
> +}
> +
> +static void isp4vid_vb2_put(void *buf_priv)
> +{
> +	struct isp4vid_vb2_buf *buf = (struct isp4vid_vb2_buf *)buf_priv;
> +	struct amdgpu_bo *bo = (struct amdgpu_bo *)buf->bo;
> +
> +	dev_dbg(buf->dev,
> +		"release isp user bo 0x%llx size %ld refcount %d is_expbuf %d",
> +		buf->gpu_addr, buf->size,
> +		buf->refcount.refs.counter, buf->is_expbuf);
> +
> +	if (refcount_dec_and_test(&buf->refcount)) {
> +		amdgpu_bo_free_isp_user(bo);
> +
> +		// put implicit dmabuf here, detach_dmabuf will not be called

Comment style (use /**/ instead of //).

> +		if (!buf->is_expbuf)
> +			dma_buf_put(buf->dbuf);
> +
> +		vfree(buf->vaddr);
> +		kfree(buf);
> +		buf = NULL;

`buf = NULL;` here is superfluous; you can remove it.

> +	} else {
> +		dev_warn(buf->dev, "ignore buffer free, refcount %u > 0",
> +			 refcount_read(&buf->refcount));

This refcount_read() is a possible use-after-free because `buf` is accessed
after isp4vid_vb2_put() puts its reference to `buf`. So something else could put
the last reference to `buf` and free it after this refcount dec but before the
refcount_read(). Maybe just remove this dev_warn() entirely?

> +	}
> +}
> +
> +static void *isp4vid_vb2_alloc(struct vb2_buffer *vb, struct device *dev,
> +			       unsigned long size)
> +{
> +	struct isp4vid_dev *isp_vdev = vb2_get_drv_priv(vb->vb2_queue);
> +	struct isp4vid_vb2_buf *buf = NULL;
> +	struct amdgpu_bo *bo;
> +	u64 gpu_addr;
> +	u32 ret;
> +
> +	buf = kzalloc(sizeof(*buf), GFP_KERNEL | vb->vb2_queue->gfp_flags);
> +	if (!buf)
> +		return ERR_PTR(-ENOMEM);
> +
> +	buf->dev = dev;
> +	buf->size = size;
> +	buf->vaddr = vmalloc_user(buf->size);
> +	if (!buf->vaddr) {
> +		kfree(buf);
> +		return ERR_PTR(-ENOMEM);
> +	}
> +
> +	buf->dma_dir = vb->vb2_queue->dma_dir;
> +	buf->handler.refcount = &buf->refcount;
> +	buf->handler.put = isp4vid_vb2_put;
> +	buf->handler.arg = buf;

What is buf->handler for? I don't see it used anywhere in the entire patchset; I
can delete `handler` from `struct isp4vid_vb2_buf` along with these lines and it
compiles. 

> +
> +	// get implicit dmabuf

Comment style.

> +	buf->dbuf = isp4vid_get_dmabuf(vb, buf, 0);
> +	if (!buf->dbuf) {
> +		dev_err(dev, "fail to get dmabuf\n");
> +		return ERR_PTR(-EINVAL);
> +	}

Doesn't free `buf` or `buf->vaddr` on error here. Also, comment style.

> +
> +	// create isp user BO and obtain gpu_addr

Comment style.

> +	ret = amdgpu_bo_create_isp_user(isp_vdev->amdgpu_dev, buf->dbuf,
> +					AMDGPU_GEM_DOMAIN_GTT, &bo, &gpu_addr);
> +	if (ret) {
> +		dev_err(dev, "fail to create BO\n");
> +		return ERR_PTR(-EINVAL);
> +	}

Doesn't free `buf` or `buf->vaddr` or put `buf->dbuf` on error here.

> +
> +	buf->bo = (void *)bo;
> +	buf->gpu_addr = gpu_addr;
> +
> +	refcount_set(&buf->refcount, 1);

This discards the refcount inc triggered from amdgpu_bo_create_isp_user() when
it calls get_dma_buf(), leading to a use-after-free. Move this refcount_set()
up, preferably right after vmalloc_user() or right after `buf` is allocated so
there's no risk of this issue occurring again in the future.

> +
> +	dev_dbg(dev, "allocated isp user bo 0x%llx size %ld refcount %d",
> +		buf->gpu_addr, buf->size, buf->refcount.refs.counter);
> +
> +	return buf;
> +}

[snip]

Sultan

Powered by blists - more mailing lists