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

[Vishnu Reddy <busanna.reddy@oss.qualcomm.com>]

On 9/26/2026 12:19 PM, sashiko-bot@kernel.org wrote:
> 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.

This is not true. Remove callback cannot complete if driver module->refcnt,
refcounted during open, and not decremented with corresponding close call.
iris_close() would call iris_destroy_internal_buffers() to free up the buffers
before un-registering the device.

> 
>>  	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.

iris_destroy_all_internal_buffers() frees the internal buffers which is managed
within the driver outside the scope of v4l2_m2m_ctx_release().

> 
> 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?

Unbind will not happen until iris_close() completes(explained above), which
avoids this scenario.

> 
> [ ... ]
> 
>> 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?

These devices are long-lived when compare to vb2_queue and are freed only in
iris_remove().

>