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

[Sumit Garg <sumit.garg@kernel.org>]

On Tue, May 19, 2026 at 07:34:07AM +0000, sashiko-bot@kernel.org wrote:
> 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?

Not in scope of the current patch-set.

> 
> 
> 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?

The PAS backend isn't expected to unregister during the boot power
cycle.

-Sumit

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