Re: [PATCH v7 12/15] net: ipa: Switch to generic PAS TZ APIs

[sashiko-bot@kernel.org]

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

New issues:
- [High] Switching to the generic PAS TZ API introduces Time-Of-Check to Time-Of-Use (TOCTOU) races and potential NULL pointer dereferences or Use-After-Free vulnerabilities due to lack of synchronization in the generic PAS subsystem.

Pre-existing issues:
- [High] An integer overflow vulnerability in the MDT firmware segment boundary check allows a maliciously crafted firmware file to cause a heap buffer overflow.
--

commit f6c9cf680dc7b363a71ec6ed34880a3cb8222b90
Author: Sumit Garg <sumit.garg@oss.qualcomm.com>
net: ipa: Switch to generic PAS TZ APIs

This commit switches the ipa client driver over to generic PAS TZ APIs. It
allows support for multiple TZ implementation backends by replacing direct
qcom scm calls with generic pas service calls.

> diff --git a/drivers/net/ipa/ipa_main.c b/drivers/net/ipa/ipa_main.c
> --- 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);

[Severity: High]
This is a pre-existing issue, but is there an integer overflow risk in the
underlying firmware loader being called here?

When calling into the mdt loading routines, the boundary check in
qcom_mdt_load_no_init() evaluates:

if (offset < 0 || offset + phdr->p_memsz > mem_size)

On 32-bit architectures, since offset is ssize_t and phdr->p_memsz is u32,
could a maliciously crafted firmware file cause offset + phdr->p_memsz to
wrap around to 0?

This might bypass the check and cause a subsequent memset to write out of
bounds before the firmware is authenticated.

> -	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() introduce a race condition
during backend module unload?

Looking at the implementation of qcom_pas_auth_and_reset(), it checks
ops_ptr and then dereferences it:

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

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

If the backend module is unregistered concurrently, could ops_ptr become
null between the check and the use, leading to a null pointer dereference?

Is there any synchronization preventing the backend from disappearing while
it is being used?

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

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