Re: [PATCH v6 07/16] soc: qcom: mdtloader: Switch to generic PAS TZ APIs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] Heap buffer overflow and out-of-bounds read in `qcom_mdt_read_metadata()` due to unvalidated ELF header sizes.
- [High] TOCTOU race condition and Use-After-Free (UAF) due to lockless module access in the generic PAS API.
- [High] Integer overflow in `__qcom_mdt_pas_init()` memory range calculation allows TrustZone protection bypass.
- [Medium] The Kconfig dependency for QCOM_MDT_LOADER statically forces the SCM backend to be built, defeating the architectural decoupling.
--

commit 0eade83fd1eec1ecc430d6891b709c0f53c865ba
Author: Sumit Garg <sumit.garg@oss.qualcomm.com>
Subject: soc: qcom: mdtloader: Switch to generic PAS TZ APIs

This commit switches the mdtloader client driver to use generic PAS TZ APIs,
abstracting away the specific SCM backend. This allows support for multiple
TrustZone implementations such as QTEE and OP-TEE.

> Switch mdtloader client driver over to generic PAS TZ APIs. Generic PAS
> TZ service allows to support multiple TZ implementation backends like QTEE
> based SCM PAS service, OP-TEE based PAS service and any further future TZ
> backend service.

Does the Kconfig entry for QCOM_MDT_LOADER still statically select
QCOM_SCM?

Retaining the hardcoded SCM selection might prevent compiling a kernel
configured exclusively with the OP-TEE PAS backend. Should this be updated
to select QCOM_PAS instead, to match the goal of decoupling the client
driver from the specific SCM backend?

> diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c
> index c004d444d6985..137992456b712 100644
> --- a/drivers/soc/qcom/mdt_loader.c
> +++ b/drivers/soc/qcom/mdt_loader.c

[ ... ]

> @@ -271,7 +271,7 @@ static int __qcom_mdt_pas_init(struct device *dev, const struct firmware *fw,
>  		goto out;
>  	}
>  
> -	ret = qcom_scm_pas_init_image(pas_id, metadata, metadata_len, ctx);
> +	ret = qcom_pas_init_image(pas_id, metadata, metadata_len, ctx);

Is it possible for this to trigger a Use-After-Free or NULL pointer
dereference?

Looking at qcom_pas_init_image() in drivers/firmware/qcom/qcom_pas.c, it
checks if ops_ptr is NULL and then dereferences ops_ptr->init_image():

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

        return ops_ptr->init_image(ops_ptr->dev, pas_id, metadata, size, ctx);
}

Since there appears to be no locking, RCU, or module refcounting, could the
backend module be unregistered and unloaded concurrently? This might set
ops_ptr to NULL between the check and the dereference, or unload the module
while its operations are executing.

Is there also a missing bounds check when allocating metadata in
qcom_mdt_read_metadata() before it is passed here?

If ehdr_size and hash_size are derived directly from unauthenticated 32-bit
ELF program headers (phdrs[0].p_filesz) without validating they are within
the loaded firmware file size (fw->size), an attacker could supply an
enormous p_filesz.

This might wrap around during the kmalloc calculation to allocate a tiny
heap buffer, followed by an out-of-bounds read and massive heap buffer
overflow when copying the data.

>  	kfree(metadata);
>  	if (ret) {
>  		/* Invalid firmware metadata */
> @@ -280,7 +280,7 @@ static int __qcom_mdt_pas_init(struct device *dev, const struct firmware *fw,
>  	}
>  
>  	if (relocate) {
> -		ret = qcom_scm_pas_mem_setup(pas_id, mem_phys, max_addr - min_addr);
> +		ret = qcom_pas_mem_setup(pas_id, mem_phys, max_addr - min_addr);

Does the calculation of max_addr and min_addr used here have an integer
overflow vulnerability?

In the loop earlier in __qcom_mdt_pas_init(), phdr->p_paddr + phdr->p_memsz
evaluates using 32-bit arithmetic since both fields are read from
Elf32_Phdr. If a maliciously crafted firmware triggers a wrap-around (for
example, 0xFFFFF000 + 0x2000 = 0x1000), max_addr could be incorrectly
calculated as a small value while min_addr remains large.

The resulting max_addr - min_addr passed to qcom_pas_mem_setup() would then
underflow. Could this trick TrustZone into applying memory protections to
only a tiny fraction of the firmware, bypassing secure boot protections?

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