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

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

[Dan Carpenter]

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 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;
 		}
@@ -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) },
 };
 
 static int intel_recv(struct hci_uart *hu, const void *data, int count)
-- 
2.30.2

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

[Marcel Holtmann]

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.

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

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

[Dan Carpenter]

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