Re: [PATCH v9 04/14] media: iris: Add helper to select relevant context bank device

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Using dynamically created context bank devices without taking a reference via get_device() creates a Use-After-Free risk when file descriptors outlive device unbind.

Pre-existing issues:
- [High] The `struct iris_core` is allocated via devres but is accessed in V4L2 deferred release paths, causing a Use-After-Free if a file descriptor is closed after driver unbind.
--

--- Patch [4]: [PATCH v9 04/14] media: iris: Add helper to select relevant context bank device ---
commit ff67c19893e13d3ddb40ea541f363dbf54e13228
Author: Vikash Garodia <vikash.garodia@oss.qualcomm.com>

media: iris: Add helper to select relevant context bank device

This commit introduces a helper to associate the appropriate context bank 
device with different buffer types. It updates the internal buffer allocation 
and queue initialization to use this dynamically selected device instead of 
always defaulting to the main core device.

> diff --git a/drivers/media/platform/qcom/iris/iris_buffer.c b/drivers/media/platform/qcom/iris/iris_buffer.c
> index 246ad0abbac35..249c9f1d0d5da 100644
> --- a/drivers/media/platform/qcom/iris/iris_buffer.c
> +++ b/drivers/media/platform/qcom/iris/iris_buffer.c
> @@ -545,7 +545,7 @@ static int iris_create_internal_buffer(struct iris_inst *inst,
>  	buffer->buffer_size = buffers->size;
>  	buffer->dma_attrs = DMA_ATTR_WRITE_COMBINE | DMA_ATTR_NO_KERNEL_MAPPING;
>  
> -	buffer->kvaddr = dma_alloc_attrs(core->dev, buffer->buffer_size,
> +	buffer->kvaddr = dma_alloc_attrs(dev, buffer->buffer_size,
>  					 &buffer->device_addr, GFP_KERNEL, buffer->dma_attrs);

[Severity: High]
Could this lead to a Use-After-Free of the context bank device?

When a userspace application opens the video device file descriptor,
iris_create_internal_buffer() allocates internal buffers using the context
bank device. If the driver is subsequently unbound (e.g., via sysfs) and the
context bank devices are unregistered and freed, the application could later
close the file descriptor.

Should a reference be taken on the context bank device pointer via
get_device() to ensure it outlives deferred file operations cleanup?

>  	if (!buffer->kvaddr) {
>  		kfree(buffer);

[ ... ]

> @@ -682,10 +682,10 @@ int iris_queue_internal_buffers(struct iris_inst *inst, u32 plane)
>  
>  int 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);

[Severity: High]
This is a pre-existing issue, but is it possible for inst->core to be freed
before this deferred cleanup is called?

In drivers/media/platform/qcom/iris/iris_probe.c, the core structure is
allocated via devm_kzalloc(). If the driver is unbound while a file
descriptor is open, devres will free the iris_core structure. When the file
descriptor is later closed, deferred release paths (such as
iris_check_num_queued_internal_buffers() in iris_vidc.c, or here calling
iris_get_cb_dev()) will dereference inst->core, resulting in a Use-After-Free.

>  
>  	list_del(&buffer->list);
> -	dma_free_attrs(core->dev, buffer->buffer_size, buffer->kvaddr,
> +	dma_free_attrs(dev, buffer->buffer_size, buffer->kvaddr,
>  		       buffer->device_addr, buffer->dma_attrs);

[Severity: High]
If the context bank device was freed during driver unbind, will passing the
unpinned device pointer to dma_free_attrs() during deferred fops release
cause a Use-After-Free?

>  	kfree(buffer);
>  

> diff --git a/drivers/media/platform/qcom/iris/iris_vidc.c b/drivers/media/platform/qcom/iris/iris_vidc.c
> index 14d63dc76c9ba..26df872fb4fa1 100644
> --- a/drivers/media/platform/qcom/iris/iris_vidc.c
> +++ b/drivers/media/platform/qcom/iris/iris_vidc.c
> @@ -107,7 +107,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);
>  	src_vq->lock = &inst->ctx_q_lock;
>  	ret = vb2_queue_init(src_vq);
>  	if (ret)
> @@ -121,7 +121,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);
>  	dst_vq->lock = &inst->ctx_q_lock;

[Severity: High]
Does assigning the dynamically created context bank device to the vb2_queue
without taking a reference create a Use-After-Free risk?

If the driver unbinds and the device is freed, the vb2_queue could still be
accessed during deferred cleanup when the file descriptor is finally closed.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260715-glymur-v9-0-8cf2cbe12a07@oss.qualcomm.com?part=4