Re: [PATCH] Bluetooth: btusb: avoid NULL pointer defereference in skb_dequeue()

[Paul Menzel <pmenzel@molgen.mpg.de>]

[Cc: +Tim]

Dear En-Wei,


Thank you for the patch. There is a typo in the summary/title/subject:

dereference

Am 28.11.24 um 04:08 schrieb En-Wei Wu:
> The WCN7851 (0489:e0f3) Bluetooth controller supports firmware crash dump
> collection through devcoredump. During this process, the crash dump data
> is queued to a dump queue as skb for further processing.
> 
> A NULL pointer dereference occurs in skb_dequeue() when processing the
> dump queue due to improper return value handling:
> 
> [ 93.672166] Bluetooth: hci0: ACL memdump size(589824)
> 
> [ 93.672475] BUG: kernel NULL pointer dereference, address: 0000000000000008
> [ 93.672517] Workqueue: hci0 hci_devcd_rx [bluetooth]
> [ 93.672598] RIP: 0010:skb_dequeue+0x50/0x80
> 
> The issue stems from handle_dump_pkt_qca() returning the wrong value on
> success. It currently returns the value from hci_devcd_init() (0 on success),
> but callers expect > 0 to indicate successful dump handling. This causes
> hci_recv_frame() to free the skb while it's still queued for dump
> processing, leading to the NULL pointer dereference when hci_devcd_rx()
> tries to dequeue it.
> 
> Fix this by:
> 
> 1. Extracting dump packet detection into new is_dump_pkt_qca() function
> 2. Making handle_dump_pkt_qca() return 0 on success and negative errno
>     on failure, consistent with other kernel interfaces
> 
> This prevents premature skb freeing by ensuring proper handling of dump packets.

Re-flow this line for 75/72 characters per line?

How can I force the the firmware crash dump selection?

> Fixes: 20981ce2d5a5 ("Bluetooth: btusb: Add WCN6855 devcoredump support")
> Signed-off-by: En-Wei Wu <en-wei.wu@canonical.com>
> ---
>   drivers/bluetooth/btusb.c | 75 ++++++++++++++++++++++++---------------
>   1 file changed, 47 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index 279fe6c115fa..8926f8f60e5c 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -2930,22 +2930,16 @@ static void btusb_coredump_qca(struct hci_dev *hdev)
>   		bt_dev_err(hdev, "%s: triggle crash failed (%d)", __func__, err);
>   }
>   
> -/*
> - * ==0: not a dump pkt.
> - * < 0: fails to handle a dump pkt
> - * > 0: otherwise.
> - */
> +/* Return: 0 on success, negative errno on failure. */
>   static int handle_dump_pkt_qca(struct hci_dev *hdev, struct sk_buff *skb)
>   {
> -	int ret = 1;
> +	int ret = 0;
>   	u8 pkt_type;
>   	u8 *sk_ptr;
>   	unsigned int sk_len;
>   	u16 seqno;
>   	u32 dump_size;
>   
> -	struct hci_event_hdr *event_hdr;
> -	struct hci_acl_hdr *acl_hdr;
>   	struct qca_dump_hdr *dump_hdr;
>   	struct btusb_data *btdata = hci_get_drvdata(hdev);
>   	struct usb_device *udev = btdata->udev;
> @@ -2955,30 +2949,14 @@ static int handle_dump_pkt_qca(struct hci_dev *hdev, struct sk_buff *skb)
>   	sk_len = skb->len;
>   
>   	if (pkt_type == HCI_ACLDATA_PKT) {
> -		acl_hdr = hci_acl_hdr(skb);
> -		if (le16_to_cpu(acl_hdr->handle) != QCA_MEMDUMP_ACL_HANDLE)
> -			return 0;
>   		sk_ptr += HCI_ACL_HDR_SIZE;
>   		sk_len -= HCI_ACL_HDR_SIZE;
> -		event_hdr = (struct hci_event_hdr *)sk_ptr;
> -	} else {
> -		event_hdr = hci_event_hdr(skb);
>   	}
>   
> -	if ((event_hdr->evt != HCI_VENDOR_PKT)
> -		|| (event_hdr->plen != (sk_len - HCI_EVENT_HDR_SIZE)))
> -		return 0;
> -
>   	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 0;
> -
> -	/*it is dump pkt now*/
>   	seqno = le16_to_cpu(dump_hdr->seqno);
>   	if (seqno == 0) {
>   		set_bit(BTUSB_HW_SSR_ACTIVE, &btdata->flags);
> @@ -3052,17 +3030,58 @@ static int handle_dump_pkt_qca(struct hci_dev *hdev, struct sk_buff *skb)
>   	return ret;
>   }
>   
> +/* Return: true if packet is a dump packet, false otherwise. */
> +static bool is_dump_pkt_qca(struct hci_dev *hdev, struct sk_buff *skb)
> +{
> +	u8 pkt_type;
> +	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;
> +
> +	pkt_type = hci_skb_pkt_type(skb);
> +	sk_ptr = skb->data;
> +	sk_len = skb->len;
> +
> +	if (pkt_type == HCI_ACLDATA_PKT) {
> +		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;
> +	} else {
> +		event_hdr = hci_event_hdr(skb);
> +	}
> +
> +	if ((event_hdr->evt != HCI_VENDOR_PKT)
> +		|| (event_hdr->plen != (sk_len - HCI_EVENT_HDR_SIZE)))
> +		return false;
> +
> +	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;
> +
> +	return true;
> +}

Add a blank line here?

>   static int btusb_recv_acl_qca(struct hci_dev *hdev, struct sk_buff *skb)
>   {
> -	if (handle_dump_pkt_qca(hdev, skb))
> -		return 0;
> +	if (is_dump_pkt_qca(hdev, skb))
> +		return handle_dump_pkt_qca(hdev, skb);
>   	return hci_recv_frame(hdev, skb);
>   }
>   
>   static int btusb_recv_evt_qca(struct hci_dev *hdev, struct sk_buff *skb)
>   {
> -	if (handle_dump_pkt_qca(hdev, skb))
> -		return 0;
> +	if (is_dump_pkt_qca(hdev, skb))
> +		return handle_dump_pkt_qca(hdev, skb);
>   	return hci_recv_frame(hdev, skb);
>   }

The rest looks good.


Kind regards,

Paul