Re: [PATCH v8 11/14] net: ipa: Switch to generic PAS TZ APIs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Switching to the generic PAS API exposes the IPA driver to a Time-of-Check to Time-of-Use (TOCTOU) race condition and Use-After-Free due to lockless access to the global `ops_ptr`.
--

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

net: ipa: Switch to generic PAS TZ APIs

This commit switches the ipa client driver to use the generic PAS TZ APIs
instead of the scm-specific implementation. This allows supporting multiple
TZ implementation backends.

> diff --git a/drivers/net/ipa/ipa_main.c b/drivers/net/ipa/ipa_main.c
> index 788dd99af2a4d..3cd9e44680e9f 100644
> --- a/drivers/net/ipa/ipa_main.c
> +++ b/drivers/net/ipa/ipa_main.c
[ ... ]
> @@ -624,10 +624,13 @@ static int ipa_firmware_load(struct device *dev)
>  	}
>  
>  	ret = qcom_mdt_load(dev, fw, path, IPA_PAS_ID, virt, phys, size, NULL);
> -	if (ret)
> +	if (ret) {
>  		dev_err(dev, "error %d loading \"%s\"\n", ret, path);
> -	else if ((ret = qcom_scm_pas_auth_and_reset(IPA_PAS_ID)))
> -		dev_err(dev, "error %d authenticating \"%s\"\n", ret, path);
> +	} else {
> +		ret = qcom_pas_auth_and_reset(IPA_PAS_ID);

[Severity: High]
Does switching to qcom_pas_auth_and_reset() expose the driver to a race
condition if the PAS backend is unbound concurrently?

Looking at the underlying qcom_pas_auth_and_reset() implementation in
drivers/firmware/qcom/qcom_pas.c, it performs a lockless check:

    if (!ops_ptr)
        return -ENODEV;

    return ops_ptr->auth_and_reset(ops_ptr->dev, pas_id);

If a privileged user unbinds the PAS backend or unloads its module after
ops_ptr is checked but before it is dereferenced, ops_ptr could be set to
NULL or the backend module memory could be freed.

Since qcom_pas.c doesn't appear to use synchronization or module reference
counting (like try_module_get()) to protect ops_ptr, could this result in
a NULL pointer dereference or a use-after-free by executing freed module text
when the dereference occurs?

> +		if (ret)
> +			dev_err(dev, "error %d authenticating \"%s\"\n", ret, path);
> +	}
>  
>  	memunmap(virt);

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