Re: [RFC PATCH v3 3/3] Bluetooth: btaic: Add AIC8800D80 support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] bt: aic: struct layout mismatch breaks firmware loading
- [High] bt: aic: synchronous request_firmware in probe deadlocks SDIO bus
- [High] bt: aic: type mismatch in h4_recv_buf() causes out-of-bounds access
- [Medium] bt: aic: rx_skb memory leak and state corruption on open failure
- [Medium] bt: aic: memory leak of skb on skb_cow_head failure
- [Low] bt: aic: build failure due to undefined kzalloc_obj macro
--

commit 0701d7f81641131610e34be895b933ef2ec43de9
Author: Yanli Yang <yanli.yang@bedmex.com>

Bluetooth: btaic: Add AIC8800D80 support

Add a driver for the AIC8800D80 combination chip, using SDIO function 1
for Bluetooth firmware loading and a serdev UART for H4 HCI traffic.

> diff --git a/drivers/bluetooth/btaic_core.c b/drivers/bluetooth/btaic_core.c
> --- /dev/null
> +++ b/drivers/bluetooth/btaic_core.c
> @@ -0,0 +1,180 @@

[ ... ]

> +struct aic_bt_boot *aic_bt_boot_register(struct device *dev)
> +{
> +	struct aic_bt_boot *boot;
> +
> +	boot = kzalloc_obj(*boot);
> +	if (!boot)
> +		return ERR_PTR(-ENOMEM);

[Severity: Low]
Is kzalloc_obj() defined in the kernel?

In aic_bt_boot_register(), the code attempts to allocate memory using
kzalloc_obj(). This macro is not a standard Linux kernel macro and does not
appear to be defined anywhere in the kernel source or driver headers. Will
this cause a compilation failure?

> diff --git a/drivers/bluetooth/btaic_sdio.c b/drivers/bluetooth/btaic_sdio.c
> --- /dev/null
> +++ b/drivers/bluetooth/btaic_sdio.c
> @@ -0,0 +1,901 @@

[ ... ]

> +struct aic_bt_e2a_header {
> +	__le16 id;
> +	__le16 dest_id;
> +	__le16 src_id;
> +	__le16 param_len;
> +	__le32 pattern;
> +	u8 param[];
> +} __packed;

[Severity: High]
Does this struct layout match the hardware format constructed during TX?

The driver manually constructs SDIO TX messages with a 4-byte zero padding
(pattern) starting at offset 4 of the buffer, followed by request_id at
offset 8, dest_id at 10, etc., in aic_bt_command():

	put_unaligned_le16(request_id, btdev->tx_buf + 8);
	put_unaligned_le16(AIC_TASK_DBG, btdev->tx_buf + 10);
	put_unaligned_le16(AIC_DRIVER_TASK, btdev->tx_buf + 12);
	put_unaligned_le16(param_len, btdev->tx_buf + 14);
	if (param_len)
		memcpy(btdev->tx_buf + 16, param, param_len);

However, the RX parser receives the payload starting at offset 4 and casts
it to struct aic_bt_e2a_header. This struct defines __le16 id at offset 0
and __le32 pattern at offset 8, which reverses the TX layout.

Due to this layout mismatch, message->id reads the zero padding instead of
the ID, and message->param_len reads the dest_id. Does this cause all
received responses to fail validation in aic_bt_complete_response() and
prevent firmware loading?

[ ... ]

> +static int aic_bt_download_firmware(struct aic_bt_sdio *btdev)
> +{
> +	const struct firmware *table;
> +	u32 chip_id;
> +	u32 adid_addr;
> +	u32 patch_addr;
> +	u8 revision;
> +	int err;
> +
> +	err = aic_bt_mem_read(btdev, AIC_BT_CHIP_ID_ADDR, &chip_id);
> +	if (err)
> +		return err;
> +
> +	revision = (chip_id >> 16) & 0x3f;
> +	if (revision != AIC_BT_CHIP_REV_U02 &&
> +	    revision != AIC_BT_CHIP_REV_U03) {
> +		dev_err(&btdev->func->dev,
> +			"unsupported AIC8800D80 revision %u\n", revision);
> +		return -ENODEV;
> +	}
> +
> +	err = request_firmware(&table, AIC_BT_FW_TABLE, &btdev->func->dev);

[Severity: High]
Can calling request_firmware() synchronously from the SDIO probe path
deadlock the bus?

The SDIO probe function aic_bt_sdio_probe() calls aic_bt_download_firmware(),
which initiates synchronous request_firmware() calls.

SDIO probes execute in the context of the MMC host worker thread. Blocking
the MMC host worker thread prevents the MMC subsystem from servicing other
requests.

If the requested firmware resides on a filesystem mounted via the same MMC
host, this could create an unresolvable deadlock. Even if it does not, a
missing firmware file might block the MMC bus for 60 seconds waiting for
the user-mode helper timeout.

> diff --git a/drivers/bluetooth/btaic_uart.c b/drivers/bluetooth/btaic_uart.c
> --- /dev/null
> +++ b/drivers/bluetooth/btaic_uart.c
> @@ -0,0 +1,328 @@

[ ... ]

> +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));

[Severity: High]
Does this pass the correct pointer type to h4_recv_buf()?

The generic Bluetooth core function h4_recv_buf() requires a
struct hci_dev *hdev as its first parameter. However, aic_bt_receive_buf()
passes &uart->hu, which is a pointer to struct hci_uart.

If h4_recv_buf() encounters a framing error or an internal issue, it logs
the error using bt_dev_err(hdev, ...). Passing the incorrect struct pointer
could cause the generic core to read out-of-bounds memory when evaluating
hdev->name, potentially leading to garbage logs, memory corruption, or
kernel panics.

[ ... ]

> +static int aic_bt_open(struct hci_dev *hdev)
> +{
> +	struct aic_bt_uart *uart = hci_get_drvdata(hdev);
> +	unsigned int actual_speed;
> +	int err;
> +
> +	err = aic_bt_boot_wait(uart->boot,
> +			       msecs_to_jiffies(AIC_BT_BOOT_TIMEOUT_MS));
> +	if (err) {
> +		bt_dev_err(hdev, "Bluetooth firmware is not ready (%d)", err);
> +		return err;
> +	}
> +
> +	err = serdev_device_open(uart->serdev);
> +	if (err)
> +		return err;
> +
> +	actual_speed = serdev_device_set_baudrate(uart->serdev, AIC_BT_UART_DEFAULT_SPEED);
> +	if (!actual_speed) {
> +		serdev_device_close(uart->serdev);
> +		return -EIO;
> +	}

[Severity: Medium]
Does this error path leak uart->rx_skb?

In aic_bt_open(), the serdev is opened before the baudrate is set. RX data
may arrive immediately and allocate uart->rx_skb via the RX callback.

If setting the baudrate fails, the function returns -EIO and closes the
serdev, but uart->rx_skb is not freed or set to NULL on this error path.

Since the HCI core considers the device unopened on failure, close() is never
called. If the interface is subsequently brought up again, the stale rx_skb
will corrupt the next received packet sequence, and the original allocation is
leaked.

[ ... ]

> +static int aic_bt_send_frame(struct hci_dev *hdev, struct sk_buff *skb)
> +{
> +	struct aic_bt_uart *uart = hci_get_drvdata(hdev);
> +	u8 pkt_type = hci_skb_pkt_type(skb);
> +	int err;
> +
> +	err = skb_cow_head(skb, 1);
> +	if (err)
> +		return err;

[Severity: Medium]
Should this error path free the skb?

The Bluetooth HCI core transfers ownership of the skb to the driver's
hdev->send() callback. If the driver's send callback drops the packet (even
if it returns an error), it must free the skb.

In aic_bt_send_frame(), if skb_cow_head() fails, the function returns -ENOMEM
but does not invoke kfree_skb(skb). Could this silently drop the reference
and create a memory leak?

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