| Message ID | tencent_29BA32BBF933AC9EDA1B074B621BEF259308@qq.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Bluetooth: hci_core: remove acl hdr handle error message | expand |
| Context | Check | Description |
|---|---|---|
| tedd_an/pre-ci_am | success | Success |
| tedd_an/CheckPatch | warning | WARNING: Prefer a maximum 75 chars per line (possible unwrapped commit description?) #84: l2cap_do_send(), and ultimately called hci_add_acl_hdr() to set hdr->handle. WARNING: The commit message has 'syzkaller', perhaps it also needs a 'Fixes:' tag? total: 0 errors, 2 warnings, 0 checks, 9 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. /github/workspace/src/src/13826220.patch has style problems, please review. NOTE: Ignored message types: UNKNOWN_COMMIT_ID NOTE: If any of the errors are false positives, please report them to the maintainer, see CHECKPATCH in MAINTAINERS. |
| tedd_an/GitLint | success | Gitlint PASS |
| tedd_an/SubjectPrefix | success | Gitlint PASS |
| tedd_an/BuildKernel | success | BuildKernel PASS |
| tedd_an/CheckAllWarning | success | CheckAllWarning PASS |
| tedd_an/CheckSparse | success | CheckSparse PASS |
Hi Edward, On Tue, Oct 8, 2024 at 6:47 AM Edward Adam Davis <eadavis@qq.com> wrote: > > Syzbot reported a uninit-value in hci_rx_work.This is because l2cap didn't > execute the corresponding connection request to call l2cap_send_cmd() or > l2cap_do_send(), and ultimately called hci_add_acl_hdr() to set hdr->handle. What are you talking about here, what these functions have to do with a local handle variable? > Therefore, when calling the thread callback function hci_rx_work() to call > hci_acldata_packet, hdr->handle should not be used directly. It is not being used directly, the handle is a local variable which get assigned: handle = __le16_to_cpu(hdr->handle); If what you are saying is that there is no guarantee that skb->len >= HCI_ACL_HDR_SIZE then we probably want to replace skb_pull with skb_pull_data. > Reported-and-tested-by: syzbot+6ea290ba76d8c1eb1ac2@syzkaller.appspotmail.com > Closes: https://syzkaller.appspot.com/bug?extid=6ea290ba76d8c1eb1ac2 > Signed-off-by: Edward Adam Davis <eadavis@qq.com> > --- > net/bluetooth/hci_core.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c > index d6976db02c06..20605a7f3f4e 100644 > --- a/net/bluetooth/hci_core.c > +++ b/net/bluetooth/hci_core.c > @@ -3791,8 +3791,7 @@ static void hci_acldata_packet(struct hci_dev *hdev, struct sk_buff *skb) > l2cap_recv_acldata(conn, skb, flags); > return; > } else { > - bt_dev_err(hdev, "ACL packet for unknown connection handle %d", > - handle); > + bt_dev_err(hdev, "ACL packet for unknown connection handle"); > } > > kfree_skb(skb); > -- > 2.43.0 >
On Tue, Oct 8, 2024 at 10:12 AM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > > Hi Edward, > > On Tue, Oct 8, 2024 at 6:47 AM Edward Adam Davis <eadavis@qq.com> wrote: > > > > Syzbot reported a uninit-value in hci_rx_work.This is because l2cap didn't > > execute the corresponding connection request to call l2cap_send_cmd() or > > l2cap_do_send(), and ultimately called hci_add_acl_hdr() to set hdr->handle. > > What are you talking about here, what these functions have to do with > a local handle variable? > > > Therefore, when calling the thread callback function hci_rx_work() to call > > hci_acldata_packet, hdr->handle should not be used directly. > > It is not being used directly, the handle is a local variable which > get assigned: > > handle = __le16_to_cpu(hdr->handle); > > If what you are saying is that there is no guarantee that skb->len >= > HCI_ACL_HDR_SIZE then we probably want to replace skb_pull with > skb_pull_data. > > > Reported-and-tested-by: syzbot+6ea290ba76d8c1eb1ac2@syzkaller.appspotmail.com > > Closes: https://syzkaller.appspot.com/bug?extid=6ea290ba76d8c1eb1ac2 > > Signed-off-by: Edward Adam Davis <eadavis@qq.com> > > --- > > net/bluetooth/hci_core.c | 3 +-- > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c > > index d6976db02c06..20605a7f3f4e 100644 > > --- a/net/bluetooth/hci_core.c > > +++ b/net/bluetooth/hci_core.c > > @@ -3791,8 +3791,7 @@ static void hci_acldata_packet(struct hci_dev *hdev, struct sk_buff *skb) > > l2cap_recv_acldata(conn, skb, flags); > > return; > > } else { > > - bt_dev_err(hdev, "ACL packet for unknown connection handle %d", > > - handle); > > + bt_dev_err(hdev, "ACL packet for unknown connection handle"); > > } > > > > kfree_skb(skb); > > -- > > 2.43.0 > > > > > -- > Luiz Augusto von Dentz #syz test
Hello,
syzbot has tested the proposed patch and the reproducer did not trigger any issue:
Reported-by: syzbot+6ea290ba76d8c1eb1ac2@syzkaller.appspotmail.com
Tested-by: syzbot+6ea290ba76d8c1eb1ac2@syzkaller.appspotmail.com
Tested on:
commit: 87d6aab2 Merge tag 'for_linus' of git://git.kernel.org..
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=14bf8f9f980000
kernel config: https://syzkaller.appspot.com/x/.config?x=981fe2ff8a1e457a
dashboard link: https://syzkaller.appspot.com/bug?extid=6ea290ba76d8c1eb1ac2
compiler: Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40
patch: https://syzkaller.appspot.com/x/patch.diff?x=14f41780580000
Note: testing is done by a robot and is best-effort only.
On Tue, 8 Oct 2024 10:12:34 -0400, Luiz Augusto von Dentz wrote: > On Tue, Oct 8, 2024 at 6:47 AM Edward Adam Davis <eadavis@qq.com> wrote: > > > > Syzbot reported a uninit-value in hci_rx_work.This is because l2cap didn't > > execute the corresponding connection request to call l2cap_send_cmd() or > > l2cap_do_send(), and ultimately called hci_add_acl_hdr() to set hdr->handle. > > What are you talking about here, what these functions have to do with > a local handle variable? > > > Therefore, when calling the thread callback function hci_rx_work() to call > > hci_acldata_packet, hdr->handle should not be used directly. > > It is not being used directly, the handle is a local variable which > get assigned: > > handle = __le16_to_cpu(hdr->handle); > > If what you are saying is that there is no guarantee that skb->len >= > HCI_ACL_HDR_SIZE then we probably want to replace skb_pull with > skb_pull_data. You are right, skb->len is too small. But, skb_pull is ok, maybe just need to add a check. #syz test diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c index d6976db02c06..cfb828452a13 100644 --- a/net/bluetooth/hci_core.c +++ b/net/bluetooth/hci_core.c @@ -3769,7 +3769,11 @@ static void hci_acldata_packet(struct hci_dev *hdev, struct sk_buff *skb) struct hci_conn *conn; __u16 handle, flags; - skb_pull(skb, HCI_ACL_HDR_SIZE); + if (!skb_pull(skb, HCI_ACL_HDR_SIZE)) { + BT_ERR("ACL data packet %d, smaller than ACL HEADER Size %d\n", + skb->len, HCI_ACL_HDR_SIZE); + goto out; + } handle = __le16_to_cpu(hdr->handle); flags = hci_flags(handle); @@ -3795,6 +3799,7 @@ static void hci_acldata_packet(struct hci_dev *hdev, struct sk_buff *skb) handle); } +out: kfree_skb(skb); }
Hello,
syzbot has tested the proposed patch and the reproducer did not trigger any issue:
Reported-by: syzbot+6ea290ba76d8c1eb1ac2@syzkaller.appspotmail.com
Tested-by: syzbot+6ea290ba76d8c1eb1ac2@syzkaller.appspotmail.com
Tested on:
commit: 75b607fa Merge tag 'sched_ext-for-6.12-rc2-fixes' of g..
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=1415f380580000
kernel config: https://syzkaller.appspot.com/x/.config?x=981fe2ff8a1e457a
dashboard link: https://syzkaller.appspot.com/bug?extid=6ea290ba76d8c1eb1ac2
compiler: Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40
patch: https://syzkaller.appspot.com/x/patch.diff?x=12e37707980000
Note: testing is done by a robot and is best-effort only.
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c index d6976db02c06..20605a7f3f4e 100644 --- a/net/bluetooth/hci_core.c +++ b/net/bluetooth/hci_core.c @@ -3791,8 +3791,7 @@ static void hci_acldata_packet(struct hci_dev *hdev, struct sk_buff *skb) l2cap_recv_acldata(conn, skb, flags); return; } else { - bt_dev_err(hdev, "ACL packet for unknown connection handle %d", - handle); + bt_dev_err(hdev, "ACL packet for unknown connection handle"); } kfree_skb(skb);
Syzbot reported a uninit-value in hci_rx_work.This is because l2cap didn't execute the corresponding connection request to call l2cap_send_cmd() or l2cap_do_send(), and ultimately called hci_add_acl_hdr() to set hdr->handle. Therefore, when calling the thread callback function hci_rx_work() to call hci_acldata_packet, hdr->handle should not be used directly. Reported-and-tested-by: syzbot+6ea290ba76d8c1eb1ac2@syzkaller.appspotmail.com Closes: https://syzkaller.appspot.com/bug?extid=6ea290ba76d8c1eb1ac2 Signed-off-by: Edward Adam Davis <eadavis@qq.com> --- net/bluetooth/hci_core.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)