[PATCH v2] Bluetooth: btintel: Use skb_pull_data return for bounds check

[PATCH v2] Bluetooth: btintel: Use skb_pull_data return for bounds check

[Quan Sun]

The length check at the top of btintel_print_fseq_info() verifies
that the skb has at least 66 bytes (sizeof(u32) * 16 + 2), but the
function actually consumes 74 bytes:

   2 calls to skb_pull_data(skb, 1)  =  2 bytes
   18 calls to skb_pull_data(skb, 4) = 72 bytes

When the firmware returns a packet of exactly 66 bytes, the last two
skb_pull_data(skb, 4) calls return NULL, which is then passed directly
to get_unaligned_le32(), resulting in a NULL pointer dereference.

This patch refactors the function to check the return value of
`skb_pull_data` instead. This ensures that every time data is pulled
from the `skb`, we explicitly verify that it didn't return NULL,
guaranteeing safe access. A `malformed` label is added to handle
early termination in case the `skb` is too short, preserving the
original `bt_dev_dbg` logging behavior.

Fixes: a7ba218a44aa ("Bluetooth: btintel: Print Firmware Sequencer 
information")
Signed-off-by: Quan Sun <2022090917019@std.uestc.edu.cn>
---
v2:
   - Remove manual length check and validate every skb_pull_data() return.
   - Add 'malformed' label for unified error handling.
   - Fix GitLint title length warning.

  drivers/bluetooth/btintel.c | 108 +++++++++++++++++++++++++++---------
  1 file changed, 81 insertions(+), 27 deletions(-)

diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
index dcaaa4ca02b99..053be9f88f7c3 100644
--- a/drivers/bluetooth/btintel.c
+++ b/drivers/bluetooth/btintel.c
@@ -3356,14 +3356,10 @@ void btintel_print_fseq_info(struct hci_dev *hdev)
  		return;
  	}

-	if (skb->len < (sizeof(u32) * 16 + 2)) {
-		bt_dev_dbg(hdev, "Malformed packet of length %u received",
-			   skb->len);
-		kfree_skb(skb);
-		return;
-	}
-
  	p = skb_pull_data(skb, 1);
+	if (!p)
+		goto malformed;
+
  	if (*p) {
  		bt_dev_dbg(hdev, "Failed to get fseq status (0x%2.2x)", *p);
  		kfree_skb(skb);
@@ -3371,6 +3367,9 @@ void btintel_print_fseq_info(struct hci_dev *hdev)
  	}

  	p = skb_pull_data(skb, 1);
+	if (!p)
+		goto malformed;
+
  	switch (*p) {
  	case 0:
  		str = "Success";
@@ -3394,65 +3393,120 @@ void btintel_print_fseq_info(struct hci_dev *hdev)

  	bt_dev_info(hdev, "Fseq status: %s (0x%2.2x)", str, *p);

-	val = get_unaligned_le32(skb_pull_data(skb, 4));
+	p = skb_pull_data(skb, 4);
+	if (!p)
+		goto malformed;
+	val = get_unaligned_le32(p);
  	bt_dev_dbg(hdev, "Reason: 0x%8.8x", val);

-	val = get_unaligned_le32(skb_pull_data(skb, 4));
+	p = skb_pull_data(skb, 4);
+	if (!p)
+		goto malformed;
+	val = get_unaligned_le32(p);
  	bt_dev_dbg(hdev, "Global version: 0x%8.8x", val);

-	val = get_unaligned_le32(skb_pull_data(skb, 4));
+	p = skb_pull_data(skb, 4);
+	if (!p)
+		goto malformed;
+	val = get_unaligned_le32(p);
  	bt_dev_dbg(hdev, "Installed version: 0x%8.8x", val);

-	p = skb->data;
-	skb_pull_data(skb, 4);
+	p = skb_pull_data(skb, 4);
+	if (!p)
+		goto malformed;
  	bt_dev_info(hdev, "Fseq executed: %2.2u.%2.2u.%2.2u.%2.2u", p[0], p[1],
  		    p[2], p[3]);

-	p = skb->data;
-	skb_pull_data(skb, 4);
+	p = skb_pull_data(skb, 4);
+	if (!p)
+		goto malformed;
  	bt_dev_info(hdev, "Fseq BT Top: %2.2u.%2.2u.%2.2u.%2.2u", p[0], p[1],
  		    p[2], p[3]);

-	val = get_unaligned_le32(skb_pull_data(skb, 4));
+	p = skb_pull_data(skb, 4);
+	if (!p)
+		goto malformed;
+	val = get_unaligned_le32(p);
  	bt_dev_dbg(hdev, "Fseq Top init version: 0x%8.8x", val);

-	val = get_unaligned_le32(skb_pull_data(skb, 4));
+	p = skb_pull_data(skb, 4);
+	if (!p)
+		goto malformed;
+	val = get_unaligned_le32(p);
  	bt_dev_dbg(hdev, "Fseq Cnvio init version: 0x%8.8x", val);

-	val = get_unaligned_le32(skb_pull_data(skb, 4));
+	p = skb_pull_data(skb, 4);
+	if (!p)
+		goto malformed;
+	val = get_unaligned_le32(p);
  	bt_dev_dbg(hdev, "Fseq MBX Wifi file version: 0x%8.8x", val);

-	val = get_unaligned_le32(skb_pull_data(skb, 4));
+	p = skb_pull_data(skb, 4);
+	if (!p)
+		goto malformed;
+	val = get_unaligned_le32(p);
  	bt_dev_dbg(hdev, "Fseq BT version: 0x%8.8x", val);

-	val = get_unaligned_le32(skb_pull_data(skb, 4));
+	p = skb_pull_data(skb, 4);
+	if (!p)
+		goto malformed;
+	val = get_unaligned_le32(p);
  	bt_dev_dbg(hdev, "Fseq Top reset address: 0x%8.8x", val);

-	val = get_unaligned_le32(skb_pull_data(skb, 4));
+	p = skb_pull_data(skb, 4);
+	if (!p)
+		goto malformed;
+	val = get_unaligned_le32(p);
  	bt_dev_dbg(hdev, "Fseq MBX timeout: 0x%8.8x", val);

-	val = get_unaligned_le32(skb_pull_data(skb, 4));
+	p = skb_pull_data(skb, 4);
+	if (!p)
+		goto malformed;
+	val = get_unaligned_le32(p);
  	bt_dev_dbg(hdev, "Fseq MBX ack: 0x%8.8x", val);

-	val = get_unaligned_le32(skb_pull_data(skb, 4));
+	p = skb_pull_data(skb, 4);
+	if (!p)
+		goto malformed;
+	val = get_unaligned_le32(p);
  	bt_dev_dbg(hdev, "Fseq CNVi id: 0x%8.8x", val);

-	val = get_unaligned_le32(skb_pull_data(skb, 4));
+	p = skb_pull_data(skb, 4);
+	if (!p)
+		goto malformed;
+	val = get_unaligned_le32(p);
  	bt_dev_dbg(hdev, "Fseq CNVr id: 0x%8.8x", val);

-	val = get_unaligned_le32(skb_pull_data(skb, 4));
+	p = skb_pull_data(skb, 4);
+	if (!p)
+		goto malformed;
+	val = get_unaligned_le32(p);
  	bt_dev_dbg(hdev, "Fseq Error handle: 0x%8.8x", val);

-	val = get_unaligned_le32(skb_pull_data(skb, 4));
+	p = skb_pull_data(skb, 4);
+	if (!p)
+		goto malformed;
+	val = get_unaligned_le32(p);
  	bt_dev_dbg(hdev, "Fseq Magic noalive indication: 0x%8.8x", val);

-	val = get_unaligned_le32(skb_pull_data(skb, 4));
+	p = skb_pull_data(skb, 4);
+	if (!p)
+		goto malformed;
+	val = get_unaligned_le32(p);
  	bt_dev_dbg(hdev, "Fseq OTP version: 0x%8.8x", val);

-	val = get_unaligned_le32(skb_pull_data(skb, 4));
+	p = skb_pull_data(skb, 4);
+	if (!p)
+		goto malformed;
+	val = get_unaligned_le32(p);
  	bt_dev_dbg(hdev, "Fseq MBX otp version: 0x%8.8x", val);

  	kfree_skb(skb);
+	return;
+
+malformed:
+	bt_dev_dbg(hdev, "Malformed packet received");
+	kfree_skb(skb);
  }
  EXPORT_SYMBOL_GPL(btintel_print_fseq_info);

-- 
2.43.0

RE: [v2] Bluetooth: btintel: Use skb_pull_data return for bounds check

[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/btintel.c:3356
error: drivers/bluetooth/btintel.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