| Message ID | YK+Yo6c1UuiACSZA@mwanda (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Bluetooth: hci_intel: prevent reads beyond the end of skb->data | expand |
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
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
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
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)
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(-)