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 5 potential issue(s) to consider:
- [High] bluetooth: btaic: off-by-one in SDIO credit check causes timeout
- [High] bluetooth: btaic: missing SDIO transaction sequence numbers
- [High] bluetooth: btaic: UAF race in aic_bt_flush with RX path
- [High] bluetooth: btaic: deadlock in SDIO IRQ due to GFP_KERNEL allocation
- [Medium] bluetooth: btaic: UART max-speed DT property breaks firmware baud rate sync
--

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

Adds a driver for the AIC8800D80 combination chip. It implements an SDIO
firmware loader and a serdev UART for H4 HCI traffic.

> diff --git a/drivers/bluetooth/btaic_sdio.c b/drivers/bluetooth/btaic_sdio.c
> --- a/drivers/bluetooth/btaic_sdio.c
> +++ b/drivers/bluetooth/btaic_sdio.c
[ ... ]
> static void aic_bt_complete_response(struct aic_bt_sdio *btdev,
> 				     const struct aic_bt_e2a_header *message,
> 				     size_t message_len)
> {
[ ... ]
> 	spin_lock_irqsave(&btdev->response_lock, flags);
> 	if (!btdev->waiting_response || id != btdev->expected_response)
> 		goto unlock;

[Severity: High]
Is there a risk of mismatched responses if a command times out?

In aic_bt_command(), a timeout clears waiting_response and aborts, leaving
the hardware state machine active:

    spin_lock_irqsave(&btdev->response_lock, flags);
    btdev->waiting_response = false;
    btdev->response = NULL;

If a subsequent command of the same type is sent, and the hardware belatedly
responds to the timed-out request, aic_bt_complete_response() would accept
the late response since the expected_response ID matches. Could this corrupt
the initialization sequence?

[ ... ]
> static void aic_bt_sdio_irq(struct sdio_func *func)
> {
[ ... ]
> 	data = kmalloc(data_len, GFP_KERNEL);

[Severity: High]
Could this allocation deadlock under memory pressure?

The mmc core's sdio_irq_thread calls aic_bt_sdio_irq() with the mmc host lock
held. If GFP_KERNEL triggers direct reclaim, and reclaim tries to flush dirty
pages to a block device on the same mmc host, it would attempt to acquire the
host lock again, hanging the system.

[ ... ]
> static int aic_bt_wait_for_credits(struct aic_bt_sdio *btdev, size_t tx_len)
> {
[ ... ]
> 	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: High]
Can this condition prevent transmission when exactly the right amount of
credits are available?

If the hardware grants exactly the required number of credits, this strictly
less-than check evaluates to false, causing the initialization to time out.
Should this be a <= check?

[ ... ]
> static u32 aic_bt_mode_value(unsigned int pair, u32 firmware_value)
> {
> 	switch (pair) {
[ ... ]
> 	case 5:
> 		return AIC_BT_UART_BAUD;

[Severity: Medium]
Does this firmware configuration get out of sync with the device tree speed?

Here, aic_bt_mode_value() hardcodes the firmware's baud rate patch to
1,500,000. However, in aic_bt_uart_probe(), the max-speed DT property is
read and configures the host UART speed. If the device tree specifies a
different speed, the host and controller will operate at different baud rates,
causing HCI communication to fail.

> diff --git a/drivers/bluetooth/btaic_uart.c b/drivers/bluetooth/btaic_uart.c
> --- a/drivers/bluetooth/btaic_uart.c
> +++ b/drivers/bluetooth/btaic_uart.c
[ ... ]
> 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,
[ ... ]
> static int aic_bt_flush(struct hci_dev *hdev)
> {
[ ... ]
> 	kfree_skb(uart->rx_skb);
> 	uart->rx_skb = NULL;

[Severity: High]
Could this race with concurrent UART receives and cause a use-after-free?

When the HCI core invokes aic_bt_flush(), it frees uart->rx_skb. But the
serdev device is not closed until aic_bt_close(), meaning aic_bt_receive_buf()
could be executing simultaneously on another CPU, calling h4_recv_buf() on the
freed socket buffer.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/178954773705.2.3446471521233620446@bedmex.com?part=3