Re: [PATCH v10 05/14] media: iris: Skip DMA mask setting to core device when IOMMU is not mapped

On 7/26/2026 11:35 AM, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
>
> New issues:
> - [High] Incorrect DMA mask configuration on legacy platforms when IOMMU is missing or disabled, leading to DMA allocations outside hardware limits.
>
> Pre-existing issues:
> - [High] Use-After-Free in delayed work `sys_error_handler` due to missing cancellation during device removal.
> - [High] Use-After-Free and locking freed memory in V4L2 file operations due to `core` context being tied to `devres` instead of being reference-counted.
> - [High] Race condition exposing video device nodes before drvdata and critical setup are complete, leading to NULL pointer dereferences or Use-After-Free.
> --
>
> --- Patch [5]: [PATCH v10 05/14] media: iris: Skip DMA mask setting to core device when IOMMU is not mapped ---
> commit 618ea537a61c437b82cec491d04d1126e0ba8f26
> Author: Vikash Garodia <[email protected]>
> Subject: media: iris: Skip DMA mask setting to core device when IOMMU is not mapped
>
> This patch modifies the probe function to only set the DMA mask on the core
> device if an IOMMU is mapped. It aims to avoid unnecessary DMA mask setup when
> IOMMU streams are handled by subnodes rather than the parent device.
>
>> diff --git a/drivers/media/platform/qcom/iris/iris_probe.c b/drivers/media/platform/qcom/iris/iris_probe.c
>> index 2594f5a67378a..c40c4ec44e83e 100644
>> --- a/drivers/media/platform/qcom/iris/iris_probe.c
>> +++ b/drivers/media/platform/qcom/iris/iris_probe.c
>> @@ -310,9 +310,11 @@ static int iris_probe(struct platform_device *pdev)
>>  
>>  	dma_mask = core->iris_platform_data->dma_mask;
>>  
>> -	ret = dma_set_mask_and_coherent(dev, dma_mask);
>> -	if (ret)
>> -		goto err_vdev_unreg_enc;
>> +	if (device_iommu_mapped(dev)) {
>> +		ret = dma_set_mask_and_coherent(dev, dma_mask);
>> +		if (ret)
>> +			goto err_vdev_unreg_enc;
>> +	}
> [Severity: High]
> Does this skip the DMA mask configuration on legacy platforms if the IOMMU is
> simply disabled or missing (e.g., via the iommu=off kernel parameter)?
>
> If device_iommu_mapped() is false, the required hardware DMA mask is skipped.
> However, on legacy platforms, core->np_dev is NULL, so core->dev is still
> used as a fallback for DMA allocations:
>
> drivers/media/platform/qcom/iris/iris_hfi_queue.c:iris_hfi_queues_init() {
>     ...
> 	struct device *dev = core->np_dev ? core->np_dev : core->dev;
> 	...
> 	core->iface_q_table_vaddr = dma_alloc_attrs(dev, queue_size,
>     ...
> }

On valid DT, either subnodes with iommus or a top-level iommus property
will be present. If neither exists, the DT itself is invalid per the
schema — that is a DT error, not a driver issue.

> Without the mask being set, dma_alloc_attrs() will use the platform's default
> 32-bit mask. Can this allow allocations in physical memory regions the
> hardware cannot address, leading to DMA corruption or hardware faults?
>
>>  
>>  	dma_set_max_seg_size(&pdev->dev, DMA_BIT_MASK(32));
>>  	dma_set_seg_boundary(&pdev->dev, DMA_BIT_MASK(32));
> [Severity: High]
> This isn't a bug introduced by this patch, but does iris_remove() fail to 
> cancel the sys_error_handler delayed work?
>
> drivers/media/platform/qcom/iris/iris_probe.c:iris_remove() {
> 	...
> 	iris_core_deinit(core);
>
> 	video_unregister_device(core->vdev_dec);
> 	video_unregister_device(core->vdev_enc);
>
> 	v4l2_device_unregister(&core->v4l2_dev);
>
> 	iris_deinit_cb_devs(core);
>
> 	mutex_destroy(&core->lock);
> }
>
> The driver initializes a delayed work sys_error_handler which can be scheduled
> dynamically. During device unbind, iris_remove() is called but does not
> explicitly cancel this work.
>
> Because the iris_core structure is allocated with devm_kzalloc(), it is
> automatically freed by devres when iris_remove() returns.
>
> If the delayed work fires after the device is unbound, it accesses the freed
> core structure:
>
> drivers/media/platform/qcom/iris/iris_probe.c:iris_sys_error_handler() {
> 	struct iris_core *core =
> 			container_of(work, struct iris_core, sys_error_handler.work);
>
> 	iris_core_deinit(core);
> 	...
> }
>
> Can this lead to a Use-After-Free if hardware errors race with unbind events?
>
> [Severity: High]
> This isn't a bug introduced by this patch, but is the driver context
> vulnerable to a Use-After-Free during V4L2 file release?
>
> Because core is allocated via devm_kzalloc() in probe, its lifetime is tied
> to the device bind cycle. If a userspace process holds the file descriptor
> open and the driver is unbound, iris_remove() returns and devres frees the
> core structure.
>
> When the user later closes the file, the VFS calls iris_close():
>
> drivers/media/platform/qcom/iris/iris_vidc.c:iris_close() {
>     ...
> 	iris_remove_session(inst);
>     ...
> }
>
> Which in turn accesses the lock inside the freed core structure:
>
> drivers/media/platform/qcom/iris/iris_vidc.c:iris_remove_session() {
> 	struct iris_core *core = inst->core;
> 	...
> 	mutex_lock(&core->lock);
> 	...
> }
>
> Should the core structure be reference-counted rather than tied to devres to
> prevent this from being triggered by unprivileged userspace holding an open
> file descriptor?
>
> [Severity: High]
> This isn't a bug introduced by this patch, but does
> iris_register_video_device() expose the video device node to userspace before
> drvdata is fully initialized?
>
> drivers/media/platform/qcom/iris/iris_probe.c:iris_register_video_device() {
>     ...
> 	ret = video_register_device(vdev, VFL_TYPE_VIDEO, -1);
> 	if (ret)
> 		goto err_vdev_release;
>
> 	video_set_drvdata(vdev, core);
>     ...
> }
>
> video_register_device() immediately exposes the /dev/videoX node to userspace.
> If a concurrent open() wins the race, it will read a NULL core:
>
> drivers/media/platform/qcom/iris/iris_vidc.c:iris_open() {
> 	struct iris_core *core = video_drvdata(filp);
>     ...
> 	ret = pm_runtime_resume_and_get(core->dev);
>     ...
> }
>
> Can this lead to a NULL pointer dereference?
>
> Additionally, since iris_probe() continues with fallible setup operations
> (like setting the DMA mask) after registering the video device, if probe fails
> later, a successful concurrent open() will retain a reference to the freed
> core, leading to a Use-After-Free.
>