On Thu, May 27, 2021 at 05:19:04PM +0200, Marcel Holtmann wrote:
> Hi Dan,
>
> > There doesn't appear to be any checks to ensure that skb->data is large
> > enough in these functions. For most of these, if we specify a header
> > length, then h4_recv_buf() will ensure that all packets are at least the
> > minimum length. The intel_recv_lpm() function needs an additional
> > check for LPM_OP_TX_NOTIFY packets.
> >
> > Fixes: ca93cee5a56e ("Bluetooth: hci_uart: Add basic support for Intel Lightning Peak devices")
> >
> > No signed-off-by because I can't test this and just wanted to collect
> > feedback. This is part of a static checker warning because someone
> > reported the hci_event.c read overflows to security@xxxxxxxxxx. This
> > stuff is quite complicated for static checkers of course and I don't
> > understand all the rules yet. Right now I have about 2000 warnings
> > that look like this:
> >
> > drivers/bluetooth/hci_intel.c:877 intel_recv_event() warn: assignment assumes 'skb->len' is '2' bytes
> > drivers/bluetooth/hci_intel.c:922 intel_recv_lpm() warn: assignment assumes 'skb->len' is '2' bytes
> > drivers/bluetooth/hci_intel.c:1028 intel_dequeue() warn: assignment assumes 'skb->len' is '3' bytes
>
> I think it will be hard to find people with this hardware. LnP devices are rare, but maybe someone will speak up here.
>
It's easier to fix all the bugs than it is to try figure out if anyone
has the hardware. Plus if no one has the hardware then I will get the
credit for fixing a security bug with none of the risk of breaking
someone's system. ;)
[ snip ]
> > + { H4_RECV_ACL, .recv = hci_recv_frame, .hlen = sizeof(struct bt_skb_cb) },
> > + { H4_RECV_SCO, .recv = hci_recv_frame, .hlen = sizeof(struct bt_skb_cb) },
> > + { H4_RECV_EVENT, .recv = intel_recv_event, .hlen = sizeof(struct hci_event_hdr) },
> > + { INTEL_RECV_LPM, .recv = intel_recv_lpm, .hlen = sizeof(struct hci_lpm_pkt) },
>
> This part I do not understand, all the H4_RECV_* and even INTEL_RECV_* provide the hlen. So I have no idea what your change is doing here. And the two for H4_RECV_{ACL,SCO} are actually wrong. In case you wonder this is how they are defined:
>
> #define H4_RECV_ACL \
> .type = HCI_ACLDATA_PKT, \
> .hlen = HCI_ACL_HDR_SIZE, \
> .loff = 2, \
> .lsize = 2, \
> .maxlen = HCI_MAX_FRAME_SIZE \
>
> #define H4_RECV_SCO \
> .type = HCI_SCODATA_PKT, \
> .hlen = HCI_SCO_HDR_SIZE, \
> .loff = 2, \
> .lsize = 1, \
> .maxlen = HCI_MAX_SCO_SIZE
>
> #define H4_RECV_EVENT \
> .type = HCI_EVENT_PKT, \
> .hlen = HCI_EVENT_HDR_SIZE, \
> .loff = 1, \
> .lsize = 1, \
> .maxlen = HCI_MAX_EVENT_SIZE
Oh... Crap... I've been banging my head into the wall trying to figure
out why I couldn't make Smatch generate a warning for this. But now
when I remove the macro it does.
drivers/bluetooth/hci_intel.c:961 (null)() struct member not set 'intel_recv_pkts[0]->hlen'
It's embarrassing how long I have spend trying to figure out why it
said it was already initialized to non-zero...
regards,
dan carpenter
