[PATCH 0/5] Bluetooth: harden packet and transport parsing

[PATCH 0/5] Bluetooth: harden packet and transport parsing

[Li Qiang]

This series hardens Bluetooth packet receive paths and the HCI UART
transmit path against malformed lengths and invalid callback return
values.

The affected code accesses protocol fields before confirming that the
complete headers are present, or trusts a received length while
appending to an skb. The HCI UART write worker also assumes that a tty
driver always returns a valid written-byte count. These assumptions can
lead to out-of-bounds accesses or invalid skb manipulation when data is
truncated or a lower-level driver reports an invalid result.

Validate packet headers and declared data ranges before dereferencing
or copying them. Reject tty write return values outside the skb length,
and reset the QCA memdump state when a collection is discarded.

The affected Bluetooth objects were built with the required Bluetooth
configuration dependencies enabled. The series also passes
git diff --check and checkpatch on the generated patches.

Li Qiang (5):
  Bluetooth: bfusb: validate received block boundaries
  Bluetooth: btmrvl: validate event packet lengths
  Bluetooth: hci_bcsp: validate received packet lengths
  Bluetooth: hci_ldisc: reject invalid tty write lengths
  Bluetooth: hci_qca: validate controller memdump frames

 drivers/bluetooth/bfusb.c       | 37 +++++++++++++++++++--
 drivers/bluetooth/btmrvl_main.c | 10 ++++++
 drivers/bluetooth/hci_bcsp.c    |  5 ++-
 drivers/bluetooth/hci_ldisc.c   |  6 ++++
 drivers/bluetooth/hci_qca.c     | 58 +++++++++++++++++++++++----------
 5 files changed, 95 insertions(+), 21 deletions(-)


base-commit: 123b934b63a0bb3f7aa016f640b992406acd4047
-- 
2.43.0

[PATCH 1/5] Bluetooth: bfusb: validate received block boundaries

[Li Qiang]

The USB receive path trusts the block header to contain the required
number of bytes and passes it to the reassembly routine. The routine
also trusts a malformed HCI packet type and can append more data than
the skb allocated from the advertised packet length. A malformed USB
transfer can therefore cause out-of-bounds reads or an skb tail
overwrite.

Validate block header availability, declared block size, packet type,
and reassembly tailroom. Drop the partial frame on an invalid block.

Signed-off-by: Li Qiang <liqiang01@kylinos.cn>
---
 drivers/bluetooth/bfusb.c | 37 ++++++++++++++++++++++++++++++++++---
 1 file changed, 34 insertions(+), 3 deletions(-)

diff --git a/drivers/bluetooth/bfusb.c b/drivers/bluetooth/bfusb.c
index 8df310983bf6..d31d797639b5 100644
--- a/drivers/bluetooth/bfusb.c
+++ b/drivers/bluetooth/bfusb.c
@@ -301,6 +301,11 @@ static inline int bfusb_recv_block(struct bfusb_data *data, int hdr, unsigned ch
 				return -EILSEQ;
 			}
 			break;
+
+		default:
+			bt_dev_err(data->hdev, "unknown packet type 0x%02x",
+				   pkt_type);
+			return -EILSEQ;
 		}
 
 		skb = bt_skb_alloc(pkt_len, GFP_ATOMIC);
@@ -319,6 +324,13 @@ static inline int bfusb_recv_block(struct bfusb_data *data, int hdr, unsigned ch
 		}
 	}
 
+	if (len > skb_tailroom(data->reassembly)) {
+		bt_dev_err(data->hdev, "block exceeds packet length");
+		kfree_skb(data->reassembly);
+		data->reassembly = NULL;
+		return -EILSEQ;
+	}
+
 	if (len > 0)
 		skb_put_data(data->reassembly, buf, len);
 
@@ -353,6 +365,13 @@ static void bfusb_rx_complete(struct urb *urb)
 	skb_put(skb, count);
 
 	while (count) {
+		if (count < 2) {
+			bt_dev_err(data->hdev, "short block header");
+			kfree_skb(data->reassembly);
+			data->reassembly = NULL;
+			break;
+		}
+
 		hdr = buf[0] | (buf[1] << 8);
 
 		if (hdr & 0x4000) {
@@ -360,16 +379,28 @@ static void bfusb_rx_complete(struct urb *urb)
 			count -= 2;
 			buf   += 2;
 		} else {
+			if (count < 3) {
+				bt_dev_err(data->hdev, "short block header");
+				kfree_skb(data->reassembly);
+				data->reassembly = NULL;
+				break;
+			}
+
 			len = (buf[2] == 0) ? 256 : buf[2];
 			count -= 3;
 			buf   += 3;
 		}
 
-		if (count < len)
+		if (count < len) {
 			bt_dev_err(data->hdev, "block extends over URB buffer ranges");
+			kfree_skb(data->reassembly);
+			data->reassembly = NULL;
+			break;
+		}
 
-		if ((hdr & 0xe1) == 0xc1)
-			bfusb_recv_block(data, hdr, buf, len);
+		if ((hdr & 0xe1) == 0xc1 &&
+		    bfusb_recv_block(data, hdr, buf, len) < 0)
+			data->hdev->stat.err_rx++;
 
 		count -= len;
 		buf   += len;
-- 
2.43.0

RE: Bluetooth: harden packet and transport parsing

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 557 bytes --]

This is an automated email and please do not reply to this email.

Dear Submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
While preparing the CI tests, the patches you submitted couldn't be applied to the current HEAD of the repository.

----- Output -----

error: patch failed: drivers/bluetooth/hci_qca.c:1082
error: drivers/bluetooth/hci_qca.c: patch does not apply
hint: Use 'git am --show-current-patch' to see the failed patch

Please resolve the issue and submit the patches again.


---
Regards,
Linux Bluetooth

[PATCH 2/5] Bluetooth: btmrvl: validate event packet lengths

[Li Qiang]

The Marvell event handlers access the HCI event header, command
complete payload, and driver-specific event header before validating
that the received skb contains them. A truncated event can consequently
cause an out-of-bounds read.

Validate each header and the command-complete payload length before
dereferencing the corresponding fields.

Signed-off-by: Li Qiang <liqiang01@kylinos.cn>
---
 drivers/bluetooth/btmrvl_main.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/bluetooth/btmrvl_main.c b/drivers/bluetooth/btmrvl_main.c
index d6f0ad0b4b6e..aaf1614ccfd7 100644
--- a/drivers/bluetooth/btmrvl_main.c
+++ b/drivers/bluetooth/btmrvl_main.c
@@ -43,10 +43,17 @@ bool btmrvl_check_evtpkt(struct btmrvl_private *priv, struct sk_buff *skb)
 {
 	struct hci_event_hdr *hdr = (void *) skb->data;
 
+	if (skb->len < sizeof(*hdr))
+		return true;
+
 	if (hdr->evt == HCI_EV_CMD_COMPLETE) {
 		struct hci_ev_cmd_complete *ec;
 		u16 opcode;
 
+		if (hdr->plen < sizeof(*ec) ||
+		    skb->len < HCI_EVENT_HDR_SIZE + sizeof(*ec))
+			return true;
+
 		ec = (void *) (skb->data + HCI_EVENT_HDR_SIZE);
 		opcode = __le16_to_cpu(ec->opcode);
 
@@ -74,6 +81,9 @@ int btmrvl_process_event(struct btmrvl_private *priv, struct sk_buff *skb)
 	struct btmrvl_event *event;
 	int ret = 0;
 
+	if (skb->len < sizeof(*event))
+		return -EINVAL;
+
 	event = (struct btmrvl_event *) skb->data;
 	if (event->ec != 0xff) {
 		BT_DBG("Not Marvell Event=%x", event->ec);
-- 
2.43.0

[PATCH 3/5] Bluetooth: hci_bcsp: validate received packet lengths

[Li Qiang]

The BCSP transmit path reads an HCI command header when an extension
packet has only been tested for a nonzero length. Its LE configuration
packet handler also indexes bytes through offset seven without a length
check.

Validate the complete command and LE configuration packet headers
before accessing their fields.

Signed-off-by: Li Qiang <liqiang01@kylinos.cn>
---
 drivers/bluetooth/hci_bcsp.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/bluetooth/hci_bcsp.c b/drivers/bluetooth/hci_bcsp.c
index db56eead27ce..0323db21c428 100644
--- a/drivers/bluetooth/hci_bcsp.c
+++ b/drivers/bluetooth/hci_bcsp.c
@@ -194,7 +194,7 @@ static struct sk_buff *bcsp_prepare_pkt(struct bcsp_struct *bcsp, u8 *data,
 		return NULL;
 	}
 
-	if (hciextn && chan == 5) {
+	if (hciextn && chan == 5 && len > HCI_COMMAND_HDR_SIZE) {
 		__le16 opcode = ((struct hci_command_hdr *)data)->opcode;
 
 		/* Vendor specific commands */
@@ -402,6 +402,9 @@ static void bcsp_handle_le_pkt(struct hci_uart *hu)
 	u8 sync_pkt[4]     = { 0xda, 0xdc, 0xed, 0xed };
 
 	/* spot "conf" pkts and reply with a "conf rsp" pkt */
+	if (bcsp->rx_skb->len < 8)
+		return;
+
 	if (bcsp->rx_skb->data[1] >> 4 == 4 && bcsp->rx_skb->data[2] == 0 &&
 	    !memcmp(&bcsp->rx_skb->data[4], conf_pkt, 4)) {
 		struct sk_buff *nskb = alloc_skb(4, GFP_ATOMIC);
-- 
2.43.0

[PATCH 4/5] Bluetooth: hci_ldisc: reject invalid tty write lengths

[Li Qiang]

The HCI UART write worker assumes that a tty write callback returns a
value in the range from zero through the skb length. A negative value or
a value larger than the skb length is passed to accounting and skb_pull,
which can corrupt skb state.

Treat either return value as a transmit error and discard the skb.

Signed-off-by: Li Qiang <liqiang01@kylinos.cn>
---
 drivers/bluetooth/hci_ldisc.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index 47f4902b40b4..668c4f84d7f2 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -163,6 +163,12 @@ static void hci_uart_write_work(struct work_struct *work)
 
 		set_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
 		len = tty->ops->write(tty, skb->data, skb->len);
+		if (len < 0 || len > skb->len) {
+			hdev->stat.err_tx++;
+			kfree_skb(skb);
+			continue;
+		}
+
 		hdev->stat.byte_tx += len;
 
 		skb_pull(skb, len);
-- 
2.43.0

[PATCH 5/5] Bluetooth: hci_qca: validate controller memdump frames

[Li Qiang]

The QCA controller memdump worker reads frame and dump-size headers
before validating the received skb length. The event classifier likewise
accesses its fixed header without ensuring that all accessed bytes are
present. Malformed controller data can therefore cause out-of-bounds
reads.

Validate every header and the initial dump size before changing the dump
state. Reset the local dump state when discarding an unstarted
collection. Use subtraction-based capacity checks to prevent overflow
while accounting for received and synthesized dump data.

Signed-off-by: Li Qiang <liqiang01@kylinos.cn>
---
 drivers/bluetooth/hci_qca.c | 58 ++++++++++++++++++++++++++-----------
 1 file changed, 41 insertions(+), 17 deletions(-)

diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index b2d1ee3a3d11..b5d4b8e0e376 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -1057,6 +1057,34 @@ static void qca_controller_memdump(struct work_struct *work)
 			return;
 		}
 
+		if (skb->len < sizeof(*cmd_hdr)) {
+			bt_dev_err(hu->hdev, "Rx short memdump header");
+			kfree_skb(skb);
+			mutex_unlock(&qca->hci_memdump_lock);
+			continue;
+		}
+
+		cmd_hdr = (void *) skb->data;
+		seq_no = __le16_to_cpu(cmd_hdr->seq_no);
+		skb_pull(skb, sizeof(struct qca_memdump_event_hdr));
+
+		if (!seq_no) {
+			if (skb->len < sizeof(*dump)) {
+				bt_dev_err(hu->hdev, "Rx short memdump size");
+				kfree_skb(skb);
+				mutex_unlock(&qca->hci_memdump_lock);
+				continue;
+			}
+
+			dump = (void *)skb->data;
+			if (!__le32_to_cpu(dump->dump_size)) {
+				bt_dev_err(hu->hdev, "Rx invalid memdump size");
+				kfree_skb(skb);
+				mutex_unlock(&qca->hci_memdump_lock);
+				continue;
+			}
+		}
+
 		if (!qca_memdump) {
 			qca_memdump = kzalloc_obj(*qca_memdump, GFP_ATOMIC);
 			if (!qca_memdump) {
@@ -1068,9 +1096,6 @@ static void qca_controller_memdump(struct work_struct *work)
 		}
 
 		qca->memdump_state = QCA_MEMDUMP_COLLECTING;
-		cmd_hdr = (void *) skb->data;
-		seq_no = __le16_to_cpu(cmd_hdr->seq_no);
-		skb_pull(skb, sizeof(struct qca_memdump_event_hdr));
 
 		if (!seq_no) {
 
@@ -1082,15 +1107,7 @@ static void qca_controller_memdump(struct work_struct *work)
 			 */
 			set_bit(QCA_IBS_DISABLED, &qca->flags);
 			set_bit(QCA_MEMDUMP_COLLECTION, &qca->flags);
-			dump = (void *) skb->data;
 			qca_memdump->ram_dump_size = __le32_to_cpu(dump->dump_size);
-			if (!(qca_memdump->ram_dump_size)) {
-				bt_dev_err(hu->hdev, "Rx invalid memdump size");
-				kfree(qca_memdump);
-				kfree_skb(skb);
-				mutex_unlock(&qca->hci_memdump_lock);
-				return;
-			}
 
 			queue_delayed_work(qca->workqueue,
 					   &qca->ctrl_memdump_timeout,
@@ -1123,6 +1140,8 @@ static void qca_controller_memdump(struct work_struct *work)
 		if (!test_bit(QCA_MEMDUMP_COLLECTION, &qca->flags)) {
 			bt_dev_err(hu->hdev, "QCA: Discarding other packets");
 			kfree(qca_memdump);
+			qca->qca_memdump = NULL;
+			qca->memdump_state = QCA_MEMDUMP_IDLE;
 			kfree_skb(skb);
 			mutex_unlock(&qca->hci_memdump_lock);
 			return;
@@ -1140,9 +1159,11 @@ static void qca_controller_memdump(struct work_struct *work)
 			seq_no != QCA_LAST_SEQUENCE_NUM) {
 			bt_dev_err(hu->hdev, "QCA controller missed packet:%d",
 				   qca_memdump->current_seq_no);
-			rx_size = qca_memdump->received_dump;
-			rx_size += QCA_DUMP_PACKET_SIZE;
-			if (rx_size > qca_memdump->ram_dump_size) {
+			if (qca_memdump->received_dump >
+				    qca_memdump->ram_dump_size ||
+			    QCA_DUMP_PACKET_SIZE >
+				    qca_memdump->ram_dump_size -
+				    qca_memdump->received_dump) {
 				bt_dev_err(hu->hdev,
 					   "QCA memdump received %d, no space for missed packet",
 					   qca_memdump->received_dump);
@@ -1154,8 +1175,10 @@ static void qca_controller_memdump(struct work_struct *work)
 			qca_memdump->current_seq_no++;
 		}
 
-		rx_size = qca_memdump->received_dump  + skb->len;
-		if (rx_size <= qca_memdump->ram_dump_size) {
+		if (qca_memdump->received_dump <= qca_memdump->ram_dump_size &&
+		    skb->len <= qca_memdump->ram_dump_size -
+				qca_memdump->received_dump) {
+			rx_size = qca_memdump->received_dump + skb->len;
 			if ((seq_no != QCA_LAST_SEQUENCE_NUM) &&
 			    (seq_no != qca_memdump->current_seq_no)) {
 				bt_dev_err(hu->hdev,
@@ -1235,7 +1258,8 @@ static int qca_recv_event(struct hci_dev *hdev, struct sk_buff *skb)
 	 * received we store dump into a file before closing hci. This
 	 * dump will help in triaging the issues.
 	 */
-	if ((skb->data[0] == HCI_VENDOR_PKT) &&
+	if (skb->len >= offsetof(struct qca_memdump_event_hdr, seq_no) &&
+	    (skb->data[0] == HCI_VENDOR_PKT) &&
 	    (get_unaligned_be16(skb->data + 2) == QCA_SSR_DUMP_HANDLE))
 		return qca_controller_memdump_event(hdev, skb);
 
-- 
2.43.0