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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 12 potential issue(s) to consider:
- [Critical] Integer underflow when computing auxiliary pointer lengths causes a heap buffer overflow.
- [Critical] Unvalidated shared memory offsets allow the M0 co-processor to trigger arbitrary out-of-bounds reads/writes on the host.
- [High] Missing locking in the TX path corrupts shared IPC ring buffers and state.
- [High] Lifecycle mismanagement of the workqueue and IRQ leads to memory leaks and broken interface toggling.
- [High] Device memory offsets are cast directly to host virtual pointers in ring buffer loops.
- [High] Off-by-one errors during HCI frame length validation allow out-of-bounds memory reads.
- [High] Directly dereferencing __iomem memory via struct pointers violates safe I/O access and triggers synchronous aborts.
- [High] Permanent leak of shared long message buffer capacity on failed short buffer acquires.
- [High] Unvalidated indices from shared memory can hang the workqueue thread in an infinite loop.
- [High] Lockless IPC ring buffers omit memory barriers, risking data corruption due to reordering.
- [Medium] Native arithmetic and bitwise operations on little-endian IPC structures break big-endian compatibility.
- [Medium] Missing acquire/release memory barriers around `desc->running` flag risk NULL pointer dereferences.
--

commit b13642cec5c040da0ea77113de5feea1556c0a79
Author: George Moussalem <george.moussalem@outlook.com>
Bluetooth: Introduce Qualcomm IPQ5018 IPC based HCI driver

This commit adds support for the Qualcomm IPQ5018 Bluetooth chip, using
shared memory for transport between the host and the M0 co-processor.
It implements ring buffers for communication and uses IPC registers
and interrupts for event notifications.

> --- /dev/null
> +++ b/drivers/bluetooth/btqcomipc.c
[ ... ]
> +static struct ring_buffer_info *btss_get_tx_rbuf(struct qcom_btss *desc,
> +						 bool *is_sbuf_full)
> +{
> +	u8 idx;
> +	struct ring_buffer_info *rinfo;
> +
> +	for (rinfo = &(desc->tx_ctxt->sring_buf_info);	rinfo != NULL;
> +		rinfo = (struct ring_buffer_info *)(uintptr_t)(rinfo->next)) {

[Severity: High]
Can this direct pointer cast cause a panic? 

The firmware provides the next ring buffer via rinfo->next, which is an 
__le32 field in shared memory. 

Directly casting this 32-bit device offset to a host virtual pointer might
result in an unmapped address being dereferenced on the next loop iteration.

[ ... ]
> +static int btss_send(struct qcom_btss *desc, u16 msg_hdr,
> +		     struct sk_buff *skb)
> +{
[ ... ]
> +	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, "long msg buf 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, "short msg buf full");
> +		hdev->stat.err_tx++;
> +		return PTR_ERR(rinfo);
> +	}

[Severity: High]
Does returning early here permanently leak the long message buffer?

When transmitting a large packet, btss_alloc_lmsg() reserves space in the
long message ring. If the short ring is full and btss_get_tx_rbuf() returns
-EAGAIN, the network stack requeues the packet.

The successfully allocated long message space doesn't appear to be freed,
which could exhaust the IPC transport's capability upon retries.

[ ... ]
> +	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]
Are memory barriers required here to prevent reordering? 

The IPC protocol uses lockless ring buffers across the CPU and coprocessor.
Without a wmb() before updating rinfo->widx, the M0 processor might perceive
the incremented write index before the payload writes are fully visible.

[ ... ]
> +static inline int btss_recv_hci_frame(struct qcom_btss *desc, const u8 *data, size_t len)
> +{
[ ... ]
> +	case HCI_EVENT_PKT:
> +	{
> +		if (len < HCI_EVENT_HDR_SIZE)
> +			return -EILSEQ;

[Severity: High]
Could this size check allow out-of-bounds reads?

The len parameter includes the 1-byte packet type prefix in data[0], but
validations like this one do not account for it. If len is exactly
HCI_EVENT_HDR_SIZE (2 bytes), the check passes.

Reading hdr->plen could then access out-of-bounds memory.

[ ... ]
> +	if (pkt_len > len)
> +		return -EINVAL;

[Severity: High]
Similarly, does this permit an out-of-bounds read during the skb data copy?

If pkt_len is exactly equal to len, the subsequent call to
skb_put_data(skb, data + 1, pkt_len) will copy len bytes from a buffer tail
that only contains len - 1 bytes.

[ ... ]
> +static inline int btss_process_rx(struct qcom_btss *desc,
> +				  struct ring_buffer_info *rinfo,
> +				  bool *ack, u8 *rx_count)
> +{
[ ... ]
> +	while (ridx != rinfo->widx) {

[Severity: High]
Can this loop hang the workqueue thread? 

The termination relies on ridx matching rinfo->widx, which is read from
shared memory. If the firmware provides a corrupted widx that is greater
than or equal to ring_buf_cnt, the modulo-wrapped ridx will never match it.

This could potentially cause an infinite loop, halting IPC processing.

[ ... ]
> +		if (rbuf->msg_hdr & IPC_HDR_LONG_MSG) {

[Severity: Medium]
Does this code safely handle endianness? 

The IPC protocol structures define sizes and offsets as explicit __le16 and
__le32 fields. Performing bitwise operations like rbuf->msg_hdr &
IPC_HDR_LONG_MSG without conversion could yield reversed bytes on big-endian
architectures.

> +			rxbuf = TO_APPS_ADDR(rbuf->payload.lmsg_data);

[Severity: Critical]
Does this translation of the payload offset trust the M0 firmware too much?

TO_APPS_ADDR() translates the untrusted 32-bit value into a direct host
pointer. If the coprocessor supplies a maliciously large offset, the pointer
could aim outside the ioremapped mem_region.

This could allow arbitrary out-of-bounds reads.

[ ... ]
> +				blks_consumed = desc->rx_ctxt->lmsg_buf_cnt -
> +					lbuf_idx;
> +				aux_ptr.len = rbuf->len - (blks_consumed * lsz);

[Severity: Critical]
Can this calculation underflow and cause a heap buffer overflow? 

Both blks_consumed and lsz are derived from inputs supplied by the
coprocessor. If (blks_consumed * lsz) is greater than rbuf->len,
aux_ptr.len will wrap to an excessively large 32-bit value.

[ ... ]
> +		case IPC_HDR_PKT_TYPE_HCI:
> +			buf = kmalloc(rbuf->len, GFP_ATOMIC);

[Severity: Medium]
As noted above, using the __le16 field rbuf->len directly in kmalloc() on
big-endian architectures could lead to excessively large allocation requests.

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

[Severity: Critical]
If aux_ptr.len underflows, this subtraction evaluates back to the large
(blks_consumed * lsz) value. 

Since buf is only allocated to rbuf->len, could this copy operation
write far out-of-bounds?

[ ... ]
> +static void btss_worker(struct work_struct *work)
> +{
[ ... ]
> +	if (unlikely(!READ_ONCE(desc->running))) {
> +		// FW sets offset of RX context info at start of memory region upon boot
> +		offset = readl(desc->mem_region);
> +		dev_dbg(desc->dev, "offset after M0 boot: 0x%08x\n", offset);
> +		desc->rx_ctxt = (struct context_info *)(desc->mem_region + offset);

[Severity: Critical]
Does this offset need bounds checking? 

The offset is read directly from the coprocessor and used to set the context
pointer. If an abnormally large offset is provided, this could point outside
the allocated mem_size, leading to out-of-bounds accesses.

[Severity: High]
Does this directly dereference I/O memory using a host struct pointer?

The shared memory region is mapped as void __iomem, but here it is cast to
a native struct context_info pointer. Because the struct is packed, the
compiler might generate unaligned or byte-wise loads when its fields are
accessed. 

On architectures like ARM64, unaligned loads to strongly-ordered device
memory can trigger synchronous external aborts.

[ ... ]
> +static int btss_init(struct qcom_btss *desc)
> +{
[ ... ]
> +	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);

[ ... ]
> +static int btqcomipc_close(struct hci_dev *hdev)
> +{
> +	struct qcom_btss *desc = hci_get_drvdata(hdev);
> +
> +	rproc_shutdown(desc->rproc);
> +	msleep(POWER_CONTROL_DELAY_MS);
> +
> +	return 0;
> +}

[Severity: High]
Does this implementation leak the workqueue and break interface toggling?

During btqcomipc_open(), btss_init() allocates a new workqueue and requests
the IRQ line. However, btqcomipc_close() doesn't tear down the workqueue or
free the IRQ. 

Toggling the interface via 'hciconfig hci0 down && hciconfig hci0 up'
would leak the workqueue, and the subsequent open might fail with -EBUSY
since the IRQ is already registered.

[ ... ]
> +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))) {

[Severity: Medium]
Is a memory barrier needed here to safely access the dependent context
pointers? 

In btss_recv_cust_frame(), pointers to shared context regions are populated
before calling WRITE_ONCE(desc->running, true). Without smp_store_release()
and smp_load_acquire(), weakly-ordered architectures could reorder the
execution.

This could allow the path below to observe desc->running as true while
reading a stale or NULL tx_ctxt.

> +		bt_dev_err(hdev, "BTSS not initialized, failed to send message");
> +		ret = -ENODEV;
> +		goto free_skb;
> +	}
> +
> +	ret = btss_send(desc, hdr, skb);

[Severity: High]
Can this lockless call corrupt the shared IPC ring buffers? 

btqcomipc_send() executes locklessly from the network stack context, while
btss_worker() can process the TX queue simultaneously. Both paths modify the
exact same ring buffer indices like rinfo->widx and internal state. 

Could this race lead to ring buffer corruption or packet loss without
acquiring desc->lock?

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