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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Dear Paul,

Thanks for the quick response.

> How can I force the the firmware crash dump selection?
I haven't found a way to force the firmware crashing dump. In our
cases, we often encountered the hardware error and it would
automatically do the crash dump process.

Here is the log of crash dump after applying this patch
[ 178.424131] Bluetooth: hci0: ACL memdump size(589824)
[ 179.028136] Bluetooth: hci0: memdump done: pkts(2418), total(589824)
[ 179.036315] Bluetooth: hci0: hardware error 0x85

As we can see, the NULL pointer dereference error is gone.

Best regards,
En-Wei.

On Thu, 28 Nov 2024 at 13:55, Paul Menzel <pmenzel@xxxxxxxxxxxxx> wrote:
>
> [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@xxxxxxxxxxxxx>
> > ---
> >   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




[Index of Archives]     [Bluez Devel]     [Linux Wireless Networking]     [Linux Wireless Personal Area Networking]     [Linux ATH6KL]     [Linux USB Devel]     [Linux Media Drivers]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Big List of Linux Books]

  Powered by Linux