Re: [RFC PATCH bluetooth-next 3/3] Bluetooth: btaic: Add AIC8800D80 SDIO loader and UART transport

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] bluetooth: btaic: use-after-free of rx_skb during concurrent flush and receive
- [Medium] bluetooth: btaic: off-by-one in credits check causes timeout for exact-fit payloads
- [Medium] bluetooth: btaic: spurious command failure due to delayed completion signal
--

commit d0fbf93152e71e54fc5cd6389eb4e023f7adbfc3
Author: Yanli Yang <yanli.yang@bedmex.com>
Subject: Bluetooth: btaic: Add AIC8800D80 SDIO loader and UART transport

This commit adds a driver for the AIC8800D80 combination chip. It utilizes 
SDIO function 1 for loading Bluetooth firmware and a serdev UART interface 
for handling the H4 HCI traffic. The firmware loading process uses a vendor 
protocol to validate patch-table layouts and transfer the necessary images.

> --- /dev/null
> +++ b/drivers/bluetooth/btaic_sdio.c
> @@ -0,0 +1,886 @@

[ ... ]

> +static void aic_bt_complete_response(struct aic_bt_sdio *btdev,
> +				     const struct aic_bt_e2a_header *message,
> +				     size_t message_len)
> +{
> +	unsigned long flags;
> +	size_t param_len;
> +	u16 id;
> +	bool complete_command = false;
> +
> +	if (message_len < sizeof(*message))
> +		return;
> +
> +	id = get_unaligned_le16(&message->id);
> +	param_len = get_unaligned_le16(&message->param_len);
> +	if (param_len > message_len - sizeof(*message))
> +		return;
> +
> +	spin_lock_irqsave(&btdev->response_lock, flags);
> +	if (!btdev->waiting_response || id != btdev->expected_response)
> +		goto unlock;
> +
> +	if (btdev->response && param_len < btdev->response_size) {
> +		btdev->command_result = -EMSGSIZE;
> +	} else {
> +		if (btdev->response)
> +			memcpy(btdev->response, message->param,
> +			       btdev->response_size);
> +		btdev->command_result = 0;
> +	}
> +
> +	btdev->waiting_response = false;
> +	complete_command = true;
> +
> +unlock:
> +	spin_unlock_irqrestore(&btdev->response_lock, flags);
> +
> +	if (complete_command)
> +		complete(&btdev->command_done);
> +}

[Severity: Medium]
Could this delayed completion signal cause a race condition with a subsequent
command?

If a command times out, aic_bt_command() aborts and unlocks command_mutex.
If the SDIO IRQ handler processes the timed-out command's delayed response
concurrently, it releases response_lock just before calling complete().

If another thread executing aic_bt_command() acquires command_mutex and calls
reinit_completion() in this narrow window, the delayed complete() might satisfy
the new command's wait immediately. The new command would then incorrectly
return its initial command_result (-EINPROGRESS) instead of waiting for its
own response.

[ ... ]

> +static int aic_bt_wait_for_credits(struct aic_bt_sdio *btdev, size_t tx_len)
> +{
> +	unsigned int retry;
> +	int err;
> +
> +	for (retry = 0; retry < AIC_SDIO_FLOW_RETRIES; retry++) {
> +		u8 credits;
> +
> +		credits = sdio_readb(btdev->func, AIC_SDIO_FLOW_CTRL_Q1, &err);
> +		if (err)
> +			return err;
> +
> +		if (credits && tx_len < (size_t)credits * AIC_SDIO_BUFFER_SIZE)
> +			return 0;

[Severity: Medium]
Does this strict inequality incorrectly reject a transmission that exactly fits
the available credit buffers?

For example, when aic_bt_mem_block_write() sends firmware chunks of 1024
bytes, the total tx_len (including headers and padding) is calculated as
1536 bytes. If the hardware advertises exactly 1 credit (1536 bytes of
capacity), the check (1536 < 1536) evaluates to false, causing the driver
to unnecessarily spin until it times out.

> +
> +		if (retry < 30)
> +			usleep_range(30, 50);
> +		else if (retry < 40)
> +			usleep_range(1000, 1500);
> +		else
> +			usleep_range(10000, 12000);
> +	}
> +
> +	return -ETIMEDOUT;
> +}
> +
> +static int aic_bt_command(struct aic_bt_sdio *btdev, u16 request_id,
> +			  const void *param, size_t param_len, u16 response_id,
> +			  void *response, size_t response_size)
> +{
> +	unsigned long flags;
> +	size_t frame_len;
> +	size_t message_len;
> +	size_t tx_len;
> +	long timeout;
> +	int err;

[ ... ]

> +	reinit_completion(&btdev->command_done);
> +	spin_lock_irqsave(&btdev->response_lock, flags);
> +	btdev->expected_response = response_id;
> +	btdev->response = response;
> +	btdev->response_size = response_size;
> +	btdev->command_result = -EINPROGRESS;
> +	btdev->waiting_response = true;
> +	spin_unlock_irqrestore(&btdev->response_lock, flags);

[ ... ]

> --- /dev/null
> +++ b/drivers/bluetooth/btaic_uart.c
> @@ -0,0 +1,331 @@

[ ... ]

> +static size_t aic_bt_receive_buf(struct serdev_device *serdev,
> +				 const u8 *data, size_t count)
> +{
> +	struct aic_bt_uart *uart = serdev_device_get_drvdata(serdev);
> +
> +	uart->rx_skb = h4_recv_buf(&uart->hu, uart->rx_skb, data, count,
> +				   aic_bt_recv_pkts,
> +				   ARRAY_SIZE(aic_bt_recv_pkts));

[ ... ]

> +static int aic_bt_close(struct hci_dev *hdev)
> +{
> +	struct aic_bt_uart *uart = hci_get_drvdata(hdev);
> +
> +	serdev_device_close(uart->serdev);
> +	return 0;
> +}
> +
> +static int aic_bt_flush(struct hci_dev *hdev)
> +{
> +	struct aic_bt_uart *uart = hci_get_drvdata(hdev);
> +
> +	serdev_device_write_flush(uart->serdev);
> +	cancel_work_sync(&uart->tx_work);
> +	skb_queue_purge(&uart->txq);
> +	kfree_skb(uart->rx_skb);
> +	uart->rx_skb = NULL;

[Severity: High]
Can this lead to a use-after-free of uart->rx_skb if data is received
concurrently during teardown?

During interface teardown, the HCI stack calls hdev->flush() before it calls
hdev->close(). This means aic_bt_flush() will free uart->rx_skb while the
UART serdev is still open.

If a UART receive interrupt fires before aic_bt_close() runs,
aic_bt_receive_buf() could be called and pass the freed uart->rx_skb
pointer into h4_recv_buf(), causing memory corruption.

> +	clear_bit(AIC_BT_TX_ACTIVE, &uart->tx_state);
> +	clear_bit(AIC_BT_TX_WAKEUP, &uart->tx_state);
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1789546358.git.yanli.yang@bedmex.com?part=3