Re: [PATCH v2 6/8] media: platform: amd: isp4 video node and buffers handling added

[Sultan Alsawaf <sultan@kerneltoast.com>]

I found more refcounting issues in addition to the ones from my other emails
while trying to make my webcam work:

On Wed, Jun 18, 2025 at 05:19:57PM +0800, Bin Du wrote:
> +static int isp4vid_vb2_mmap(void *buf_priv, struct vm_area_struct *vma)
> +{
> +	struct isp4vid_vb2_buf *buf = buf_priv;
> +	int ret;
> +
> +	if (!buf) {
> +		pr_err("fail no memory to map\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = remap_vmalloc_range(vma, buf->vaddr, 0);
> +	if (ret) {
> +		dev_err(buf->dev, "fail remap vmalloc mem, %d\n", ret);
> +		return ret;
> +	}
> +
> +	/*
> +	 * Make sure that vm_areas for 2 buffers won't be merged together
> +	 */
> +	vm_flags_set(vma, VM_DONTEXPAND);
> +
> +	dev_dbg(buf->dev, "mmap isp user bo 0x%llx size %ld refcount %d\n",
> +		buf->gpu_addr, buf->size, buf->refcount.refs.counter);

Use refcount_read() instead of reading the refcount's atomic_t counter directly.
This is done in 3 other places; change those to refcount_read() as well.

This didn't cause any functional problems, but it should still be fixed.

> +
> +	return 0;
> +}

[snip]

> +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);
> +
> +	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);
> +
> +	kfree(buf);
> +}

As mentioned in the other email, the dma_buf_put() here needs to be removed. But
that's not all: the dma_buf_vunmap_unlocked() needs to be removed too because
vb2 will always unmap the buffer before detaching it. As a result, having the
dma_buf_vunmap_unlocked() call here results in a use-after-free when vb2 calls
the unmap_dmabuf memop.

Change this function to the following:

	static void isp4vid_vb2_detach_dmabuf(void *mem_priv)
	{
		struct isp4vid_vb2_buf *buf = mem_priv;

		kfree(buf);
	}

> +static void isp4vid_qops_buffer_cleanup(struct vb2_buffer *vb)
> +{
> +	struct isp4vid_dev *isp_vdev = vb2_get_drv_priv(vb->vb2_queue);
> +	struct isp4vid_vb2_buf *buf = vb->planes[0].mem_priv;
> +
> +	dev_dbg(isp_vdev->dev, "%s|index=%u vb->memory %u",
> +		isp_vdev->vdev.name, vb->index, vb->memory);
> +
> +	if (!buf) {
> +		dev_err(isp_vdev->dev, "Invalid buf handle");
> +		return;
> +	}
> +
> +	// release implicit dmabuf reference here for vb2 buffer
> +	// of type MMAP and is exported
> +	if (vb->memory == VB2_MEMORY_MMAP && buf->is_expbuf) {
> +		dma_buf_put(buf->dbuf);
> +		dev_dbg(isp_vdev->dev,
> +			"put dmabuf for vb->memory %d expbuf %d",
> +			vb->memory,
> +			buf->is_expbuf);
> +	}
> +}
> +

Remove the isp4vid_qops_buffer_cleanup() function. It causes a use-after-free by
doing an extra dma_buf_put(). This function isn't needed now that the refcount
issues are solved.

[snip]

> +static const struct vb2_ops isp4vid_qops = {
> +	.queue_setup = isp4vid_qops_queue_setup,
> +	.buf_cleanup = isp4vid_qops_buffer_cleanup,

Remove the .buf_cleanup hook too.

> +	.buf_queue = isp4vid_qops_buffer_queue,
> +	.start_streaming = isp4vid_qops_start_streaming,
> +	.stop_streaming = isp4vid_qops_stop_streaming,
> +	.wait_prepare = vb2_ops_wait_prepare,
> +	.wait_finish = vb2_ops_wait_finish,
> +};

[snip]

Along with the changes from my other emails, I believe this finally fixes all of
the refcounting issues. No more UaF or leaks here. :-)

Sultan