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

[Marcel Holtmann <marcel@xxxxxxxxxxxx>]

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.

> 
> 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.

> @@ -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