Re: [PATCH v3 4/6] Bluetooth: Introduce Qualcomm IPQ5018 IPC based HCI driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 16 potential issue(s) to consider:
- [High] Data race in `btqcomipc_send` corrupts shared ring buffer state.
- [High] Invalid pointer cast of 32-bit DSP value causes kernel panic.
- [High] Missing and incorrect endianness conversions break functionality on big-endian architectures.
- [High] Unsafe firmware segment name formatting is vulnerable to buffer underflow and overflow.
- [High] The RX worker inadvertently corrupts the ring buffer size configuration.
- [High] Missing length bounds check on received short messages.
- [High] Spurious IRQ can trigger a NULL pointer dereference of `desc->hdev`.
- [High] Integer overflow in firmware segment address validation allows out-of-bounds write.
- [High] Incorrect teardown order in `btqcomipc_remove` leaks TX packets.
- [High] Missing memory barriers and improper I/O memory handling.
- [Medium] The workqueue is leaked if IRQ registration fails.
- [Medium] Resource leak on `btqcomipc_open` timeout.
- [Medium] Long message slots are leaked when the short TX ring is full.
- [Medium] Reopen state corruption due to missing flag reset in `btqcomipc_close`.
- [Medium] Missing structural validation of ELF file size.
- [Medium] Memory allocation failure in RX path causes permanent ring stall.
--

commit e292b0c9ddcf5a4d994ac5c30c3559ed0e18bb8d
Author: George Moussalem <george.moussalem@outlook.com>

Bluetooth: Introduce Qualcomm IPQ5018 IPC based HCI driver

Add support for the Bluetooth controller found in the IPQ5018 SoC.
This driver implements firmware loading and the transport layer between
the HCI core and the Bluetooth controller.

> diff --git a/drivers/bluetooth/btqcomipc.c b/drivers/bluetooth/btqcomipc.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/bluetooth/btqcomipc.c
[ ... ]
> +#define IPC_LBUF_SZ(w, x, y, z)	(((TO_BT_ADDR((void *)w) + w->x) - w->y) / w->z)

[Severity: High]
Does this macro perform native arithmetic on little-endian fields?

When used with desc->tx_ctxt, fields like total_size and lring_buf are __le16
and __le32. Without endianness conversion, this could produce incorrect
results on big-endian architectures.

[ ... ]
> +	if (len > IPC_MSG_PLD_SZ) {
> +		hdr |= IPC_HDR_LONG_MSG;
> +
> +		ptr_buf = btss_alloc_lmsg(desc, len,
> +					  &aux_ptr, &is_lbuf_full);
> +		if (IS_ERR(ptr_buf)) {
> +			bt_dev_err(hdev, "TX long buffers full");
> +			hdev->stat.err_tx++;
> +			return PTR_ERR(ptr_buf);
> +		}
> +	}
> +
> +	rinfo = btss_get_tx_rbuf(desc, &is_sbuf_full);
> +	if (IS_ERR(rinfo)) {
> +		bt_dev_err(hdev, "TX short buffers full");
> +		hdev->stat.err_tx++;
> +		return PTR_ERR(rinfo);
> +	}

[Severity: Medium]
What happens to the allocated long message buffer if the short message ring
is full?

If btss_alloc_lmsg() succeeds but btss_get_tx_rbuf() subsequently fails, the
function returns an error without undoing the long message reservation. Could
this permanently leak long message slots under heavy traffic?

[ ... ]
> +	rbuf->msg_hdr = cpu_to_le16(hdr);
> +	rbuf->len = cpu_to_le16(len);
> +
> +	rinfo->widx = (rinfo->widx + 1) % desc->tx_ctxt->smsg_buf_cnt;

[Severity: High]
Does this index update require a memory barrier?

The payload is written to the shared memory ring buffer, and then the widx
is updated. Without a dma_wmb(), is it possible for the CPU to reorder the
index update before the payload writes are fully visible to the firmware?

[ ... ]
> +	case HCI_COMMAND_PKT: {
> +		if (len < 1 + HCI_COMMAND_HDR_SIZE)
> +			return -EILSEQ;
> +		struct hci_command_hdr *hdr = (struct hci_command_hdr *)(data + 1);
> +
> +		pkt_len = HCI_COMMAND_HDR_SIZE + le16_to_cpu(hdr->plen);
> +		break;
> +	}

[Severity: High]
Is it safe to use le16_to_cpu() on the plen field?

The plen field in struct hci_command_hdr is an 8-bit unsigned integer. Calling
le16_to_cpu() on it will zero-extend the value and byte-swap it on big-endian
architectures, effectively shifting the length left by 8 bits and causing
large over-allocations.

[ ... ]
> +		case IPC_HDR_PKT_TYPE_HCI:
> +			buf = kmalloc(rbuf->len, GFP_ATOMIC);
> +			if (!buf) {
> +				rinfo->ridx = ridx;
> +				return -ENOMEM;
> +			}

[Severity: Medium]
Will this memory allocation failure permanently stall the RX queue?

If kmalloc() fails, the function returns -ENOMEM without advancing the ring
index. The next time the worker runs, it will attempt to process the exact
same packet. Unless the packet is dropped and the index advanced, this could
permanently block the ring.

> +
> +			memcpy_fromio(buf, rxbuf, rbuf->len - aux_ptr.len);

[Severity: High]
Does this code safely handle potentially large payload lengths from firmware?

If the packet is a short message, rxbuf points to the inline 40-byte smsg_data
buffer. The driver uses the unvalidated rbuf->len for the copy length. Could a
malfunctioning firmware provide a length greater than 40, leading to an
out-of-bounds read from shared memory?

[ ... ]
> +	for (rinfo = &(desc->rx_ctxt->sring_buf_info);
> +	     rinfo != NULL;
> +	     rinfo = (struct ring_buffer_info *)(uintptr_t)(rinfo->next)) {

[Severity: High]
Does this properly convert the next pointer from the firmware?

The next field in struct ring_buffer_info is a little-endian offset or address
value. Casting it directly to uintptr_t without address translation or endian
conversion could cause a kernel panic due to an invalid page fault when
dereferenced on the next loop iteration.

> +		ret = btss_process_rx(desc, rinfo, &ack,
> +				      &desc->rx_ctxt->smsg_buf_cnt);

[Severity: High]
Are we inadvertently overwriting the ring buffer configuration here?

The fourth argument to btss_process_rx() is treated as an output counter and
incremented for every processed packet. However, we are passing the address of
the firmware's fixed smsg_buf_cnt configuration parameter. Incrementing this
will break the modulo arithmetic used for ring buffer indexing.

[ ... ]
> +	for (i = 0; i < ehdr->e_phnum; i++) {

[Severity: Medium]
Does this loop verify that the program headers exist within the firmware image?

The loop iterates based on the untrusted e_phnum field from the ELF header.
Could a malformed firmware file specify a high number of program headers,
causing out-of-bounds reads if fw->size is not properly validated?

[ ... ]
> +		if (phdr->p_vaddr + phdr->p_filesz > desc->mem_size) {
> +			dev_err(desc->dev,
> +				"Segment data exceeds the reserved memory area!\n");
> +			goto release_fw;
> +		}

[Severity: High]
Can this bounds check be bypassed by an integer overflow?

The check uses 32-bit arithmetic based on untrusted ELF header fields.
If p_vaddr and p_filesz sum to a value that exceeds 32 bits, they could wrap
around to a small result and pass this check, leading to an out-of-bounds
write when copying the segment data.

[ ... ]
> +		/* 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]
Is this string formatting safe from buffer underflows and overflows?

If the original firmware name is less than 3 characters long, strlen - 3 will
underflow to a huge value, writing out of bounds. Additionally, if the segment
index i is 100 or greater, the format string writes 5 bytes into a location
where only 4 bytes were expected, overflowing the kstrdup allocation.

[ ... ]
> +	desc->wq = create_singlethread_workqueue("btss_wq");
> +	if (!desc->wq) {
> +		dev_err(dev, "Failed to initialize workqueue\n");
> +		return -EAGAIN;
> +	}
> +
> +	INIT_WORK(&desc->work, btss_worker);
> +
> +	ret = devm_request_threaded_irq(dev, desc->irq, NULL, btss_irq_handler,
> +					IRQF_TRIGGER_RISING | IRQF_ONESHOT,
> +					"btss_irq", desc);
> +
> +	if (ret)
> +		dev_err(dev, "error registering irq[%d] ret = %d\n",
> +			desc->irq, ret);
> +
> +	return ret;

[Severity: Medium]
Does this error path leak the newly created workqueue?

If devm_request_threaded_irq() fails, the function returns immediately. The
workqueue created by create_singlethread_workqueue() is never destroyed,
leading to a memory leak.

[ ... ]
> +	msleep(BTSS_PWR_CTRL_DELAY_MS);
> +	ret = wait_event_timeout(desc->wait_q, READ_ONCE(desc->running),
> +				 msecs_to_jiffies(1000));
> +
> +	if (!ret) {
> +		bt_dev_err(hdev, "Timeout waiting for BTSS start");
> +		return -ETIMEDOUT;
> +	}

[Severity: Medium]
Should we shut down the peripheral on this timeout path?

If the wait times out, the function returns -ETIMEDOUT. Because the open
operation failed, the networking core will not call btqcomipc_close(), which
abandons the DSP in a powered-on state without calling qcom_scm_pas_shutdown().

[ ... ]
> +static int btqcomipc_close(struct hci_dev *hdev)
> +{
> +	int ret;
> +
> +	ret = qcom_scm_pas_shutdown(BTSS_PAS_ID);
> +	if (ret) {
> +		bt_dev_err(hdev, "Failed to stop firmware: %d", ret);
> +		return ret;
> +	}
> +
> +	msleep(BTSS_PWR_CTRL_DELAY_MS);
> +
> +	return 0;
> +}

[Severity: Medium]
Should desc->running be reset to false when closing the device?

The shutdown process stops the DSP, but does not clear the desc->running flag.
If the interface is brought up again, the next btqcomipc_open() might bypass
its wait_event_timeout() check because the flag is still true from the
previous session.

[ ... ]
> +	ret = btqcomipc_init(desc);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to initialize\n");
> +
> +	hdev = hci_alloc_dev();

[Severity: High]
Does enabling the IRQ before the HCI device is allocated expose a race condition?

btqcomipc_init() requests the threaded IRQ, which enables it immediately. If a
spurious interrupt fires before hdev is allocated and assigned to desc->hdev,
the worker thread could trigger a NULL pointer dereference when accessing
desc->hdev for logging or processing.

[ ... ]
> +static int btqcomipc_send(struct hci_dev *hdev, struct sk_buff *skb)
> +{
> +	u16 hdr = FIELD_PREP(IPC_HDR_PKT_TYPE_MASK, IPC_HDR_PKT_TYPE_HCI);
> +	struct qcom_btss *desc = hci_get_drvdata(hdev);
> +	int ret;
> +
> +	if (unlikely(!READ_ONCE(desc->running))) {
> +		bt_dev_err(hdev,
> +			   "BTSS not initialized, failed to send message");
> +		ret = -ENODEV;
> +		goto free_skb;
> +	}
> +
> +	ret = btss_send(desc, hdr, skb);

[Severity: High]
Is there a data race when accessing the TX ring buffer?

This entry point from the HCI stack calls btss_send() without acquiring
desc->lock. Concurrently, btss_worker() can also call btss_send() with
desc->lock held. Executing both paths simultaneously could corrupt the
ring buffer indices and free space counters.

[ ... ]
> +static void btqcomipc_remove(struct platform_device *pdev)
> +{
> +	struct qcom_btss *desc = platform_get_drvdata(pdev);
> +
> +	if (!desc)
> +		return;
> +
> +	btqcomipc_deinit(desc);
> +
> +	if (desc->hdev) {
> +		hci_unregister_dev(desc->hdev);
> +		hci_free_dev(desc->hdev);
> +	}
> +}

[Severity: High]
Could the teardown order lead to use-after-free or leaked packets?

btqcomipc_deinit() purges the TX queue and destroys the workqueue. If this
happens before hci_unregister_dev() removes the device from the networking
core, the core could still call hdev->send(), which would place packets into
the destroyed TX queue or attempt to schedule the destroyed workqueue.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260703-ipq5018-bluetooth-v3-0-62da72818ab3@outlook.com?part=4