Re: [PATCH v6 12/16] media: qcom: Pass proper PAS ID to set_remote_state API

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Missing cancellation of the `sys_error_handler` delayed work in `iris_remove` leads to Use-After-Free and locking a destroyed mutex.
- [High] Missing memory barrier (`smp_load_acquire` or `READ_ONCE`) when reading `ops_ptr` in PAS API wrappers creates a TOCTOU race and potential NULL pointer dereference or uninitialized memory access.
--

commit dc3a6dd9cf1f9278b9e5d984b574b26cf894a4e9
Author: Sumit Garg <sumit.garg@oss.qualcomm.com>

media: qcom: Pass proper PAS ID to set_remote_state API

This commit modifies the set_hw_state functions for both iris and venus
drivers to pass a specific PAS ID instead of 0 to qcom_pas_set_remote_state.
This change allows the OP-TEE backend to utilize the PAS ID for proper
bookkeeping.

> diff --git a/drivers/media/platform/qcom/iris/iris_firmware.c b/drivers/media/platform/qcom/iris/iris_firmware.c
> index b3c5281aea919..856fa6a79064c 100644
> --- a/drivers/media/platform/qcom/iris/iris_firmware.c
> +++ b/drivers/media/platform/qcom/iris/iris_firmware.c
> @@ -109,5 +109,5 @@ int iris_fw_unload(struct iris_core *core)
>  
>  int iris_set_hw_state(struct iris_core *core, bool resume)
>  {
> -	return qcom_pas_set_remote_state(resume, 0);
> +	return qcom_pas_set_remote_state(resume, core->iris_platform_data->pas_id);

This isn't a bug in this commit, but since we are modifying the state
management for iris_core, there is a pre-existing issue with the
sys_error_handler delayed work.

Looking at iris_remove(), we explicitly destroy the core->lock and allow
the core memory to be freed via devres:

drivers/media/platform/qcom/iris/iris_probe.c:iris_remove() {
	...
	mutex_destroy(&core->lock);
}

Could the delayed work execute after the module is unloaded or the device
unbinds? If so, would iris_sys_error_handler() attempt to acquire the
destroyed core->lock and access the freed core memory?

Should we add cancel_delayed_work_sync(&core->sys_error_handler) before
destroying the lock?


This isn't a bug in this commit either, but since this function calls
qcom_pas_set_remote_state(), I noticed a potential race condition in how
the global ops_ptr is read in the API wrapper.

Looking at qcom_pas_set_remote_state(), it performs lockless plain reads:

drivers/firmware/qcom/qcom_pas.c:qcom_pas_set_remote_state() {
	if (!ops_ptr)
		return -ENODEV;

	return ops_ptr->set_remote_state(ops_ptr->dev, state, pas_id);
}

Because ops_ptr is read without READ_ONCE() or smp_load_acquire(), could
the compiler reload the pointer?

If qcom_pas_ops_unregister() clears ops_ptr concurrently, could the second
read evaluate to NULL, causing a NULL pointer dereference? Furthermore,
without a barrier, could the CPU reorder reads and observe uninitialized
contents of the qcom_pas_ops structure?

>  }

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260518072856.22790-1-sumit.garg@kernel.org?part=12