[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Thu, 4 Apr 2019 11:34:49 +0300
From: Dan Carpenter <dan.carpenter@...cle.com>
To: Cong Wang <xiyou.wangcong@...il.com>
Cc: netdev@...r.kernel.org, linux-bluetooth@...r.kernel.org,
Marcel Holtmann <marcel@...tmann.org>,
Johan Hedberg <johan.hedberg@...il.com>,
Tomas Bortoli <tomasbortoli@...il.com>
Subject: Re: [Patch net v2 2/3] bluetooth: validate HCI_EV_LE_META packet
carefully
On Wed, Apr 03, 2019 at 04:08:34PM -0700, Cong Wang wrote:
> static void hci_le_ext_adv_report_evt(struct hci_dev *hdev, struct sk_buff *skb)
> {
> - u8 num_reports = skb->data[0];
> - void *ptr = &skb->data[1];
> + unsigned int len;
> + u8 num_reports;
> +
> + if (unlikely(!pskb_may_pull(skb, 1)))
> + return;
> + num_reports = skb->data[0];
> + len = 1;
>
> hci_dev_lock(hdev);
>
> while (num_reports--) {
> - struct hci_ev_le_ext_adv_report *ev = ptr;
> + struct hci_ev_le_ext_adv_report *ev;
> u8 legacy_evt_type;
> u16 evt_type;
> + u8 ev_len;
> +
> + if (unlikely(!pskb_may_pull(skb, len + sizeof(*ev))))
> + break;
> + ev = (void *)skb->data + len;
> + ev_len = ev->length + 1;
The "+ 1" is a bug. It was discussed in a different thread. Jaganath
says he has sent a patch to fix it already.
Probably it worked in testing because the num_reports was always 1, but
now when we add this condition with the "+ 1" bug at the start of the
loop the the the num_reports == 1 case will be broken as well.
Tomas and I should get Reported-by tags for parts of this patch.
> + if (unlikely(!pskb_may_pull(skb, len + sizeof(*ev) + ev_len)))
> + break;
> + ev = (void *)skb->data + len;
>
> evt_type = __le16_to_cpu(ev->evt_type);
> legacy_evt_type = ext_evt_type_to_legacy(evt_type);
> @@ -5601,7 +5645,7 @@ static void hci_le_ext_adv_report_evt(struct hci_dev *hdev, struct sk_buff *skb)
> ev->data, ev->length);
> }
>
> - ptr += sizeof(*ev) + ev->length + 1;
> + len += sizeof(*ev) + ev_len;
> }
>
> hci_dev_unlock(hdev);
regards,
dan carpenter
Powered by blists - more mailing lists