Re: [PATCH v5 06/13] media: iris: Route buffers to the matching context bank device

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] media: use-after-free of context bank devices during deferred V4L2 cleanup
--

commit 720de65140e46882c5d0651161ef771fcb1189a2
Author: Vikash Garodia <vikash.garodia@oss.qualcomm.com>

media: iris: Route buffers to the matching context bank device

The VPU issues DMA through several SMMU streams, and the hardware does
not give every stream the same addressable range. The non-pixel stream
cannot address the low 600MB of IOVA space, while the pixel stream can
address the full range.
Add iris_get_cb_dev(), which maps a buffer type to the owning context
bank device. Bitstream and internal buffers (BIN, ARP, COMV, LINE,
NON_COMV, PERSIST) belong to the non-pixel device, and uncompressed
buffers (DPB, PARTIAL, SCRATCH_1, SCRATCH_2, VPSS) to the pixel device.
BUF_INPUT and BUF_OUTPUT depend on direction and are resolved from
inst->domain: for a decoder the input is non-pixel and the output pixel,
and the other way round for an encoder.
Fall back to core->dev whenever the relevant context bank device is
absent, so platforms still describing "iommus" on the parent iris node
behave exactly as before to maintain backward compatibility.

> diff --git a/drivers/media/platform/qcom/iris/iris_buffer.c b/drivers/media/platform/qcom/iris/iris_buffer.c
> index eb8de60c1177f..6e03d13ac1a79 100644
> --- a/drivers/media/platform/qcom/iris/iris_buffer.c
> +++ b/drivers/media/platform/qcom/iris/iris_buffer.c
> @@ -531,7 +531,7 @@ static int iris_create_internal_buffer(struct iris_inst *inst,
>  				       enum iris_buffer_type buffer_type, u32 index)
>  {
>  	struct iris_buffers *buffers = &inst->buffers[buffer_type];
> -	struct iris_core *core = inst->core;
> +	struct device *dev = iris_get_cb_dev(inst, buffer_type);

[Severity: High]
Does this acquire the context bank device for DMA mapping without taking a
reference to guarantee its lifetime matches the buffer?

If iris_remove() explicitly calls platform_device_unregister() on these
devices upon sysfs driver unbind, and no standard buffers are active, their
refcount can drop to 0 and they will be freed.

>  	struct iris_buffer *buffer;
>  
>  	if (!buffers->size)

[ ... ]

> @@ -650,10 +650,10 @@ int iris_queue_internal_buffers(struct iris_inst *inst, u32 plane)
>  
>  void iris_destroy_internal_buffer(struct iris_inst *inst, struct iris_buffer *buffer)
>  {
> -	struct iris_core *core = inst->core;
> +	struct device *dev = iris_get_cb_dev(inst, buffer->type);
>  
>  	list_del(&buffer->list);
> -	dma_free_attrs(core->dev, buffer->buffer_size, buffer->kvaddr,
> +	dma_free_attrs(dev, buffer->buffer_size, buffer->kvaddr,

[Severity: High]
Can this result in a use-after-free of the context bank device during
deferred V4L2 cleanup?

If standard buffers are active, videobuf2 holds a reference. However, during
deferred cleanup in iris_close(), v4l2_m2m_ctx_release() frees standard
buffers before iris_destroy_all_internal_buffers() runs. This drops the
final refcount, freeing the devices.

Will this fetch and dereference the freed device pointer, passing it to
dma_free_attrs()? Additionally, does the devm-allocated core also get freed
on unbind, making inst->core a dangling pointer here?

[ ... ]

> diff --git a/drivers/media/platform/qcom/iris/iris_vidc.c b/drivers/media/platform/qcom/iris/iris_vidc.c
> index fcbc60016beec..064cf0a80748d 100644
> --- a/drivers/media/platform/qcom/iris/iris_vidc.c
> +++ b/drivers/media/platform/qcom/iris/iris_vidc.c
> @@ -108,7 +108,7 @@ iris_m2m_queue_init(void *priv, struct vb2_queue *src_vq, struct vb2_queue *dst_
>  	src_vq->drv_priv = inst;
>  	src_vq->buf_struct_size = sizeof(struct iris_buffer);
>  	src_vq->min_reqbufs_allocation = MIN_BUFFERS;
> -	src_vq->dev = inst->core->dev;
> +	src_vq->dev = iris_get_cb_dev(inst, BUF_INPUT);

[Severity: High]
Does this assign a manually lifetime-managed, short-lived device to the
long-lived vb2_queue context?

>  	src_vq->lock = &inst->ctx_q_lock;
>  	ret = vb2_queue_init(src_vq);
>  	if (ret)
> @@ -122,7 +122,7 @@ iris_m2m_queue_init(void *priv, struct vb2_queue *src_vq, struct vb2_queue *dst_
>  	dst_vq->drv_priv = inst;
>  	dst_vq->buf_struct_size = sizeof(struct iris_buffer);
>  	dst_vq->min_reqbufs_allocation = MIN_BUFFERS;
> -	dst_vq->dev = inst->core->dev;
> +	dst_vq->dev = iris_get_cb_dev(inst, BUF_OUTPUT);

[Severity: High]
Does this also assign a manually lifetime-managed, short-lived device to the
long-lived vb2_queue context?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260926-vpu_iommu_iova_handling-v5-0-0322ca5dc10c@oss.qualcomm.com?part=6