netdev - Re: [Patch net v2 2/3] bluetooth: validate HCI_EV_LE

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 5 Apr 2019 10:26:49 -0700
From:   Cong Wang <xiyou.wangcong@...il.com>
To:     Dan Carpenter <dan.carpenter@...cle.com>
Cc:     Linux Kernel Network Developers <netdev@...r.kernel.org>,
        linux-bluetooth <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 Thu, Apr 4, 2019 at 1:35 AM Dan Carpenter <dan.carpenter@...cle.com> wrote:
>
> 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.

It could be, at least it is not a bug introduced by my patch therefore
I have no obligation to fix it?

I am happy to rebase if that fix is merged before mine, like for any other
changes, if this is what you are trying to say.

>
> Tomas and I should get Reported-by tags for parts of this patch.


Sure, sorry for missing it, as I got reports from syzbot directly and
before you reported it. Will add it in v3.

Thanks.