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

Sultan Alsawaf
2025-07-28T07:04:42Z

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