diff mbox series

[v4,2/2] Bluetooth: btusb: use skb_pull to avoid unsafe access in QCA dump handling

Message ID 20250421130038.34998-3-en-wei.wu@canonical.com
State New
Headers show
Series btusb: fix NULL pointer dereference in QCA devcoredump handling | expand

Commit Message

En-Wei Wu April 21, 2025, 1 p.m. UTC 
Use skb_pull() and skb_pull_data() to safely parse QCA dump packets.

This avoids direct pointer math on skb->data, which could lead to
invalid access if the packet is shorter than expected.

Signed-off-by: En-Wei Wu <en-wei.wu@canonical.com>
---
 drivers/bluetooth/btusb.c | 98 ++++++++++++++++-----------------------
 1 file changed, 41 insertions(+), 57 deletions(-)

Comments

Luiz Augusto von Dentz April 21, 2025, 3:29 p.m. UTC | #1 
Hi En-Wei,

On Mon, Apr 21, 2025 at 9:00 AM En-Wei Wu <en-wei.wu@canonical.com> wrote:
>
> Use skb_pull() and skb_pull_data() to safely parse QCA dump packets.
>
> This avoids direct pointer math on skb->data, which could lead to
> invalid access if the packet is shorter than expected.
>
> Signed-off-by: En-Wei Wu <en-wei.wu@canonical.com>
> ---
>  drivers/bluetooth/btusb.c | 98 ++++++++++++++++-----------------------
>  1 file changed, 41 insertions(+), 57 deletions(-)
>
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index f905780fcdea..031292ab766f 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -3017,8 +3017,6 @@ static int handle_dump_pkt_qca(struct hci_dev *hdev, struct sk_buff *skb)
>  {
>         int ret = 0;
>         u8 pkt_type;
> -       u8 *sk_ptr;
> -       unsigned int sk_len;
>         u16 seqno;
>         u32 dump_size;
>
> @@ -3027,18 +3025,8 @@ static int handle_dump_pkt_qca(struct hci_dev *hdev, struct sk_buff *skb)
>         struct usb_device *udev = btdata->udev;
>
>         pkt_type = hci_skb_pkt_type(skb);
> -       sk_ptr = skb->data;
> -       sk_len = skb->len;
> +       dump_hdr = (struct qca_dump_hdr *)skb->data;
>
> -       if (pkt_type == HCI_ACLDATA_PKT) {
> -               sk_ptr += HCI_ACL_HDR_SIZE;
> -               sk_len -= HCI_ACL_HDR_SIZE;
> -       }
> -
> -       sk_ptr += HCI_EVENT_HDR_SIZE;
> -       sk_len -= HCI_EVENT_HDR_SIZE;
> -
> -       dump_hdr = (struct qca_dump_hdr *)sk_ptr;
>         seqno = le16_to_cpu(dump_hdr->seqno);
>         if (seqno == 0) {
>                 set_bit(BTUSB_HW_SSR_ACTIVE, &btdata->flags);
> @@ -3058,16 +3046,15 @@ static int handle_dump_pkt_qca(struct hci_dev *hdev, struct sk_buff *skb)
>
>                 btdata->qca_dump.ram_dump_size = dump_size;
>                 btdata->qca_dump.ram_dump_seqno = 0;
> -               sk_ptr += offsetof(struct qca_dump_hdr, data0);
> -               sk_len -= offsetof(struct qca_dump_hdr, data0);
> +
> +               skb_pull(skb, offsetof(struct qca_dump_hdr, data0));
>
>                 usb_disable_autosuspend(udev);
>                 bt_dev_info(hdev, "%s memdump size(%u)\n",
>                             (pkt_type == HCI_ACLDATA_PKT) ? "ACL" : "event",
>                             dump_size);
>         } else {
> -               sk_ptr += offsetof(struct qca_dump_hdr, data);
> -               sk_len -= offsetof(struct qca_dump_hdr, data);
> +               skb_pull(skb, offsetof(struct qca_dump_hdr, data));
>         }
>
>         if (!btdata->qca_dump.ram_dump_size) {
> @@ -3087,7 +3074,6 @@ static int handle_dump_pkt_qca(struct hci_dev *hdev, struct sk_buff *skb)
>                 return ret;
>         }
>
> -       skb_pull(skb, skb->len - sk_len);
>         hci_devcd_append(hdev, skb);
>         btdata->qca_dump.ram_dump_seqno++;
>         if (seqno == QCA_LAST_SEQUENCE_NUM) {
> @@ -3115,67 +3101,65 @@ static int handle_dump_pkt_qca(struct hci_dev *hdev, struct sk_buff *skb)
>  /* Return: true if the ACL packet is a dump packet, false otherwise. */
>  static bool acl_pkt_is_dump_qca(struct hci_dev *hdev, struct sk_buff *skb)
>  {
> -       u8 *sk_ptr;
> -       unsigned int sk_len;
> -
>         struct hci_event_hdr *event_hdr;
>         struct hci_acl_hdr *acl_hdr;
>         struct qca_dump_hdr *dump_hdr;
> +       void *orig_data;
> +       unsigned int orig_len;
>
> -       sk_ptr = skb->data;
> -       sk_len = skb->len;
> +       orig_data = skb->data;
> +       orig_len = skb->len;
>
> -       acl_hdr = hci_acl_hdr(skb);
> -       if (le16_to_cpu(acl_hdr->handle) != QCA_MEMDUMP_ACL_HANDLE)
> -               return false;
> -       sk_ptr += HCI_ACL_HDR_SIZE;
> -       sk_len -= HCI_ACL_HDR_SIZE;
> -       event_hdr = (struct hci_event_hdr *)sk_ptr;
> +       acl_hdr = skb_pull_data(skb, sizeof(*acl_hdr));
> +       if (!acl_hdr || (le16_to_cpu(acl_hdr->handle) != QCA_MEMDUMP_ACL_HANDLE))
> +               goto restore_return;
>
> -       if ((event_hdr->evt != HCI_VENDOR_PKT)
> -               || (event_hdr->plen != (sk_len - HCI_EVENT_HDR_SIZE)))
> -               return false;
> +       event_hdr = skb_pull_data(skb, sizeof(*event_hdr));
> +       if (!event_hdr || (event_hdr->evt != HCI_VENDOR_PKT))
> +               goto restore_return;
>
> -       sk_ptr += HCI_EVENT_HDR_SIZE;
> -       sk_len -= HCI_EVENT_HDR_SIZE;
> -
> -       dump_hdr = (struct qca_dump_hdr *)sk_ptr;
> -       if ((sk_len < offsetof(struct qca_dump_hdr, data))
> -               || (dump_hdr->vse_class != QCA_MEMDUMP_VSE_CLASS)
> -           || (dump_hdr->msg_type != QCA_MEMDUMP_MSG_TYPE))
> -               return false;
> +       dump_hdr = (struct qca_dump_hdr *)skb->data;
> +       if ((skb->len < sizeof(*dump_hdr)) ||
> +          (dump_hdr->vse_class != QCA_MEMDUMP_VSE_CLASS) ||
> +          (dump_hdr->msg_type != QCA_MEMDUMP_MSG_TYPE))
> +               goto restore_return;
>
>         return true;
> +
> +restore_return:
> +       skb->data = orig_data;
> +       skb->len = orig_len;
> +       return false;
>  }
>
>  /* Return: true if the event packet is a dump packet, false otherwise. */
>  static bool evt_pkt_is_dump_qca(struct hci_dev *hdev, struct sk_buff *skb)
>  {
> -       u8 *sk_ptr;
> -       unsigned int sk_len;
> -
>         struct hci_event_hdr *event_hdr;
>         struct qca_dump_hdr *dump_hdr;
> +       void *orig_data;
> +       unsigned int orig_len;
>
> -       sk_ptr = skb->data;
> -       sk_len = skb->len;
> +       orig_data = skb->data;
> +       orig_len = skb->len;
>
> -       event_hdr = hci_event_hdr(skb);
> +       event_hdr = skb_pull_data(skb, sizeof(*event_hdr));
> +       if (!event_hdr || (event_hdr->evt != HCI_VENDOR_PKT))
> +               goto restore_return;
>
> -       if ((event_hdr->evt != HCI_VENDOR_PKT)
> -               || (event_hdr->plen != (sk_len - HCI_EVENT_HDR_SIZE)))
> -               return false;
> +       dump_hdr = (struct qca_dump_hdr *)skb->data;
> +       if ((skb->len < sizeof(*dump_hdr)) ||
> +          (dump_hdr->vse_class != QCA_MEMDUMP_VSE_CLASS) ||
> +          (dump_hdr->msg_type != QCA_MEMDUMP_MSG_TYPE))
> +               goto restore_return;
>
> -       sk_ptr += HCI_EVENT_HDR_SIZE;
> -       sk_len -= HCI_EVENT_HDR_SIZE;
> +       return true;
>
> -       dump_hdr = (struct qca_dump_hdr *)sk_ptr;
> -       if ((sk_len < offsetof(struct qca_dump_hdr, data))
> -               || (dump_hdr->vse_class != QCA_MEMDUMP_VSE_CLASS)
> -           || (dump_hdr->msg_type != QCA_MEMDUMP_MSG_TYPE))
> -               return false;
> +restore_return:
> +       skb->data = orig_data;
> +       skb->len = orig_len;
> +       return false;
>
> -       return true;
>  }
>
>  static int btusb_recv_acl_qca(struct hci_dev *hdev, struct sk_buff *skb)
> --
> 2.43.0

While I agree using the likes of skb_pull is a much safer way to parse
these packets Im not so sure it is safe to restore the skb->data and
skb->len like that, perhaps we should be using skb_clone and
skb_unclone for example.
diff mbox series

Patch

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index f905780fcdea..031292ab766f 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -3017,8 +3017,6 @@  static int handle_dump_pkt_qca(struct hci_dev *hdev, struct sk_buff *skb)
 {
 	int ret = 0;
 	u8 pkt_type;
-	u8 *sk_ptr;
-	unsigned int sk_len;
 	u16 seqno;
 	u32 dump_size;
 
@@ -3027,18 +3025,8 @@  static int handle_dump_pkt_qca(struct hci_dev *hdev, struct sk_buff *skb)
 	struct usb_device *udev = btdata->udev;
 
 	pkt_type = hci_skb_pkt_type(skb);
-	sk_ptr = skb->data;
-	sk_len = skb->len;
+	dump_hdr = (struct qca_dump_hdr *)skb->data;
 
-	if (pkt_type == HCI_ACLDATA_PKT) {
-		sk_ptr += HCI_ACL_HDR_SIZE;
-		sk_len -= HCI_ACL_HDR_SIZE;
-	}
-
-	sk_ptr += HCI_EVENT_HDR_SIZE;
-	sk_len -= HCI_EVENT_HDR_SIZE;
-
-	dump_hdr = (struct qca_dump_hdr *)sk_ptr;
 	seqno = le16_to_cpu(dump_hdr->seqno);
 	if (seqno == 0) {
 		set_bit(BTUSB_HW_SSR_ACTIVE, &btdata->flags);
@@ -3058,16 +3046,15 @@  static int handle_dump_pkt_qca(struct hci_dev *hdev, struct sk_buff *skb)
 
 		btdata->qca_dump.ram_dump_size = dump_size;
 		btdata->qca_dump.ram_dump_seqno = 0;
-		sk_ptr += offsetof(struct qca_dump_hdr, data0);
-		sk_len -= offsetof(struct qca_dump_hdr, data0);
+
+		skb_pull(skb, offsetof(struct qca_dump_hdr, data0));
 
 		usb_disable_autosuspend(udev);
 		bt_dev_info(hdev, "%s memdump size(%u)\n",
 			    (pkt_type == HCI_ACLDATA_PKT) ? "ACL" : "event",
 			    dump_size);
 	} else {
-		sk_ptr += offsetof(struct qca_dump_hdr, data);
-		sk_len -= offsetof(struct qca_dump_hdr, data);
+		skb_pull(skb, offsetof(struct qca_dump_hdr, data));
 	}
 
 	if (!btdata->qca_dump.ram_dump_size) {
@@ -3087,7 +3074,6 @@  static int handle_dump_pkt_qca(struct hci_dev *hdev, struct sk_buff *skb)
 		return ret;
 	}
 
-	skb_pull(skb, skb->len - sk_len);
 	hci_devcd_append(hdev, skb);
 	btdata->qca_dump.ram_dump_seqno++;
 	if (seqno == QCA_LAST_SEQUENCE_NUM) {
@@ -3115,67 +3101,65 @@  static int handle_dump_pkt_qca(struct hci_dev *hdev, struct sk_buff *skb)
 /* Return: true if the ACL packet is a dump packet, false otherwise. */
 static bool acl_pkt_is_dump_qca(struct hci_dev *hdev, struct sk_buff *skb)
 {
-	u8 *sk_ptr;
-	unsigned int sk_len;
-
 	struct hci_event_hdr *event_hdr;
 	struct hci_acl_hdr *acl_hdr;
 	struct qca_dump_hdr *dump_hdr;
+	void *orig_data;
+	unsigned int orig_len;
 
-	sk_ptr = skb->data;
-	sk_len = skb->len;
+	orig_data = skb->data;
+	orig_len = skb->len;
 
-	acl_hdr = hci_acl_hdr(skb);
-	if (le16_to_cpu(acl_hdr->handle) != QCA_MEMDUMP_ACL_HANDLE)
-		return false;
-	sk_ptr += HCI_ACL_HDR_SIZE;
-	sk_len -= HCI_ACL_HDR_SIZE;
-	event_hdr = (struct hci_event_hdr *)sk_ptr;
+	acl_hdr = skb_pull_data(skb, sizeof(*acl_hdr));
+	if (!acl_hdr || (le16_to_cpu(acl_hdr->handle) != QCA_MEMDUMP_ACL_HANDLE))
+		goto restore_return;
 
-	if ((event_hdr->evt != HCI_VENDOR_PKT)
-		|| (event_hdr->plen != (sk_len - HCI_EVENT_HDR_SIZE)))
-		return false;
+	event_hdr = skb_pull_data(skb, sizeof(*event_hdr));
+	if (!event_hdr || (event_hdr->evt != HCI_VENDOR_PKT))
+		goto restore_return;
 
-	sk_ptr += HCI_EVENT_HDR_SIZE;
-	sk_len -= HCI_EVENT_HDR_SIZE;
-
-	dump_hdr = (struct qca_dump_hdr *)sk_ptr;
-	if ((sk_len < offsetof(struct qca_dump_hdr, data))
-		|| (dump_hdr->vse_class != QCA_MEMDUMP_VSE_CLASS)
-	    || (dump_hdr->msg_type != QCA_MEMDUMP_MSG_TYPE))
-		return false;
+	dump_hdr = (struct qca_dump_hdr *)skb->data;
+	if ((skb->len < sizeof(*dump_hdr)) ||
+	   (dump_hdr->vse_class != QCA_MEMDUMP_VSE_CLASS) ||
+	   (dump_hdr->msg_type != QCA_MEMDUMP_MSG_TYPE))
+		goto restore_return;
 
 	return true;
+
+restore_return:
+	skb->data = orig_data;
+	skb->len = orig_len;
+	return false;
 }
 
 /* Return: true if the event packet is a dump packet, false otherwise. */
 static bool evt_pkt_is_dump_qca(struct hci_dev *hdev, struct sk_buff *skb)
 {
-	u8 *sk_ptr;
-	unsigned int sk_len;
-
 	struct hci_event_hdr *event_hdr;
 	struct qca_dump_hdr *dump_hdr;
+	void *orig_data;
+	unsigned int orig_len;
 
-	sk_ptr = skb->data;
-	sk_len = skb->len;
+	orig_data = skb->data;
+	orig_len = skb->len;
 
-	event_hdr = hci_event_hdr(skb);
+	event_hdr = skb_pull_data(skb, sizeof(*event_hdr));
+	if (!event_hdr || (event_hdr->evt != HCI_VENDOR_PKT))
+		goto restore_return;
 
-	if ((event_hdr->evt != HCI_VENDOR_PKT)
-		|| (event_hdr->plen != (sk_len - HCI_EVENT_HDR_SIZE)))
-		return false;
+	dump_hdr = (struct qca_dump_hdr *)skb->data;
+	if ((skb->len < sizeof(*dump_hdr)) ||
+	   (dump_hdr->vse_class != QCA_MEMDUMP_VSE_CLASS) ||
+	   (dump_hdr->msg_type != QCA_MEMDUMP_MSG_TYPE))
+		goto restore_return;
 
-	sk_ptr += HCI_EVENT_HDR_SIZE;
-	sk_len -= HCI_EVENT_HDR_SIZE;
+	return true;
 
-	dump_hdr = (struct qca_dump_hdr *)sk_ptr;
-	if ((sk_len < offsetof(struct qca_dump_hdr, data))
-		|| (dump_hdr->vse_class != QCA_MEMDUMP_VSE_CLASS)
-	    || (dump_hdr->msg_type != QCA_MEMDUMP_MSG_TYPE))
-		return false;
+restore_return:
+	skb->data = orig_data;
+	skb->len = orig_len;
+	return false;
 
-	return true;
 }
 
 static int btusb_recv_acl_qca(struct hci_dev *hdev, struct sk_buff *skb)