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

[sashiko-bot@kernel.org]

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

New issues:
- [High] Firmware generation detection fails for split `.mdt` firmware files provided via DT override.
- [High] Firmware generation detection aborts on the first occurrence of the marker string, risking false Gen1 classification.
- [Low] MAX_FIRMWARE_NAME_SIZE macro is unused after the patch.

Pre-existing issues:
- [Critical] Lockless traversal of `core->instances` linked list leads to potential Use-After-Free.
- [High] Possible NULL pointer dereference of `core->hfi_sys_ops` in the interrupt handler.
- [High] Concurrent open() allows userspace to issue ioctls to uninitialized or failed firmware.
- [Medium] iris_add_session silently fails if max_session_count is reached, causing ioctl timeouts.
--

--- Patch [1]: [PATCH v7 01/18] media: iris: Add Gen2 firmware autodetect and fallback ---
commit 895d948eb718207e479cf98e9811c2b53fd39e23
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 is a pre-existing issue, but can the interrupt handler dereference a NULL
core->hfi_sys_ops here?

Looking at iris_core_init(), iris_vpu_power_on(core) is called earlier, which
enables interrupts. However, core->hfi_sys_ops is not populated until this
init_hfi_ops() call. If an interrupt fires during this window,
iris_hfi_isr_handler() will unconditionally dereference core->hfi_sys_ops and
cause a kernel panic.

[Severity: High]
This is a pre-existing issue, but can a concurrent open() cause userspace to
issue ioctls to uninitialized hardware?

In iris_core_init(), the core state is set to IRIS_CORE_INIT before waiting
for the firmware SYS_INIT_DONE response. The lock is dropped during the wait.
If a second thread concurrently calls iris_open(), it sees
core->state == IRIS_CORE_INIT, bypasses the wait, and returns success
immediately. The second thread can then issue V4L2 ioctls while the firmware
is still booting.

> +	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..a3aa41aa1e66b 100644
> --- a/drivers/media/platform/qcom/iris/iris_firmware.c
> +++ b/drivers/media/platform/qcom/iris/iris_firmware.c
> @@ -16,20 +16,138 @@
>  
>  #define MAX_FIRMWARE_NAME_SIZE	128

[Severity: Low]
Is the MAX_FIRMWARE_NAME_SIZE macro still needed? It appears to be unused
after the removal of the strlen check in this patch.

> -static int iris_load_fw_to_memory(struct iris_core *core, const char *fw_name)
> +/* Detect Gen2 firmware by scanning the blob for:
> + *   QC_IMAGE_VERSION_STRING=<version>
> + * and then checking:
> + *   - version starts with "vfw", OR
> + *   - version matches "video-firmware.N.M" with N >= 2
> + */
> +
> +static bool iris_detect_gen2_from_fwdata(const u8 *data, size_t size)
> +{
> +	static const char *marker = "QC_IMAGE_VERSION_STRING=";
> +	const size_t mlen = strlen(marker);
> +	static const char *vfw = "vfw";
> +	const size_t vfwlen = strlen(vfw);
> +	static const char *vf = "video-firmware.";
> +	const size_t vflen = strlen(vf);
> +
> +	for (size_t i = 0; i + mlen < size; i++) {
> +		const char *found;
> +
> +		if (memcmp(data + i, marker, mlen))
> +			continue;
> +
> +		found = data + i + mlen;
> +		size -= i + mlen;
> +
> +		/* vfw => Gen2 */
> +		if (size > vfwlen && !memcmp(found, vfw, vfwlen))
> +			return true;
> +
> +		if (size < vflen ||
> +		    memcmp(found, vf, vflen))
> +			return false;

[Severity: High]
Will this prematurely abort the search if the marker string is encountered but
the prefix doesn't match?

If a debug or format string matches "QC_IMAGE_VERSION_STRING=" earlier in the
binary (e.g., "Error parsing QC_IMAGE_VERSION_STRING=%s"), this function
returns false immediately instead of continuing to scan, missing the actual
version string. Should it continue searching instead?

> +
> +		found += vflen;
> +		size -= vflen;

[ ... ]

> +	if (ret) {
> +		/* No firmware-name in DT: select by probing Gen2 then Gen1. */
> +		*fw_name = desc->fwname;
> +		if (has_both_gens)
> +			ret = firmware_request_nowarn(&firmware, *fw_name, core->dev);
> +		else
> +			ret = request_firmware(&firmware, *fw_name, core->dev);
> +		if (ret && has_both_gens) {
> +			desc = core->iris_platform_data->firmware_desc_gen1;
> +			*fw_name = desc->fwname;
> +			ret = request_firmware(&firmware, *fw_name, core->dev);
> +		}
> +		if (ret)
> +			return ERR_PTR(ret);
> +	} else {
> +		/* firmware-name given: load it and detect its generation. */
> +		ret = request_firmware(&firmware, *fw_name, core->dev);
> +		if (ret)
> +			return ERR_PTR(ret);
> +
> +		if (has_both_gens &&
> +		    !iris_detect_gen2_from_fwdata((const u8 *)firmware->data, firmware->size)) {

[Severity: High]
Does this code fail to detect Gen2 firmware when a standard split .mdt file is
provided via DT override?

If request_firmware() is called on a split .mdt file, it provides only the
ELF headers. Since the actual version string resides in the .bXX data segments,
scanning the header will miss the version string and falsely assume Gen1.
Subsequently, qcom_mdt_load() will properly load the Gen2 segments into
memory, causing a mismatch between the running firmware and the driver's HFI
operations.

> +			dev_info(core->dev, "Gen1 FW detected in %s\n", *fw_name);
> +			desc = core->iris_platform_data->firmware_desc_gen1;
> +		}
> +	}

[ ... ]

> diff --git a/drivers/media/platform/qcom/iris/iris_vidc.c b/drivers/media/platform/qcom/iris/iris_vidc.c
> index 14d63dc76c9ba..33edbc5cab8f0 100644
> --- a/drivers/media/platform/qcom/iris/iris_vidc.c
> +++ b/drivers/media/platform/qcom/iris/iris_vidc.c
> @@ -9,6 +9,7 @@
>  #include <media/v4l2-mem2mem.h>
>  #include <media/videobuf2-dma-contig.h>
>  
> +#include "iris_ctrls.h"
>  #include "iris_vidc.h"
>  #include "iris_instance.h"
>  #include "iris_vdec.h"

[Severity: Medium]
This isn't a bug introduced by this patch, but does iris_add_session() silently
fail when max_session_count is reached?

When userspace opens more instances than max_session_count, iris_add_session()
skips adding the instance but returns void. iris_open() continues as if
successful and provides an FD to userspace. Subsequent ioctls will timeout
because the responses are dropped since the instance is missing from
core->instances.

[Severity: Critical]
This is a pre-existing issue, but is it safe to traverse core->instances
without holding core->lock in iris_check_session_supported()?

iris_check_session_supported() uses list_for_each_entry() to traverse the
core->instances list holding only inst->lock. Concurrently, another thread
executing iris_close() can call iris_remove_session() (which holds
core->lock), remove a different instance from the list via list_del_init(),
and free it. The lockless traversal will then follow a freed pointer,
leading to a use-after-free.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709-iris-ar50lt-v7-0-76af9dd4d1f6@oss.qualcomm.com?part=1