Re: [PATCH v5 5/6] arm64: dts: qcom: ipq5018: add node required for Bluetooth support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Out-of-bounds write in `memset_io` during firmware ELF loading.
- [High] Unvalidated hardware/firmware inputs from shared memory lead to division-by-zero panics and out-of-bounds accesses.
- [High] Direct pointer dereferences to I/O memory without using proper I/O accessors, leading to potential alignment faults.
- [High] Incorrect teardown sequence in `btqcomipc_close` causing a race condition and unrecoverable bus faults.
--

--- Patch [5]: [PATCH v5 5/6] arm64: dts: qcom: ipq5018: add node required for Bluetooth support ---
commit 30194f675ebc660c7f509c3ca16e858f9db019e2
Author: George Moussalem <george.moussalem@outlook.com>
Subject: arm64: dts: qcom: ipq5018: add node required for Bluetooth support

This commit adds a device tree node to enable Bluetooth support on the
IPQ5018 platform. It sets up the bluetooth node with the appropriate
register space, firmware name, clocks, resets, and interrupts.

> diff --git a/arch/arm64/boot/dts/qcom/ipq5018.dtsi b/arch/arm64/boot/dts/qcom/ipq5018.dtsi
> index 6f8004a22a1ff..8c252fa3ff5bf 100644
> --- a/arch/arm64/boot/dts/qcom/ipq5018.dtsi
> +++ b/arch/arm64/boot/dts/qcom/ipq5018.dtsi
> @@ -453,6 +453,22 @@ tcsr: syscon@1937000 {
>  			reg = <0x01937000 0x21000>;
>  		};
>  
> +		bluetooth: bluetooth@7000000 {
> +			compatible = "qcom,ipq5018-bt";
> +			reg = <0x07000000 0x58000>;
> +
> +			firmware-name = "qca/bt_fw_patch.mbn";
> +
> +			clocks = <&gcc GCC_BTSS_LPO_CLK>;
> +			clock-names = "lpo";
> +			resets = <&gcc GCC_BTSS_BCR>;
> +
> +			qcom,ipc = <&apcs_glb 8 23>;
> +			interrupts = <GIC_SPI 162 IRQ_TYPE_EDGE_RISING>;
> +
> +			status = "disabled";
> +		};
> +
>  		sdhc_1: mmc@7804000 {
>  			compatible = "qcom,ipq5018-sdhci", "qcom,sdhci-msm-v5";
>  			reg = <0x7804000 0x1000>;

While reviewing the driver code associated with this hardware, a few questions
came up regarding the implementation in drivers/bluetooth/btqcomipc.c.

drivers/bluetooth/btqcomipc.c:btcomqipc_firmware_load() {
...
			if (phdr->p_memsz > phdr->p_filesz)
				memset_io(desc->base + phdr->p_vaddr +
					  phdr->p_filesz, 0,
					  phdr->p_memsz - phdr->p_filesz);
...
}

[Severity: High]
Can this lead to an out-of-bounds I/O memory write?

When the firmware ELF is parsed in btcomqipc_firmware_load(), it appears
we validate that phdr->p_filesz fits within the allocated IO memory, but
is there a similar check for phdr->p_memsz?

If a malformed firmware image provides an arbitrarily large p_memsz, could
this memset_io() write zeroes out-of-bounds past the end of the desc->base
mapping?


drivers/bluetooth/btqcomipc.c:btss_process_rx() {
...
		ridx = (ridx + 1) % rinfo->ring_buf_cnt;
		rinfo->ridx = ridx;
...
}

drivers/bluetooth/btqcomipc.c:btss_get_tx_rbuf() {
...
		idx = (rinfo->widx + 1) % (desc->tx_ctxt->smsg_buf_cnt);
...
}

[Severity: High]
Could these modulo operations cause a division-by-zero panic?

The firmware running on the Bluetooth controller populates the context_info
and ring_buffer_info structures in shared memory. If the firmware sets
rinfo->ring_buf_cnt or desc->tx_ctxt->smsg_buf_cnt to 0, could the kernel
crash here?

Additionally, are offsets like rinfo->rbuf checked against desc->mem_size
to prevent out-of-bounds I/O memory accesses?


drivers/bluetooth/btqcomipc.c:btss_process_rx() {
...
		msg_hdr = le16_to_cpu(rbuf->msg_hdr);
		msg_len = le16_to_cpu(rbuf->len);
...
}

drivers/bluetooth/btqcomipc.c:btss_send() {
...
	rbuf->msg_hdr = cpu_to_le16(hdr);
	rbuf->len = cpu_to_le16(len);
...
}

drivers/bluetooth/btqcomipc.c:btss_worker() {
...
		total_size = le16_to_cpu(desc->rx_ctxt->total_size);
...
}

[Severity: High]
Does this code perform direct pointer dereferences to I/O memory without
using proper I/O accessors?

The memory mapped via devm_platform_get_and_ioremap_resource() is Device
memory on ARM64. Since the driver drops the __iomem annotation and accesses
packed structures using direct C pointer assignments, could the compiler
emit unaligned load/store instructions?

Executing these instructions against ARM64 Device memory violates
architectural rules and could trigger an alignment fault.


drivers/bluetooth/btqcomipc.c:btqcomipc_close() {
...
	ret = qcom_scm_pas_shutdown(BTSS_PAS_ID);
	if (ret) {
		bt_dev_err(hdev, "Failed to stop firmware: %d", ret);
		return ret;
	}

	disable_irq(desc->irq);

	spin_lock_irqsave(&desc->lock, flags);
	WRITE_ONCE(desc->running, false);
...
}

[Severity: High]
Is the teardown sequence in btqcomipc_close() safe from race conditions?

The driver calls qcom_scm_pas_shutdown() to power down the Bluetooth
subsystem before disabling the interrupt, setting desc->running = false,
and flushing the workqueue.

If an interrupt fires during or immediately after shutdown, or if btss_worker
is already running concurrently, could the worker attempt to read from or
write to the BTSS memory-mapped I/O registers? Accessing a powered-off
peripheral's registers typically results in a fatal bus error.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709-ipq5018-bluetooth-v5-0-e476c41f03b8@outlook.com?part=5