Re: [PATCH] Bluetooth: hci_intel: prevent reads beyond the end of skb->data
From: Marcel Holtmann <marcel@holtmann.org>
Date: 2021-05-27 15:19:08
Also in:
kernel-janitors
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' bytesI think it will be hard to find people with this hardware. LnP devices are rare, but maybe someone will speak up here.
quoted hunk ↗ jump to hunk
I think there should be a different additional static checker warning for h4_recv_pkt structs like in this patch if you fail to specify a .hlen value? regards, dan carpenter --- drivers/bluetooth/hci_intel.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)diff --git a/drivers/bluetooth/hci_intel.c b/drivers/bluetooth/hci_intel.c index 7249b91d9b91..3e4bccacad9b 100644 --- a/drivers/bluetooth/hci_intel.c +++ b/drivers/bluetooth/hci_intel.c@@ -925,7 +925,7 @@ static int intel_recv_lpm(struct hci_dev *hdev, struct sk_buff *skb)switch (lpm->opcode) { case LPM_OP_TX_NOTIFY: - if (lpm->dlen < 1) { + if (lpm->dlen < 1 || skb->len < struct_size(lpm, data, 1)) { bt_dev_err(hu->hdev, "Invalid LPM notification packet"); break; }
This change looks fine to me and I would accept a patch for it.
quoted hunk ↗ jump to hunk
@@ -959,10 +959,10 @@ static int intel_recv_lpm(struct hci_dev *hdev, struct sk_buff *skb).maxlen = HCI_LPM_MAX_SIZE static const struct h4_recv_pkt intel_recv_pkts[] = { - { H4_RECV_ACL, .recv = hci_recv_frame }, - { H4_RECV_SCO, .recv = hci_recv_frame }, - { H4_RECV_EVENT, .recv = intel_recv_event }, - { INTEL_RECV_LPM, .recv = intel_recv_lpm }, + { 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
Regards
Marcel