Re: [PATCH v4 05/13] media: iris: Add non-pixel and pixel context bank devices

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

Pre-existing issues:
- [High] Use-After-Free of `iris_core` structure due to devres and asynchronous V4L2 file closures.
- [High] Race condition in probe exposes video device before initialization, causing NULL dereference.
- [High] `iris_remove()` destroys `core->lock` without acquiring it, leading to a race condition with concurrent file closure.
--

commit 06944aa24efc3154b815139f80472c743bbbd5e1
Author: Vikash Garodia [off-list ref]

media: iris: Add non-pixel and pixel context bank devices

This commit adds helper functions to create and clean up "non-pixel" and
"pixel" context bank devices from device tree subnodes. It sets up the context
banks prior to video device registration to ensure proper DMA addressable
range routing, preventing SMMU page faults and spontaneous reboots.

[ ... ]

[Severity: High]
This is a pre-existing issue, but does iris_remove() allow a use-after-free
of core if a userspace process keeps the device node open?

The core structure is allocated with devm_kzalloc() in iris_probe(),
meaning it is freed automatically when the driver unbinds. However,
iris_remove() runs to completion and does not block for active file
descriptors to close.

If userspace holds the file descriptor open during an unbind event and later
closes it, iris_close() is invoked, which dereferences the freed memory:

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

 	mutex_destroy(&core->lock);

[Severity: High]
This isn't a bug introduced by this patch, but is it safe to destroy
core->lock here unconditionally?

If iris_remove() executes concurrently with a file closure, iris_close()
can acquire this lock via iris_remove_session():

drivers/media/platform/qcom/iris/iris_vidc.c:iris_remove_session() {
    mutex_lock(&core->lock);
    list_for_each_entry_safe(iter, temp, &core->instances, list) {
    ...
    mutex_unlock(&core->lock);
}

Destroying the mutex while it might be held by another thread violates locking
rules and can lead to corruption when the racing thread releases the lock.

 }
 

[ ... ]

[Severity: High]
This is a pre-existing issue, but is there a race condition here that exposes
the video device to userspace before initialization is complete?

iris_register_video_device() calls video_register_device(), making the
/dev/videoX node visible, and then sets video_set_drvdata(). If userspace
races to open the device node in between, video_drvdata() could return NULL.

Additionally, PM runtime enablement in iris_probe() happens after the device
is registered:

drivers/media/platform/qcom/iris/iris_probe.c:iris_probe() {
    ...
    pm_runtime_use_autosuspend(core->dev);
    ret = devm_pm_runtime_enable(core->dev);
    ...
}

If iris_open() is called by userspace before this completes, it attempts to
dereference core->dev (which could be NULL if video_set_drvdata() hasn't
executed yet), leading to a kernel panic:

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);
    ...
}

 	if (ret)
 		goto err_v4l2_unreg;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914-vpu_iommu_iova_handling-v4-0-9b9074a73c41@oss.qualcomm.com?part=5