Re: [PATCH 2/6] remoteproc: qcom: Add M0 BTSS secure PIL driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider:
- [High] Redundant request_firmware call leaks newly allocated firmware and metadata on success path.
- [High] Direct return on kstrdup failure leaks previously acquired metadata and firmware objects.
- [High] Heap buffer underflow and overflow when formatting split segment filenames with sprintf.
- [High] 32-bit integer overflow in ELF segment bounds validation allows out-of-bounds memory writes.
- [High] Missing size validation for split firmware segments leads to out-of-bounds reads.
- [High] Out-of-bounds read due to unvalidated ELF headers and hardcoded program header offset.
- [High] Time-of-Check to Time-of-Use (TOCTOU) vulnerability due to parsing ELF headers from a different firmware copy than the one authenticated.
- [Medium] Subsystem guide violation: Mixing goto-based error handling with scope-based __free() cleanup.
- [Medium] Failure to zero-initialize the BSS section of ELF segments exposes stale memory to the peripheral.
- [Medium] Missing reset assertion on late probe failures and driver removal leads to resource leaks and state mismatch.
--

commit a3d2212c4a13a5646a7fb048e4d391fc0784f11e
Author: George Moussalem <george.moussalem@outlook.com>
Subject: remoteproc: qcom: Add M0 BTSS secure PIL driver

This commit introduces a new Peripheral Image Loader driver for the M0
bluetooth subsystem found in the IPQ5018 SoC. The driver brings up the
M0 core by loading signed firmware, authenticating it via TrustZone,
and starting the peripheral using a Secure Channel Manager call.

> diff --git a/drivers/remoteproc/qcom_m0_btss_pil.c b/drivers/remoteproc/qcom_m0_btss_pil.c
> new file mode 100644
> index 0000000000000..7168e270e4d46
> --- /dev/null
> +++ b/drivers/remoteproc/qcom_m0_btss_pil.c

[ ... ]

> +static int m0_btss_load(struct rproc *rproc, const struct firmware *fw)
> +{
> +	struct m0_btss *desc = rproc->priv;
> +	const struct elf32_phdr *phdrs;
> +	const struct firmware *seg_fw;
> +	const struct elf32_phdr *phdr;
> +	const struct elf32_hdr *ehdr;
> +	void __iomem *metadata;
> +	size_t metadata_size;
> +	int i, ret;
> +
> +	ehdr = (const struct elf32_hdr *)fw->data;
> +	phdrs = (const struct elf32_phdr *)(ehdr + 1);

[Severity: High]
Does this code perform an out-of-bounds read if the firmware file is smaller
than the ELF header structure? The pointer is cast and dereferenced without
validating that the provided firmware is large enough.

[Severity: High]
Is there an issue with using a hardcoded offset to locate the program headers
instead of using the standard e_phoff field from the ELF header? If the file
uses a non-standard layout, it could parse arbitrary memory as program
headers.

> +
> +	ret = request_firmware(&fw, rproc->firmware, rproc->dev.parent);

[Severity: High]
Does this code introduce a time-of-check to time-of-use vulnerability? The
ehdr and phdrs pointers are established from the original fw parameter, but
then a new fw copy is requested and authenticated by TrustZone. If the file
changes between these reads, could malicious segment locations from the first
unauthenticated read be used to subvert the loading of the authenticated data?

[Severity: High]
Is this call to request_firmware() redundantly overwriting the local fw
parameter, preventing the original structure from being accessed?

> +	if (ret) {
> +		dev_err(rproc->dev.parent, "Failed to request firmware: %d\n",
> +			ret);
> +		return ret;
> +	}
> +
> +	metadata = qcom_mdt_read_metadata(fw, &metadata_size, rproc->firmware,
> +					  rproc->dev.parent);
> +	if (IS_ERR(metadata)) {
> +		ret = PTR_ERR(metadata);
> +		dev_err(rproc->dev.parent,
> +			"Failed to read firmware metadata: %d\n", ret);
> +		goto release_fw;
> +	}
> +
> +	ret = qcom_scm_pas_init_image(BTSS_PAS_ID, metadata,
> +				      metadata_size, NULL);
> +	if (ret) {
> +		dev_err(rproc->dev.parent, "PAS init image failed: %d\n", ret);
> +		goto free_metadata;
> +	}
> +
> +	for (i = 0; i < ehdr->e_phnum; i++) {
> +		char *seg_name __free(kfree) = kstrdup(rproc->firmware,
> +						       GFP_KERNEL);

[Severity: Medium]
Is it safe to mix goto-based error handling with scope-based cleanup helpers
like __free(kfree) in the same function? The kernel cleanup subsystem
guidelines indicate that functions should either use goto-based cleanup
exclusively, or scope-based cleanup exclusively, to prevent confusing
ownership semantics.

> +		if (!seg_name)
> +			return -ENOMEM;

[Severity: High]
Does returning directly here leak the metadata buffer and the locally
requested fw structure? It appears this direct return bypasses the
free_metadata and release_fw cleanup labels.

> +
> +		phdr = &phdrs[i];
> +
> +		/* Only process valid loadable data segments */
> +		if (phdr->p_type != PT_LOAD || !phdr->p_memsz)
> +			continue;
> +
> +		if (phdr->p_vaddr + phdr->p_filesz > desc->mem_size) {

[Severity: High]
Can the addition of phdr->p_vaddr and phdr->p_filesz overflow? Since both
are read directly from the ELF header as 32-bit integers, a crafted firmware
could provide values that wrap around the 32-bit limit, passing the bounds
check against desc->mem_size and allowing out-of-bounds memory writes.

> +			dev_err(rproc->dev.parent,
> +				"Segment data exceeds the reserved memory area!\n");
> +			goto free_metadata;
> +		}
> +
> +		/* Check if firmware is split across multiple segment files */
> +		if (phdr->p_offset > fw->size ||
> +		    phdr->p_offset + phdr->p_filesz > fw->size) {
> +			sprintf(seg_name + strlen(seg_name) - 3, "b%02d", i);

[Severity: High]
Could this sprintf() call result in a heap buffer underflow or overflow?
If the provided firmware-name is fewer than 3 characters long, the pointer
arithmetic could underflow. Additionally, if the unvalidated ehdr->e_phnum
produces a large index, the formatted string could overflow the allocated
buffer.

> +			ret = request_firmware(&seg_fw, seg_name,
> +					       rproc->dev.parent);
> +			if (ret) {
> +				dev_err(rproc->dev.parent,
> +					"Could not find split segment binary: %s\n",
> +					seg_name);
> +				goto free_metadata;
> +			}
> +
> +			/*
> +			 * Use the virtual instead of the physical address as
> +			 * the offset
> +			 */
> +			memcpy_toio(desc->mem_region + phdr->p_vaddr,
> +				    seg_fw->data, phdr->p_filesz);

[Severity: High]
Does this memcpy_toio() read past the end of seg_fw->data? It uses
phdr->p_filesz from the main ELF header, but there doesn't seem to be a check
ensuring that the loaded seg_fw->size is actually at least that large.

> +
> +			release_firmware(seg_fw);
> +		} else {
> +			memcpy_toio(desc->mem_region + phdr->p_vaddr,
> +				    fw->data + phdr->p_offset, phdr->p_filesz);

[Severity: Medium]
If an ELF segment's memory size (p_memsz) exceeds its file size (p_filesz),
does the remaining memory need to be zero-initialized for the BSS section?
The driver currently only copies p_filesz bytes, which might leave stale
kernel memory exposed to the peripheral.

> +		}
> +	}
> +
> +	return 0;

[Severity: High]
Does this success path leak the dynamically allocated metadata buffer and the
firmware structure? The function returns directly without freeing them.

> +
> +free_metadata:
> +	kfree(metadata);
> +release_fw:
> +	release_firmware(fw);
> +	return ret;
> +}

[ ... ]

> +static int m0_btss_pil_probe(struct platform_device *pdev)
> +{

[ ... ]

> +	desc->btss_reset = devm_reset_control_get(dev, "btss_reset");
> +	if (IS_ERR_OR_NULL(desc->btss_reset))
> +		return dev_err_probe(dev, PTR_ERR(desc->btss_reset),
> +				     "unable to acquire btss_reset\n");
> +
> +	ret = reset_control_deassert(desc->btss_reset);

[Severity: Medium]
Is a cleanup action needed to assert the reset if probe fails later or if the
driver is unbound? The probe function deasserts desc->btss_reset but
doesn't seem to register a corresponding devm_add_action_or_reset() to handle
teardown.

> +	if (ret)
> +		return dev_err_probe(rproc->dev.parent, ret,
> +				     "Failed to deassert reset\n");
> +
> +	rproc->auto_boot = false;
> +	ret = devm_rproc_add(dev, rproc);
> +	if (ret)
> +		return ret;
> +
> +	platform_set_drvdata(pdev, rproc);
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260625-ipq5018-bluetooth-v1-0-d999be0e04f7@outlook.com?part=2