Re: [PATCH v8 01/18] media: iris: Add Gen2 firmware autodetect and fallback

[sashiko-bot@kernel.org]

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

New issues:
- [High] Uninitialized memory scan in firmware generation detection.

Pre-existing issues:
- [High] Kernel crash: IRQ is enabled before the HFI ops are initialized, leading to a NULL pointer dereference on early interrupts.
- [High] Use-After-Free of `iris_core` due to uncancelled delayed work during module teardown.
- [High] Race condition in `iris_open` allows concurrent callers to bypass the hardware initialization wait.
--

--- Patch [1]: [PATCH v8 01/18] media: iris: Add Gen2 firmware autodetect and fallback ---
commit a2c8d521a685de60d5919ebd8f396f7a268a25cc
Author: Dikshita Agarwal <dikshita.agarwal@oss.qualcomm.com>

media: iris: Add Gen2 firmware autodetect and fallback

Some Iris platforms support both Gen1 and Gen2 HFI firmware images.
Update the firmware loading logic to handle this generically by
preferring Gen2 when available, while safely falling back to Gen1
when required.

[ ... ]

> diff --git a/drivers/media/platform/qcom/iris/iris_core.c b/drivers/media/platform/qcom/iris/iris_core.c
> index 52bf56e517f91..6dbe18be5b495 100644
> --- a/drivers/media/platform/qcom/iris/iris_core.c
> +++ b/drivers/media/platform/qcom/iris/iris_core.c
> @@ -79,6 +80,7 @@ int iris_core_init(struct iris_core *core)
>  		goto error_unload_fw;
>  
>  	core->iris_firmware_data->init_hfi_ops(core);

[Severity: High]
This isn't a bug introduced by this patch, but could this initialization
sequence cause a NULL pointer dereference on early interrupts?

In iris_core_init(), iris_vpu_power_on() is called earlier, which unmasks the
interrupt via enable_irq(core->irq). At that point, core->hfi_sys_ops is still
NULL because init_hfi_ops() hasn't run yet.

If an interrupt fires early (for example, from the VPU firmware booting),
iris_hfi_isr_handler() will execute and unconditionally dereference
core->hfi_sys_ops->sys_hfi_response_handler. Should init_hfi_ops() be called
before enabling the IRQ?

[Severity: High]
This is a pre-existing issue, but does this function have a race condition that
allows concurrent callers to bypass the hardware initialization wait?

When a user opens the device, iris_core_init() acquires the lock, sets
core->state = IRIS_CORE_INIT, drops the lock, and then proceeds with the
lengthy initialization and iris_wait_for_system_response().

If a concurrent thread calls iris_open() and acquires the lock, it will see
core->state == IRIS_CORE_INIT, immediately return 0, and proceed to interact
with the hardware before the firmware has actually finished booting. Could this
lead to undefined behavior?

> +	iris_session_init_caps(core);
>  
>  	ret = iris_hfi_core_init(core);
>  	if (ret)

[ ... ]

> diff --git a/drivers/media/platform/qcom/iris/iris_firmware.c b/drivers/media/platform/qcom/iris/iris_firmware.c
> index 1a476146d7580..c5d2b9fd0502e 100644
> --- a/drivers/media/platform/qcom/iris/iris_firmware.c
> +++ b/drivers/media/platform/qcom/iris/iris_firmware.c

[ ... ]

> @@ -56,6 +209,9 @@ static int iris_load_fw_to_memory(struct iris_core *core, const char *fw_name)
>  	ret = qcom_mdt_load(dev, firmware, fw_name,
>  			    IRIS_PAS_ID, mem_virt, mem_phys, res_size, NULL);
>  
> +	if (!ret)
> +		iris_detect_firmware(core, fw_name, mem_virt, res_size, desc);

[Severity: High]
Could this scan uninitialized memory?

The memory region is mapped via memremap() using MEMREMAP_WC without being
zeroed, meaning it could retain contents from a previous boot or firmware load.
qcom_mdt_load() populates the memory but might leave gaps or leave the tail
end untouched.

Since iris_detect_firmware() is called with the total reserved memory size
(res_size) rather than the actual loaded firmware size, could the loop in
iris_detect_gen2_from_fwdata() scan past the valid loaded data and falsely
detect a leftover QC_IMAGE_VERSION_STRING from an older firmware?

> +
>  	memunmap(mem_virt);
>  err_release_fw:
>  	release_firmware(firmware);

[ ... ]

> diff --git a/drivers/media/platform/qcom/iris/iris_probe.c b/drivers/media/platform/qcom/iris/iris_probe.c
> index c2dcb50a27824..7fe31136df21b 100644
> --- a/drivers/media/platform/qcom/iris/iris_probe.c
> +++ b/drivers/media/platform/qcom/iris/iris_probe.c
> @@ -251,8 +251,6 @@ static int iris_probe(struct platform_device *pdev)
>  		return core->irq;

[Severity: High]
This is a pre-existing issue, but does the module teardown path leak a running
delayed work, leading to a use-after-free?

iris_probe() initializes a delayed work sys_error_handler which uses the core
pointer. In the teardown path, iris_remove() unregisters devices but does not
seem to call cancel_delayed_work_sync(&core->sys_error_handler).

Since the core struct is managed by devm_kzalloc, it is freed when
iris_remove() returns. If the delayed work was scheduled and runs after
removal, won't it dereference the freed core pointer?

>  	core->iris_platform_data = of_device_get_match_data(core->dev);
> -	core->iris_firmware_desc = core->iris_platform_data->firmware_desc;
> -	core->iris_firmware_data = core->iris_firmware_desc->firmware_data;
>  
>  	core->ubwc_cfg = qcom_ubwc_config_get_data();

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260730-iris-ar50lt-v8-0-6597c08cea6f@oss.qualcomm.com?part=1