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

On Thu, May 27, 2021 at 05:19:04PM +0200, Marcel Holtmann wrote:

Hi Dan,

quoted

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 ]

quoted

+	{ 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