Re: [RFC PATCH 06/11] media: iris: Add hooks for pixel and non-pixel context banks

[sashiko-bot@kernel.org]

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

New issues:
- [High] Unregistering context bank devices in `remove` while V4L2 FDs are open causes DMA API mismatch and IOMMU faults.
- [Medium] Hardcoded platform device ID causes name collision if multiple Iris instances exist.

Pre-existing issues:
- [High] Missing cancellation of sys_error_handler delayed work leads to Use-After-Free.
- [High] Use-After-Free on `iris_core` during deferred V4L2 close due to `devm_kzalloc`.
--

--- Patch [6]: [RFC PATCH 06/11] media: iris: Add hooks for pixel and non-pixel context banks ---
commit ac38538cd2eb8a1aa776f7262b1bd953d7ac3a78
Author: Vikash Garodia <vikash.garodia@oss.qualcomm.com>

media: iris: Add hooks for pixel and non-pixel context banks
    
Iris platforms use separate context-bank devices for the pixel and
non-pixel domains. Add platform hooks to create and destroy those
subdevices, and wire them up for the affected platforms.

> diff --git a/drivers/media/platform/qcom/iris/iris_platform_sm8550.c b/drivers/media/platform/qcom/iris/iris_platform_sm8550.c
> new file mode 100644
> index 0000000000000..bea904a9249ba
> --- /dev/null
> +++ b/drivers/media/platform/qcom/iris/iris_platform_sm8550.c
> @@ -0,0 +1,71 @@

[ ... ]

> +static int sm8550_init_cb_devs(struct iris_core *core)
> +{
> +	struct device *dev;
> +
> +	dev = iris_create_cb_dev(core, "non-pixel");

[Severity: Medium]
When creating the child context banks here, could this cause a name
collision if there are multiple Iris instances?

Looking at iris_create_cb_dev() in iris_resources.c, it zero-initializes
struct platform_device_info, which forces the device id to 0:

drivers/media/platform/qcom/iris/iris_resources.c:iris_create_cb_dev() {
	struct platform_device_info plat_dev_info = {};
	...
}

This causes platform_device_register_full() to assign exact names like
"pixel.0" and "non-pixel.0". If a second Iris instance probes, wouldn't
kobject_add() return -EEXIST? Should PLATFORM_DEVID_AUTO be used instead?

> +	if (IS_ERR(dev))
> +		return PTR_ERR(dev);
> +
> +	core->np_dev = dev;
> +
> +	dev = iris_create_cb_dev(core, "pixel");

[ ... ]

> +static void sm8550_deinit_cb_devs(struct iris_core *core)
> +{
> +	if (core->p_dev)
> +		platform_device_unregister(to_platform_device(core->p_dev));
> +	if (core->np_dev)
> +		platform_device_unregister(to_platform_device(core->np_dev));
> +
> +	core->p_dev = NULL;
> +	core->np_dev = NULL;
> +}

[Severity: High]
Does unregistering the context bank devices here cause a DMA API mismatch if
a user-space application still holds a V4L2 file descriptor open?

V4L2 file handles can outlive the driver's remove callback. If a user closes
the device after remove completes, iris_close() calls
iris_destroy_all_internal_buffers() which calls iris_get_cb_dev().

Since p_dev and np_dev are NULL, iris_get_cb_dev() falls back to core->dev:

drivers/media/platform/qcom/iris/iris_resources.c:iris_get_cb_dev() {
	...
	return dev ? dev : core->dev;
}

This means dma_free_attrs() is executed using core->dev, but the memory
was originally allocated with the context bank devices. Could this mismatch
violate the DMA API and cause IOMMU unmap faults?

[Severity: High]
This is a pre-existing issue, but does tying the iris_core struct to the
device lifecycle result in a Use-After-Free during a deferred V4L2 close?

In iris_probe(), core is allocated with devm_kzalloc():

drivers/media/platform/qcom/iris/iris_probe.c:iris_probe() {
	...
	core = devm_kzalloc(&pdev->dev, sizeof(*core), GFP_KERNEL);
	...
}

When the driver is unbound, iris_remove() returns and devres frees core.
If an application holds a file descriptor open, iris_close() is eventually
called, which dereferences inst->core to clean up sessions:

drivers/media/platform/qcom/iris/iris_vidc.c:iris_close() {
	...
	iris_remove_session(inst);
	...
}

Since core is already freed, won't this cause memory corruption?

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for the
sys_error_handler delayed work to trigger a Use-After-Free if it runs
during or after driver unbind?

The sys_error_handler is dynamically scheduled on hardware errors, but
iris_remove() does not appear to cancel it:

drivers/media/platform/qcom/iris/iris_probe.c:iris_remove() {
	struct iris_core *core;

	core = platform_get_drvdata(pdev);
	if (!core)
		return;

	iris_core_deinit(core);
}

If the unbind occurs while the work is pending or running, core will be
freed by devres after iris_remove() returns. When the workqueue executes
iris_sys_error_handler(), won't it dereference the freed core?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709-vpu_iommu_iova_handling-v1-0-72bb62cb2dfd@oss.qualcomm.com?part=6