Re: [Patch net v2 2/3] bluetooth: validate HCI_EV_LE_META packet carefully

[Dan Carpenter <dan.carpenter@oracle.com>]

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