Re: [PATCH] Bluetooth: hci_intel: prevent reads beyond the end of skb->data

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

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@kernel.org.  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