[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: Bluetooth: hci_intel: prevent reads beyond the end of skb->data

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 3953 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=489559

---Test result---

Test Summary:
CheckPatch                    FAIL      0.75 seconds
GitLint                       FAIL      0.14 seconds
BuildKernel                   PASS      621.45 seconds
TestRunner: Setup             PASS      399.93 seconds
TestRunner: l2cap-tester      PASS      3.03 seconds
TestRunner: bnep-tester       PASS      2.14 seconds
TestRunner: mgmt-tester       PASS      28.10 seconds
TestRunner: rfcomm-tester     PASS      2.30 seconds
TestRunner: sco-tester        PASS      2.23 seconds
TestRunner: smp-tester        PASS      2.36 seconds
TestRunner: userchan-tester   PASS      2.13 seconds

Details
##############################
Test: CheckPatch - FAIL - 0.75 seconds
Run checkpatch.pl script with rule in .checkpatch.conf
Bluetooth: hci_intel: prevent reads beyond the end of skb->data
WARNING: Unknown commit id 'ca93cee5a56e', maybe rebased or not pulled?
#13: 
Fixes: ca93cee5a56e ("Bluetooth: hci_uart: Add basic support for Intel Lightning Peak devices")

ERROR: Missing Signed-off-by: line(s)

total: 1 errors, 1 warnings, 22 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

"[PATCH] Bluetooth: hci_intel: prevent reads beyond the end of" has style problems, please review.

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


##############################
Test: GitLint - FAIL - 0.14 seconds
Run gitlint with rule in .gitlint
Bluetooth: hci_intel: prevent reads beyond the end of skb->data
9: B1 Line exceeds max length (95>80): "Fixes: ca93cee5a56e ("Bluetooth: hci_uart: Add basic support for Intel Lightning Peak devices")"
18: B1 Line exceeds max length (101>80): "drivers/bluetooth/hci_intel.c:877 intel_recv_event() warn: assignment assumes 'skb->len' is '2' bytes"
19: B1 Line exceeds max length (99>80): "drivers/bluetooth/hci_intel.c:922 intel_recv_lpm() warn: assignment assumes 'skb->len' is '2' bytes"
20: B1 Line exceeds max length (99>80): "drivers/bluetooth/hci_intel.c:1028 intel_dequeue() warn: assignment assumes 'skb->len' is '3' bytes"


##############################
Test: BuildKernel - PASS - 621.45 seconds
Build Kernel with minimal configuration supports Bluetooth


##############################
Test: TestRunner: Setup - PASS - 399.93 seconds
Setup environment for running Test Runner


##############################
Test: TestRunner: l2cap-tester - PASS - 3.03 seconds
Run test-runner with l2cap-tester
Total: 40, Passed: 40 (100.0%), Failed: 0, Not Run: 0

##############################
Test: TestRunner: bnep-tester - PASS - 2.14 seconds
Run test-runner with bnep-tester
Total: 1, Passed: 1 (100.0%), Failed: 0, Not Run: 0

##############################
Test: TestRunner: mgmt-tester - PASS - 28.10 seconds
Run test-runner with mgmt-tester
Total: 433, Passed: 420 (97.0%), Failed: 0, Not Run: 13

##############################
Test: TestRunner: rfcomm-tester - PASS - 2.30 seconds
Run test-runner with rfcomm-tester
Total: 9, Passed: 9 (100.0%), Failed: 0, Not Run: 0

##############################
Test: TestRunner: sco-tester - PASS - 2.23 seconds
Run test-runner with sco-tester
Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0

##############################
Test: TestRunner: smp-tester - PASS - 2.36 seconds
Run test-runner with smp-tester
Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0

##############################
Test: TestRunner: userchan-tester - PASS - 2.13 seconds
Run test-runner with userchan-tester
Total: 3, Passed: 3 (100.0%), Failed: 0, Not Run: 0



---
Regards,
Linux Bluetooth


[-- Attachment #2: l2cap-tester.log --]
[-- Type: application/octet-stream, Size: 44350 bytes --]

[-- Attachment #3: bnep-tester.log --]
[-- Type: application/octet-stream, Size: 3556 bytes --]

[-- Attachment #4: mgmt-tester.log --]
[-- Type: application/octet-stream, Size: 568393 bytes --]

[-- Attachment #5: rfcomm-tester.log --]
[-- Type: application/octet-stream, Size: 11677 bytes --]

[-- Attachment #6: sco-tester.log --]
[-- Type: application/octet-stream, Size: 9912 bytes --]

[-- Attachment #7: smp-tester.log --]
[-- Type: application/octet-stream, Size: 11823 bytes --]

[-- Attachment #8: userchan-tester.log --]
[-- Type: application/octet-stream, Size: 5451 bytes --]

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