* [PATCH] Bluetooth: Fix kernel crash on BT stress tests. @ 2010-02-16 10:36 Andrei Emeltchenko 2010-02-16 11:34 ` Gustavo F. Padovan 2010-03-11 22:41 ` Gustavo F. Padovan 0 siblings, 2 replies; 12+ messages in thread From: Andrei Emeltchenko @ 2010-02-16 10:36 UTC (permalink / raw) To: linux-bluetooth [-- Attachment #1: Type: text/plain, Size: 2170 bytes --] >From 0135f732cb45e5e91062aca84a61a40b172200a4 Mon Sep 17 00:00:00 2001 From: Andrei Emeltchenko <andrei.emeltchenko@nokia.com> Date: Tue, 16 Feb 2010 10:52:33 +0200 Subject: [PATCH] Bluetooth: Fix kernel crash on BT stress tests. Added very simple check that req buffer has enough space to fit configuration parameters. Shall be enough to reject packets with configuration size more than req buffer. Crash trace below [ 6069.659393] Unable to handle kernel paging request at virtual address 02000205 [ 6069.673034] Internal error: Oops: 805 [#1] PREEMPT ... [ 6069.727172] PC is at l2cap_add_conf_opt+0x70/0xf0 [l2cap] [ 6069.732604] LR is at l2cap_recv_frame+0x1350/0x2e78 [l2cap] ... [ 6070.030303] Backtrace: [ 6070.032806] [<bf1c2880>] (l2cap_add_conf_opt+0x0/0xf0 [l2cap]) from [<bf1c6624>] (l2cap_recv_frame+0x1350/0x2e78 [l2cap]) [ 6070.043823] r8:dc5d3100 r7:df2a91d6 r6:00000001 r5:df2a8000 r4:00000200 [ 6070.050659] [<bf1c52d4>] (l2cap_recv_frame+0x0/0x2e78 [l2cap]) from [<bf1c8408>] (l2cap_recv_acldata+0x2bc/0x350 [l2cap]) [ 6070.061798] [<bf1c814c>] (l2cap_recv_acldata+0x0/0x350 [l2cap]) from [<bf0037a4>] (hci_rx_task+0x244/0x478 [bluetooth]) [ 6070.072631] r6:dc647700 r5:00000001 r4:df2ab740 [ 6070.077362] [<bf003560>] (hci_rx_task+0x0/0x478 [bluetooth]) from [<c006b9fc>] (tasklet_action+0x78/0xd8) [ 6070.087005] [<c006b984>] (tasklet_action+0x0/0xd8) from [<c006c160>] Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@nokia.com> --- net/bluetooth/l2cap.c | 6 ++++++ 1 files changed, 6 insertions(+), 0 deletions(-) diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c index 400efa2..69b7280 100644 --- a/net/bluetooth/l2cap.c +++ b/net/bluetooth/l2cap.c @@ -2830,6 +2830,12 @@ static inline int l2cap_config_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hdr int len = cmd->len - sizeof(*rsp); char req[64]; + if (len > sizeof(req) - sizeof(struct l2cap_conf_req)) { + BT_ERR("Config response is too big"); + l2cap_send_disconn_req(conn, sk); + goto done; + } + /* throw out any old stored conf requests */ result = L2CAP_CONF_SUCCESS; len = l2cap_parse_conf_rsp(sk, rsp->data, -- 1.6.0.4 [-- Attachment #2: 0001-Bluetooth-Fix-kernel-crash-on-BT-stress-tests.patch --] [-- Type: text/x-patch, Size: 2171 bytes --] From 0135f732cb45e5e91062aca84a61a40b172200a4 Mon Sep 17 00:00:00 2001 From: Andrei Emeltchenko <andrei.emeltchenko@nokia.com> Date: Tue, 16 Feb 2010 10:52:33 +0200 Subject: [PATCH] Bluetooth: Fix kernel crash on BT stress tests. Added very simple check that req buffer has enough space to fit configuration parameters. Shall be enough to reject packets with configuration size more than req buffer. Crash trace below [ 6069.659393] Unable to handle kernel paging request at virtual address 02000205 [ 6069.673034] Internal error: Oops: 805 [#1] PREEMPT ... [ 6069.727172] PC is at l2cap_add_conf_opt+0x70/0xf0 [l2cap] [ 6069.732604] LR is at l2cap_recv_frame+0x1350/0x2e78 [l2cap] ... [ 6070.030303] Backtrace: [ 6070.032806] [<bf1c2880>] (l2cap_add_conf_opt+0x0/0xf0 [l2cap]) from [<bf1c6624>] (l2cap_recv_frame+0x1350/0x2e78 [l2cap]) [ 6070.043823] r8:dc5d3100 r7:df2a91d6 r6:00000001 r5:df2a8000 r4:00000200 [ 6070.050659] [<bf1c52d4>] (l2cap_recv_frame+0x0/0x2e78 [l2cap]) from [<bf1c8408>] (l2cap_recv_acldata+0x2bc/0x350 [l2cap]) [ 6070.061798] [<bf1c814c>] (l2cap_recv_acldata+0x0/0x350 [l2cap]) from [<bf0037a4>] (hci_rx_task+0x244/0x478 [bluetooth]) [ 6070.072631] r6:dc647700 r5:00000001 r4:df2ab740 [ 6070.077362] [<bf003560>] (hci_rx_task+0x0/0x478 [bluetooth]) from [<c006b9fc>] (tasklet_action+0x78/0xd8) [ 6070.087005] [<c006b984>] (tasklet_action+0x0/0xd8) from [<c006c160>] Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@nokia.com> --- net/bluetooth/l2cap.c | 6 ++++++ 1 files changed, 6 insertions(+), 0 deletions(-) diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c index 400efa2..69b7280 100644 --- a/net/bluetooth/l2cap.c +++ b/net/bluetooth/l2cap.c @@ -2830,6 +2830,12 @@ static inline int l2cap_config_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hdr int len = cmd->len - sizeof(*rsp); char req[64]; + if (len > sizeof(req) - sizeof(struct l2cap_conf_req)) { + BT_ERR("Config response is too big"); + l2cap_send_disconn_req(conn, sk); + goto done; + } + /* throw out any old stored conf requests */ result = L2CAP_CONF_SUCCESS; len = l2cap_parse_conf_rsp(sk, rsp->data, -- 1.6.0.4 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] Bluetooth: Fix kernel crash on BT stress tests. 2010-02-16 10:36 [PATCH] Bluetooth: Fix kernel crash on BT stress tests Andrei Emeltchenko @ 2010-02-16 11:34 ` Gustavo F. Padovan 2010-02-16 11:58 ` Andrei Emeltchenko 2010-03-11 22:41 ` Gustavo F. Padovan 1 sibling, 1 reply; 12+ messages in thread From: Gustavo F. Padovan @ 2010-02-16 11:34 UTC (permalink / raw) To: Andrei Emeltchenko; +Cc: linux-bluetooth Hi Andrei, * Andrei Emeltchenko <andrei.emeltchenko.news@gmail.com> [2010-02-16 12:36:47 +0200]: > From 0135f732cb45e5e91062aca84a61a40b172200a4 Mon Sep 17 00:00:00 2001 > From: Andrei Emeltchenko <andrei.emeltchenko@nokia.com> > Date: Tue, 16 Feb 2010 10:52:33 +0200 > Subject: [PATCH] Bluetooth: Fix kernel crash on BT stress tests. > > Added very simple check that req buffer has enough space to > fit configuration parameters. Shall be enough to reject packets > with configuration size more than req buffer. > > Crash trace below > > [ 6069.659393] Unable to handle kernel paging request at virtual > address 02000205 > [ 6069.673034] Internal error: Oops: 805 [#1] PREEMPT > ... > [ 6069.727172] PC is at l2cap_add_conf_opt+0x70/0xf0 [l2cap] > [ 6069.732604] LR is at l2cap_recv_frame+0x1350/0x2e78 [l2cap] > ... > [ 6070.030303] Backtrace: > [ 6070.032806] [<bf1c2880>] (l2cap_add_conf_opt+0x0/0xf0 [l2cap]) from > [<bf1c6624>] (l2cap_recv_frame+0x1350/0x2e78 [l2cap]) > [ 6070.043823] r8:dc5d3100 r7:df2a91d6 r6:00000001 r5:df2a8000 r4:00000200 > [ 6070.050659] [<bf1c52d4>] (l2cap_recv_frame+0x0/0x2e78 [l2cap]) from > [<bf1c8408>] (l2cap_recv_acldata+0x2bc/0x350 [l2cap]) > [ 6070.061798] [<bf1c814c>] (l2cap_recv_acldata+0x0/0x350 [l2cap]) from > [<bf0037a4>] (hci_rx_task+0x244/0x478 [bluetooth]) > [ 6070.072631] r6:dc647700 r5:00000001 r4:df2ab740 > [ 6070.077362] [<bf003560>] (hci_rx_task+0x0/0x478 [bluetooth]) from > [<c006b9fc>] (tasklet_action+0x78/0xd8) > [ 6070.087005] [<c006b984>] (tasklet_action+0x0/0xd8) from [<c006c160>] Are you using ERTM or Streaming mode? If yes, I have a guess about the source of the problem. On l2cap_parse_conf_rsp we check for: while (len >= L2CAP_CONF_OPT_SIZE) { len -= l2cap_get_conf_opt(&rsp, &type, &olen, &val); But on case L2CAP_CONF_RFC olen is greater than L2CAP_CONF_OPT_SIZE we can exceed the buffer size. So the right fix will be check if len >= olen in that case. > > Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@nokia.com> > --- > net/bluetooth/l2cap.c | 6 ++++++ > 1 files changed, 6 insertions(+), 0 deletions(-) > > diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c > index 400efa2..69b7280 100644 > --- a/net/bluetooth/l2cap.c > +++ b/net/bluetooth/l2cap.c > @@ -2830,6 +2830,12 @@ static inline int l2cap_config_rsp(struct > l2cap_conn *conn, struct l2cap_cmd_hdr > int len = cmd->len - sizeof(*rsp); > char req[64]; > > + if (len > sizeof(req) - sizeof(struct l2cap_conf_req)) { > + BT_ERR("Config response is too big"); > + l2cap_send_disconn_req(conn, sk); > + goto done; > + } > + > /* throw out any old stored conf requests */ > result = L2CAP_CONF_SUCCESS; > len = l2cap_parse_conf_rsp(sk, rsp->data, > -- > 1.6.0.4 -- Gustavo F. Padovan http://padovan.org ProFUSION embedded systems - http://profusion.mobi ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Bluetooth: Fix kernel crash on BT stress tests. 2010-02-16 11:34 ` Gustavo F. Padovan @ 2010-02-16 11:58 ` Andrei Emeltchenko 2010-02-16 12:16 ` Andrei Emeltchenko 2010-02-17 23:57 ` Gustavo F. Padovan 0 siblings, 2 replies; 12+ messages in thread From: Andrei Emeltchenko @ 2010-02-16 11:58 UTC (permalink / raw) To: Gustavo F. Padovan; +Cc: linux-bluetooth On Tue, Feb 16, 2010 at 1:34 PM, Gustavo F. Padovan <padovan@profusion.mobi> wrote: > Hi Andrei, > > * Andrei Emeltchenko <andrei.emeltchenko.news@gmail.com> [2010-02-16 12:36:47 +0200]: > >> From 0135f732cb45e5e91062aca84a61a40b172200a4 Mon Sep 17 00:00:00 2001 >> From: Andrei Emeltchenko <andrei.emeltchenko@nokia.com> >> Date: Tue, 16 Feb 2010 10:52:33 +0200 >> Subject: [PATCH] Bluetooth: Fix kernel crash on BT stress tests. >> >> Added very simple check that req buffer has enough space to >> fit configuration parameters. Shall be enough to reject packets >> with configuration size more than req buffer. >> >> Crash trace below >> >> [ 6069.659393] Unable to handle kernel paging request at virtual >> address 02000205 >> [ 6069.673034] Internal error: Oops: 805 [#1] PREEMPT >> ... >> [ 6069.727172] PC is at l2cap_add_conf_opt+0x70/0xf0 [l2cap] >> [ 6069.732604] LR is at l2cap_recv_frame+0x1350/0x2e78 [l2cap] >> ... >> [ 6070.030303] Backtrace: >> [ 6070.032806] [<bf1c2880>] (l2cap_add_conf_opt+0x0/0xf0 [l2cap]) from >> [<bf1c6624>] (l2cap_recv_frame+0x1350/0x2e78 [l2cap]) >> [ 6070.043823] r8:dc5d3100 r7:df2a91d6 r6:00000001 r5:df2a8000 r4:00000200 >> [ 6070.050659] [<bf1c52d4>] (l2cap_recv_frame+0x0/0x2e78 [l2cap]) from >> [<bf1c8408>] (l2cap_recv_acldata+0x2bc/0x350 [l2cap]) >> [ 6070.061798] [<bf1c814c>] (l2cap_recv_acldata+0x0/0x350 [l2cap]) from >> [<bf0037a4>] (hci_rx_task+0x244/0x478 [bluetooth]) >> [ 6070.072631] r6:dc647700 r5:00000001 r4:df2ab740 >> [ 6070.077362] [<bf003560>] (hci_rx_task+0x0/0x478 [bluetooth]) from >> [<c006b9fc>] (tasklet_action+0x78/0xd8) >> [ 6070.087005] [<c006b984>] (tasklet_action+0x0/0xd8) from [<c006c160>] > > Are you using ERTM or Streaming mode? If yes, I have a guess about the > source of the problem. On l2cap_parse_conf_rsp we check for: > > while (len >= L2CAP_CONF_OPT_SIZE) { > len -= l2cap_get_conf_opt(&rsp, &type, &olen, &val); > > But on case L2CAP_CONF_RFC olen is greater than L2CAP_CONF_OPT_SIZE we > can exceed the buffer size. So the right fix will be check if len >= > olen in that case. We use test tool which sends "Configure Response" packet with size 262 bytes. So "req" buffer gets overwritten. But in the code nobody checks that "req" might be overwritten. -- Andrei > > >> >> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@nokia.com> >> --- >> net/bluetooth/l2cap.c | 6 ++++++ >> 1 files changed, 6 insertions(+), 0 deletions(-) >> >> diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c >> index 400efa2..69b7280 100644 >> --- a/net/bluetooth/l2cap.c >> +++ b/net/bluetooth/l2cap.c >> @@ -2830,6 +2830,12 @@ static inline int l2cap_config_rsp(struct >> l2cap_conn *conn, struct l2cap_cmd_hdr >> int len = cmd->len - sizeof(*rsp); >> char req[64]; >> >> + if (len > sizeof(req) - sizeof(struct l2cap_conf_req)) { >> + BT_ERR("Config response is too big"); >> + l2cap_send_disconn_req(conn, sk); >> + goto done; >> + } >> + >> /* throw out any old stored conf requests */ >> result = L2CAP_CONF_SUCCESS; >> len = l2cap_parse_conf_rsp(sk, rsp->data, >> -- >> 1.6.0.4 > > > -- > Gustavo F. Padovan > http://padovan.org > > ProFUSION embedded systems - http://profusion.mobi > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Bluetooth: Fix kernel crash on BT stress tests. 2010-02-16 11:58 ` Andrei Emeltchenko @ 2010-02-16 12:16 ` Andrei Emeltchenko 2010-02-17 23:57 ` Gustavo F. Padovan 1 sibling, 0 replies; 12+ messages in thread From: Andrei Emeltchenko @ 2010-02-16 12:16 UTC (permalink / raw) To: Gustavo F. Padovan; +Cc: linux-bluetooth Hi Gustavo, >> Are you using ERTM or Streaming mode? If yes, I have a guess about the >> source of the problem. On l2cap_parse_conf_rsp we check for: >> >> while (len >= L2CAP_CONF_OPT_SIZE) { >> len -= l2cap_get_conf_opt(&rsp, &type, &olen, &val); >> >> But on case L2CAP_CONF_RFC olen is greater than L2CAP_CONF_OPT_SIZE we >> can exceed the buffer size. So the right fix will be check if len >= >> olen in that case. > > We use test tool which sends "Configure Response" packet with size 262 > bytes. So "req" buffer gets overwritten. But in the code nobody checks > that "req" might be overwritten. The packet we are sending is: Bluetooth L2CAP Packet Length: 266 CID: 0x0001 Command: Configure Response Command Code: Configure Response (0x05) Command Identifier: 0x02 Command Length: 262 Source CID: 0x0040 .... .... .... ...0 = Continuation Flag: False Result: Failure - unacceptable parameters (0x0001) Option: MTU Option: MTU Option: MTU Option: MTU Option: MTU Option: MTU Option: MTU Option: MTU Option: MTU Option: MTU ............. We can check of course that parameter MTU is already exist. Still there is possibility to overwrite 64 bytes of req buffer so some check is needed. -- Andrei ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Bluetooth: Fix kernel crash on BT stress tests. 2010-02-16 11:58 ` Andrei Emeltchenko 2010-02-16 12:16 ` Andrei Emeltchenko @ 2010-02-17 23:57 ` Gustavo F. Padovan 2010-02-18 16:18 ` Andrei Emeltchenko 1 sibling, 1 reply; 12+ messages in thread From: Gustavo F. Padovan @ 2010-02-17 23:57 UTC (permalink / raw) To: Andrei Emeltchenko; +Cc: linux-bluetooth Hi Andrei, * Andrei Emeltchenko <andrei.emeltchenko.news@gmail.com> [2010-02-16 13:58:34 +0200]: > On Tue, Feb 16, 2010 at 1:34 PM, Gustavo F. Padovan > <padovan@profusion.mobi> wrote: > > Hi Andrei, > > > > * Andrei Emeltchenko <andrei.emeltchenko.news@gmail.com> [2010-02-16 12:36:47 +0200]: > > > >> From 0135f732cb45e5e91062aca84a61a40b172200a4 Mon Sep 17 00:00:00 2001 > >> From: Andrei Emeltchenko <andrei.emeltchenko@nokia.com> > >> Date: Tue, 16 Feb 2010 10:52:33 +0200 > >> Subject: [PATCH] Bluetooth: Fix kernel crash on BT stress tests. > >> > >> Added very simple check that req buffer has enough space to > >> fit configuration parameters. Shall be enough to reject packets > >> with configuration size more than req buffer. > >> > >> Crash trace below > >> > >> [ 6069.659393] Unable to handle kernel paging request at virtual > >> address 02000205 > >> [ 6069.673034] Internal error: Oops: 805 [#1] PREEMPT > >> ... > >> [ 6069.727172] PC is at l2cap_add_conf_opt+0x70/0xf0 [l2cap] > >> [ 6069.732604] LR is at l2cap_recv_frame+0x1350/0x2e78 [l2cap] > >> ... > >> [ 6070.030303] Backtrace: > >> [ 6070.032806] [<bf1c2880>] (l2cap_add_conf_opt+0x0/0xf0 [l2cap]) from > >> [<bf1c6624>] (l2cap_recv_frame+0x1350/0x2e78 [l2cap]) > >> [ 6070.043823] r8:dc5d3100 r7:df2a91d6 r6:00000001 r5:df2a8000 r4:00000200 > >> [ 6070.050659] [<bf1c52d4>] (l2cap_recv_frame+0x0/0x2e78 [l2cap]) from > >> [<bf1c8408>] (l2cap_recv_acldata+0x2bc/0x350 [l2cap]) > >> [ 6070.061798] [<bf1c814c>] (l2cap_recv_acldata+0x0/0x350 [l2cap]) from > >> [<bf0037a4>] (hci_rx_task+0x244/0x478 [bluetooth]) > >> [ 6070.072631] r6:dc647700 r5:00000001 r4:df2ab740 > >> [ 6070.077362] [<bf003560>] (hci_rx_task+0x0/0x478 [bluetooth]) from > >> [<c006b9fc>] (tasklet_action+0x78/0xd8) > >> [ 6070.087005] [<c006b984>] (tasklet_action+0x0/0xd8) from [<c006c160>] > > > > Are you using ERTM or Streaming mode? If yes, I have a guess about the > > source of the problem. On l2cap_parse_conf_rsp we check for: > > > > while (len >= L2CAP_CONF_OPT_SIZE) { > > len -= l2cap_get_conf_opt(&rsp, &type, &olen, &val); > > > > But on case L2CAP_CONF_RFC olen is greater than L2CAP_CONF_OPT_SIZE we > > can exceed the buffer size. So the right fix will be check if len >= > > olen in that case. > > We use test tool which sends "Configure Response" packet with size 262 > bytes. So "req" buffer gets overwritten. But in the code nobody checks > that "req" might be overwritten. So I have found another issue. ;) Have to fix it. > > -- Andrei > > > > > > >> > >> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@nokia.com> > >> --- > >> net/bluetooth/l2cap.c | 6 ++++++ > >> 1 files changed, 6 insertions(+), 0 deletions(-) > >> > >> diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c > >> index 400efa2..69b7280 100644 > >> --- a/net/bluetooth/l2cap.c > >> +++ b/net/bluetooth/l2cap.c > >> @@ -2830,6 +2830,12 @@ static inline int l2cap_config_rsp(struct > >> l2cap_conn *conn, struct l2cap_cmd_hdr > >> int len = cmd->len - sizeof(*rsp); > >> char req[64]; > >> > >> + if (len > sizeof(req) - sizeof(struct l2cap_conf_req)) { > >> + BT_ERR("Config response is too big"); > >> + l2cap_send_disconn_req(conn, sk); > >> + goto done; > >> + } > >> + > >> /* throw out any old stored conf requests */ > >> result = L2CAP_CONF_SUCCESS; > >> len = l2cap_parse_conf_rsp(sk, rsp->data, > >> -- > >> 1.6.0.4 > > > > > > -- > > Gustavo F. Padovan > > http://padovan.org > > > > ProFUSION embedded systems - http://profusion.mobi > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Gustavo F. Padovan http://padovan.org ProFUSION embedded systems - http://profusion.mobi ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Bluetooth: Fix kernel crash on BT stress tests. 2010-02-17 23:57 ` Gustavo F. Padovan @ 2010-02-18 16:18 ` Andrei Emeltchenko 2010-03-11 14:54 ` Andrei Emeltchenko 0 siblings, 1 reply; 12+ messages in thread From: Andrei Emeltchenko @ 2010-02-18 16:18 UTC (permalink / raw) To: Gustavo F. Padovan; +Cc: linux-bluetooth Hi Gustavo On Thu, Feb 18, 2010 at 1:57 AM, Gustavo F. Padovan <padovan@profusion.mobi> wrote: > Hi Andrei, > > * Andrei Emeltchenko <andrei.emeltchenko.news@gmail.com> [2010-02-16 13:58:34 +0200]: > >> On Tue, Feb 16, 2010 at 1:34 PM, Gustavo F. Padovan >> <padovan@profusion.mobi> wrote: >> > Hi Andrei, >> > >> > * Andrei Emeltchenko <andrei.emeltchenko.news@gmail.com> [2010-02-16 12:36:47 +0200]: >> > >> >> From 0135f732cb45e5e91062aca84a61a40b172200a4 Mon Sep 17 00:00:00 2001 >> >> From: Andrei Emeltchenko <andrei.emeltchenko@nokia.com> >> >> Date: Tue, 16 Feb 2010 10:52:33 +0200 >> >> Subject: [PATCH] Bluetooth: Fix kernel crash on BT stress tests. >> >> >> >> Added very simple check that req buffer has enough space to >> >> fit configuration parameters. Shall be enough to reject packets >> >> with configuration size more than req buffer. >> >> >> >> Crash trace below >> >> >> >> [ 6069.659393] Unable to handle kernel paging request at virtual >> >> address 02000205 >> >> [ 6069.673034] Internal error: Oops: 805 [#1] PREEMPT >> >> ... >> >> [ 6069.727172] PC is at l2cap_add_conf_opt+0x70/0xf0 [l2cap] >> >> [ 6069.732604] LR is at l2cap_recv_frame+0x1350/0x2e78 [l2cap] >> >> ... >> >> [ 6070.030303] Backtrace: >> >> [ 6070.032806] [<bf1c2880>] (l2cap_add_conf_opt+0x0/0xf0 [l2cap]) from >> >> [<bf1c6624>] (l2cap_recv_frame+0x1350/0x2e78 [l2cap]) >> >> [ 6070.043823] r8:dc5d3100 r7:df2a91d6 r6:00000001 r5:df2a8000 r4:00000200 >> >> [ 6070.050659] [<bf1c52d4>] (l2cap_recv_frame+0x0/0x2e78 [l2cap]) from >> >> [<bf1c8408>] (l2cap_recv_acldata+0x2bc/0x350 [l2cap]) >> >> [ 6070.061798] [<bf1c814c>] (l2cap_recv_acldata+0x0/0x350 [l2cap]) from >> >> [<bf0037a4>] (hci_rx_task+0x244/0x478 [bluetooth]) >> >> [ 6070.072631] r6:dc647700 r5:00000001 r4:df2ab740 >> >> [ 6070.077362] [<bf003560>] (hci_rx_task+0x0/0x478 [bluetooth]) from >> >> [<c006b9fc>] (tasklet_action+0x78/0xd8) >> >> [ 6070.087005] [<c006b984>] (tasklet_action+0x0/0xd8) from [<c006c160>] >> > >> > Are you using ERTM or Streaming mode? If yes, I have a guess about the >> > source of the problem. On l2cap_parse_conf_rsp we check for: >> > >> > while (len >= L2CAP_CONF_OPT_SIZE) { >> > len -= l2cap_get_conf_opt(&rsp, &type, &olen, &val); >> > >> > But on case L2CAP_CONF_RFC olen is greater than L2CAP_CONF_OPT_SIZE we >> > can exceed the buffer size. So the right fix will be check if len >= >> > olen in that case. Is len always bigger then olen? in l2cap_get_conf_opt : len = L2CAP_CONF_OPT_SIZE + opt->len; opt->len is olen -- Andrei >> >> We use test tool which sends "Configure Response" packet with size 262 >> bytes. So "req" buffer gets overwritten. But in the code nobody checks >> that "req" might be overwritten. > > So I have found another issue. ;) > Have to fix it. > >> >> -- Andrei >> >> > >> > >> >> >> >> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@nokia.com> >> >> --- >> >> net/bluetooth/l2cap.c | 6 ++++++ >> >> 1 files changed, 6 insertions(+), 0 deletions(-) >> >> >> >> diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c >> >> index 400efa2..69b7280 100644 >> >> --- a/net/bluetooth/l2cap.c >> >> +++ b/net/bluetooth/l2cap.c >> >> @@ -2830,6 +2830,12 @@ static inline int l2cap_config_rsp(struct >> >> l2cap_conn *conn, struct l2cap_cmd_hdr >> >> int len = cmd->len - sizeof(*rsp); >> >> char req[64]; >> >> >> >> + if (len > sizeof(req) - sizeof(struct l2cap_conf_req)) { >> >> + BT_ERR("Config response is too big"); >> >> + l2cap_send_disconn_req(conn, sk); >> >> + goto done; >> >> + } >> >> + >> >> /* throw out any old stored conf requests */ >> >> result = L2CAP_CONF_SUCCESS; >> >> len = l2cap_parse_conf_rsp(sk, rsp->data, >> >> -- >> >> 1.6.0.4 >> > >> > >> > -- >> > Gustavo F. Padovan >> > http://padovan.org >> > >> > ProFUSION embedded systems - http://profusion.mobi >> > >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- > Gustavo F. Padovan > http://padovan.org > > ProFUSION embedded systems - http://profusion.mobi > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Bluetooth: Fix kernel crash on BT stress tests. 2010-02-18 16:18 ` Andrei Emeltchenko @ 2010-03-11 14:54 ` Andrei Emeltchenko 2010-03-11 17:16 ` Marcel Holtmann 0 siblings, 1 reply; 12+ messages in thread From: Andrei Emeltchenko @ 2010-03-11 14:54 UTC (permalink / raw) To: Gustavo F. Padovan; +Cc: linux-bluetooth Hei, What is the status of this patch proposed? -- Andrei On Thu, Feb 18, 2010 at 6:18 PM, Andrei Emeltchenko <andrei.emeltchenko.news@gmail.com> wrote: > Hi Gustavo > > On Thu, Feb 18, 2010 at 1:57 AM, Gustavo F. Padovan > <padovan@profusion.mobi> wrote: >> Hi Andrei, >> >> * Andrei Emeltchenko <andrei.emeltchenko.news@gmail.com> [2010-02-16 13:58:34 +0200]: >> >>> On Tue, Feb 16, 2010 at 1:34 PM, Gustavo F. Padovan >>> <padovan@profusion.mobi> wrote: >>> > Hi Andrei, >>> > >>> > * Andrei Emeltchenko <andrei.emeltchenko.news@gmail.com> [2010-02-16 12:36:47 +0200]: >>> > >>> >> From 0135f732cb45e5e91062aca84a61a40b172200a4 Mon Sep 17 00:00:00 2001 >>> >> From: Andrei Emeltchenko <andrei.emeltchenko@nokia.com> >>> >> Date: Tue, 16 Feb 2010 10:52:33 +0200 >>> >> Subject: [PATCH] Bluetooth: Fix kernel crash on BT stress tests. >>> >> >>> >> Added very simple check that req buffer has enough space to >>> >> fit configuration parameters. Shall be enough to reject packets >>> >> with configuration size more than req buffer. >>> >> >>> >> Crash trace below >>> >> >>> >> [ 6069.659393] Unable to handle kernel paging request at virtual >>> >> address 02000205 >>> >> [ 6069.673034] Internal error: Oops: 805 [#1] PREEMPT >>> >> ... >>> >> [ 6069.727172] PC is at l2cap_add_conf_opt+0x70/0xf0 [l2cap] >>> >> [ 6069.732604] LR is at l2cap_recv_frame+0x1350/0x2e78 [l2cap] >>> >> ... >>> >> [ 6070.030303] Backtrace: >>> >> [ 6070.032806] [<bf1c2880>] (l2cap_add_conf_opt+0x0/0xf0 [l2cap]) from >>> >> [<bf1c6624>] (l2cap_recv_frame+0x1350/0x2e78 [l2cap]) >>> >> [ 6070.043823] r8:dc5d3100 r7:df2a91d6 r6:00000001 r5:df2a8000 r4:00000200 >>> >> [ 6070.050659] [<bf1c52d4>] (l2cap_recv_frame+0x0/0x2e78 [l2cap]) from >>> >> [<bf1c8408>] (l2cap_recv_acldata+0x2bc/0x350 [l2cap]) >>> >> [ 6070.061798] [<bf1c814c>] (l2cap_recv_acldata+0x0/0x350 [l2cap]) from >>> >> [<bf0037a4>] (hci_rx_task+0x244/0x478 [bluetooth]) >>> >> [ 6070.072631] r6:dc647700 r5:00000001 r4:df2ab740 >>> >> [ 6070.077362] [<bf003560>] (hci_rx_task+0x0/0x478 [bluetooth]) from >>> >> [<c006b9fc>] (tasklet_action+0x78/0xd8) >>> >> [ 6070.087005] [<c006b984>] (tasklet_action+0x0/0xd8) from [<c006c160>] >>> > >>> > Are you using ERTM or Streaming mode? If yes, I have a guess about the >>> > source of the problem. On l2cap_parse_conf_rsp we check for: >>> > >>> > while (len >= L2CAP_CONF_OPT_SIZE) { >>> > len -= l2cap_get_conf_opt(&rsp, &type, &olen, &val); >>> > >>> > But on case L2CAP_CONF_RFC olen is greater than L2CAP_CONF_OPT_SIZE we >>> > can exceed the buffer size. So the right fix will be check if len >= >>> > olen in that case. > > Is len always bigger then olen? > > in l2cap_get_conf_opt : > > len = L2CAP_CONF_OPT_SIZE + opt->len; > > opt->len is olen > > -- Andrei > >>> >>> We use test tool which sends "Configure Response" packet with size 262 >>> bytes. So "req" buffer gets overwritten. But in the code nobody checks >>> that "req" might be overwritten. >> >> So I have found another issue. ;) >> Have to fix it. >> >>> >>> -- Andrei >>> >>> > >>> > >>> >> >>> >> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@nokia.com> >>> >> --- >>> >> net/bluetooth/l2cap.c | 6 ++++++ >>> >> 1 files changed, 6 insertions(+), 0 deletions(-) >>> >> >>> >> diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c >>> >> index 400efa2..69b7280 100644 >>> >> --- a/net/bluetooth/l2cap.c >>> >> +++ b/net/bluetooth/l2cap.c >>> >> @@ -2830,6 +2830,12 @@ static inline int l2cap_config_rsp(struct >>> >> l2cap_conn *conn, struct l2cap_cmd_hdr >>> >> int len = cmd->len - sizeof(*rsp); >>> >> char req[64]; >>> >> >>> >> + if (len > sizeof(req) - sizeof(struct l2cap_conf_req)) { >>> >> + BT_ERR("Config response is too big"); >>> >> + l2cap_send_disconn_req(conn, sk); >>> >> + goto done; >>> >> + } >>> >> + >>> >> /* throw out any old stored conf requests */ >>> >> result = L2CAP_CONF_SUCCESS; >>> >> len = l2cap_parse_conf_rsp(sk, rsp->data, >>> >> -- >>> >> 1.6.0.4 >>> > >>> > >>> > -- >>> > Gustavo F. Padovan >>> > http://padovan.org >>> > >>> > ProFUSION embedded systems - http://profusion.mobi >>> > >>> -- >>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in >>> the body of a message to majordomo@vger.kernel.org >>> More majordomo info at http://vger.kernel.org/majordomo-info.html >> >> -- >> Gustavo F. Padovan >> http://padovan.org >> >> ProFUSION embedded systems - http://profusion.mobi >> > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Bluetooth: Fix kernel crash on BT stress tests. 2010-03-11 14:54 ` Andrei Emeltchenko @ 2010-03-11 17:16 ` Marcel Holtmann 0 siblings, 0 replies; 12+ messages in thread From: Marcel Holtmann @ 2010-03-11 17:16 UTC (permalink / raw) To: Andrei Emeltchenko; +Cc: Gustavo F. Padovan, linux-bluetooth Hi Andrei, please no top posting. It is never a good idea. > What is the status of this patch proposed? I like see Gustavo's ack or nak on this one. Regards Marcel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Bluetooth: Fix kernel crash on BT stress tests. 2010-02-16 10:36 [PATCH] Bluetooth: Fix kernel crash on BT stress tests Andrei Emeltchenko 2010-02-16 11:34 ` Gustavo F. Padovan @ 2010-03-11 22:41 ` Gustavo F. Padovan 2010-03-15 10:57 ` Andrei Emeltchenko 1 sibling, 1 reply; 12+ messages in thread From: Gustavo F. Padovan @ 2010-03-11 22:41 UTC (permalink / raw) To: Andrei Emeltchenko; +Cc: linux-bluetooth, marcel Hi Andrei, * Andrei Emeltchenko <andrei.emeltchenko.news@gmail.com> [2010-02-16 12:36:47 +0200]: > From 0135f732cb45e5e91062aca84a61a40b172200a4 Mon Sep 17 00:00:00 2001 > From: Andrei Emeltchenko <andrei.emeltchenko@nokia.com> > Date: Tue, 16 Feb 2010 10:52:33 +0200 > Subject: [PATCH] Bluetooth: Fix kernel crash on BT stress tests. > > Added very simple check that req buffer has enough space to > fit configuration parameters. Shall be enough to reject packets > with configuration size more than req buffer. > > Crash trace below > > [ 6069.659393] Unable to handle kernel paging request at virtual > address 02000205 > [ 6069.673034] Internal error: Oops: 805 [#1] PREEMPT > ... > [ 6069.727172] PC is at l2cap_add_conf_opt+0x70/0xf0 [l2cap] > [ 6069.732604] LR is at l2cap_recv_frame+0x1350/0x2e78 [l2cap] > ... > [ 6070.030303] Backtrace: > [ 6070.032806] [<bf1c2880>] (l2cap_add_conf_opt+0x0/0xf0 [l2cap]) from > [<bf1c6624>] (l2cap_recv_frame+0x1350/0x2e78 [l2cap]) > [ 6070.043823] r8:dc5d3100 r7:df2a91d6 r6:00000001 r5:df2a8000 r4:00000200 > [ 6070.050659] [<bf1c52d4>] (l2cap_recv_frame+0x0/0x2e78 [l2cap]) from > [<bf1c8408>] (l2cap_recv_acldata+0x2bc/0x350 [l2cap]) > [ 6070.061798] [<bf1c814c>] (l2cap_recv_acldata+0x0/0x350 [l2cap]) from > [<bf0037a4>] (hci_rx_task+0x244/0x478 [bluetooth]) > [ 6070.072631] r6:dc647700 r5:00000001 r4:df2ab740 > [ 6070.077362] [<bf003560>] (hci_rx_task+0x0/0x478 [bluetooth]) from > [<c006b9fc>] (tasklet_action+0x78/0xd8) > [ 6070.087005] [<c006b984>] (tasklet_action+0x0/0xd8) from [<c006c160>] > > Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@nokia.com> > --- > net/bluetooth/l2cap.c | 6 ++++++ > 1 files changed, 6 insertions(+), 0 deletions(-) > > diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c > index 400efa2..69b7280 100644 > --- a/net/bluetooth/l2cap.c > +++ b/net/bluetooth/l2cap.c > @@ -2830,6 +2830,12 @@ static inline int l2cap_config_rsp(struct > l2cap_conn *conn, struct l2cap_cmd_hdr > int len = cmd->len - sizeof(*rsp); > char req[64]; > > + if (len > sizeof(req) - sizeof(struct l2cap_conf_req)) { > + BT_ERR("Config response is too big"); Remove the BT_ERR, normally we don't print any error on l2cap_send_disconn_req. > + l2cap_send_disconn_req(conn, sk); > + goto done; > + } > + > /* throw out any old stored conf requests */ > result = L2CAP_CONF_SUCCESS; > len = l2cap_parse_conf_rsp(sk, rsp->data, Also, this is very rare crash. Even if we add all configure options to req we won't overwrite it. Doesn't make sense to me send a configuration option more than once in the same config_{rsp,req}. The only way to crash l2cap is using a bugous remote stack or a stress test tools. We need to be protected against those bugous stacks so I'll ack your patch after you send the updated patch without the BT_ERR. -- Gustavo F. Padovan http://padovan.org ProFUSION embedded systems - http://profusion.mobi ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Bluetooth: Fix kernel crash on BT stress tests. 2010-03-11 22:41 ` Gustavo F. Padovan @ 2010-03-15 10:57 ` Andrei Emeltchenko 2010-03-15 15:05 ` Gustavo F. Padovan 0 siblings, 1 reply; 12+ messages in thread From: Andrei Emeltchenko @ 2010-03-15 10:57 UTC (permalink / raw) To: Gustavo F. Padovan; +Cc: linux-bluetooth, marcel [-- Attachment #1: Type: text/plain, Size: 3404 bytes --] Hi Gustavo On Fri, Mar 12, 2010 at 12:41 AM, Gustavo F. Padovan <padovan@profusion.mobi> wrote: > Hi Andrei, > > * Andrei Emeltchenko <andrei.emeltchenko.news@gmail.com> [2010-02-16 12:36:47 +0200]: > >> From 0135f732cb45e5e91062aca84a61a40b172200a4 Mon Sep 17 00:00:00 2001 >> From: Andrei Emeltchenko <andrei.emeltchenko@nokia.com> >> Date: Tue, 16 Feb 2010 10:52:33 +0200 >> Subject: [PATCH] Bluetooth: Fix kernel crash on BT stress tests. >> >> Added very simple check that req buffer has enough space to >> fit configuration parameters. Shall be enough to reject packets >> with configuration size more than req buffer. >> >> Crash trace below >> >> [ 6069.659393] Unable to handle kernel paging request at virtual >> address 02000205 >> [ 6069.673034] Internal error: Oops: 805 [#1] PREEMPT >> ... >> [ 6069.727172] PC is at l2cap_add_conf_opt+0x70/0xf0 [l2cap] >> [ 6069.732604] LR is at l2cap_recv_frame+0x1350/0x2e78 [l2cap] >> ... >> [ 6070.030303] Backtrace: >> [ 6070.032806] [<bf1c2880>] (l2cap_add_conf_opt+0x0/0xf0 [l2cap]) from >> [<bf1c6624>] (l2cap_recv_frame+0x1350/0x2e78 [l2cap]) >> [ 6070.043823] r8:dc5d3100 r7:df2a91d6 r6:00000001 r5:df2a8000 r4:00000200 >> [ 6070.050659] [<bf1c52d4>] (l2cap_recv_frame+0x0/0x2e78 [l2cap]) from >> [<bf1c8408>] (l2cap_recv_acldata+0x2bc/0x350 [l2cap]) >> [ 6070.061798] [<bf1c814c>] (l2cap_recv_acldata+0x0/0x350 [l2cap]) from >> [<bf0037a4>] (hci_rx_task+0x244/0x478 [bluetooth]) >> [ 6070.072631] r6:dc647700 r5:00000001 r4:df2ab740 >> [ 6070.077362] [<bf003560>] (hci_rx_task+0x0/0x478 [bluetooth]) from >> [<c006b9fc>] (tasklet_action+0x78/0xd8) >> [ 6070.087005] [<c006b984>] (tasklet_action+0x0/0xd8) from [<c006c160>] >> >> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@nokia.com> >> --- >> net/bluetooth/l2cap.c | 6 ++++++ >> 1 files changed, 6 insertions(+), 0 deletions(-) >> >> diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c >> index 400efa2..69b7280 100644 >> --- a/net/bluetooth/l2cap.c >> +++ b/net/bluetooth/l2cap.c >> @@ -2830,6 +2830,12 @@ static inline int l2cap_config_rsp(struct >> l2cap_conn *conn, struct l2cap_cmd_hdr >> int len = cmd->len - sizeof(*rsp); >> char req[64]; >> >> + if (len > sizeof(req) - sizeof(struct l2cap_conf_req)) { >> + BT_ERR("Config response is too big"); > > Remove the BT_ERR, normally we don't print any error on > l2cap_send_disconn_req. > > >> + l2cap_send_disconn_req(conn, sk); >> + goto done; >> + } >> + >> /* throw out any old stored conf requests */ >> result = L2CAP_CONF_SUCCESS; >> len = l2cap_parse_conf_rsp(sk, rsp->data, > > Also, this is very rare crash. Even if we add all configure options to > req we won't overwrite it. Doesn't make sense to me send a > configuration option more than once in the same config_{rsp,req}. The > only way to crash l2cap is using a bugous remote stack or a stress test > tools. We need to be protected against those bugous stacks so I'll > ack your patch after you send the updated patch without the BT_ERR. > I am attaching patch to the mail, otherwise it may be screwed up by mailers. Regards, Andrei [-- Attachment #2: 0001-Bluetooth-Fix-kernel-crash-on-BT-stress-tests.patch --] [-- Type: text/x-patch, Size: 2127 bytes --] From af8cab3087cf4d4865ad88fb25d130ebea108db0 Mon Sep 17 00:00:00 2001 From: Andrei Emeltchenko <andrei.emeltchenko@nokia.com> Date: Tue, 16 Feb 2010 10:52:33 +0200 Subject: [PATCH] Bluetooth: Fix kernel crash on BT stress tests. Added very simple check that req buffer has enough space to fit configuration parameters. Shall be enough to reject packets with configuration size more than req buffer. Crash trace below [ 6069.659393] Unable to handle kernel paging request at virtual address 02000205 [ 6069.673034] Internal error: Oops: 805 [#1] PREEMPT ... [ 6069.727172] PC is at l2cap_add_conf_opt+0x70/0xf0 [l2cap] [ 6069.732604] LR is at l2cap_recv_frame+0x1350/0x2e78 [l2cap] ... [ 6070.030303] Backtrace: [ 6070.032806] [<bf1c2880>] (l2cap_add_conf_opt+0x0/0xf0 [l2cap]) from [<bf1c6624>] (l2cap_recv_frame+0x1350/0x2e78 [l2cap]) [ 6070.043823] r8:dc5d3100 r7:df2a91d6 r6:00000001 r5:df2a8000 r4:00000200 [ 6070.050659] [<bf1c52d4>] (l2cap_recv_frame+0x0/0x2e78 [l2cap]) from [<bf1c8408>] (l2cap_recv_acldata+0x2bc/0x350 [l2cap]) [ 6070.061798] [<bf1c814c>] (l2cap_recv_acldata+0x0/0x350 [l2cap]) from [<bf0037a4>] (hci_rx_task+0x244/0x478 [bluetooth]) [ 6070.072631] r6:dc647700 r5:00000001 r4:df2ab740 [ 6070.077362] [<bf003560>] (hci_rx_task+0x0/0x478 [bluetooth]) from [<c006b9fc>] (tasklet_action+0x78/0xd8) [ 6070.087005] [<c006b984>] (tasklet_action+0x0/0xd8) from [<c006c160>] Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@nokia.com> --- net/bluetooth/l2cap.c | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-) diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c index 930f987..f4258bc 100644 --- a/net/bluetooth/l2cap.c +++ b/net/bluetooth/l2cap.c @@ -2897,6 +2897,11 @@ static inline int l2cap_config_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hdr int len = cmd->len - sizeof(*rsp); char req[64]; + if (len > sizeof(req) - sizeof(struct l2cap_conf_req)) { + l2cap_send_disconn_req(conn, sk); + goto done; + } + /* throw out any old stored conf requests */ result = L2CAP_CONF_SUCCESS; len = l2cap_parse_conf_rsp(sk, rsp->data, -- 1.6.0.4 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] Bluetooth: Fix kernel crash on BT stress tests. 2010-03-15 10:57 ` Andrei Emeltchenko @ 2010-03-15 15:05 ` Gustavo F. Padovan 2010-03-19 8:31 ` Andrei Emeltchenko 0 siblings, 1 reply; 12+ messages in thread From: Gustavo F. Padovan @ 2010-03-15 15:05 UTC (permalink / raw) To: Andrei Emeltchenko; +Cc: linux-bluetooth, marcel * Andrei Emeltchenko <andrei.emeltchenko.news@gmail.com> [2010-03-15 12:57:= 18 +0200]: > Hi Gustavo >=20 > On Fri, Mar 12, 2010 at 12:41 AM, Gustavo F. Padovan > <padovan@profusion.mobi> wrote: > > Hi Andrei, > > > > * Andrei Emeltchenko <andrei.emeltchenko.news@gmail.com> [2010-02-16 12= :36:47 +0200]: > > > >> From 0135f732cb45e5e91062aca84a61a40b172200a4 Mon Sep 17 00:00:00 2001 > >> From: Andrei Emeltchenko <andrei.emeltchenko@nokia.com> > >> Date: Tue, 16 Feb 2010 10:52:33 +0200 > >> Subject: [PATCH] Bluetooth: Fix kernel crash on BT stress tests. > >> > >> Added very simple check that req buffer has enough space to > >> fit configuration parameters. Shall be enough to reject packets > >> with configuration size more than req buffer. > >> > >> Crash trace below > >> > >> [ 6069.659393] Unable to handle kernel paging request at virtual > >> address 02000205 > >> [ 6069.673034] Internal error: Oops: 805 [#1] PREEMPT > >> ... > >> [ 6069.727172] PC is at l2cap_add_conf_opt+0x70/0xf0 [l2cap] > >> [ 6069.732604] LR is at l2cap_recv_frame+0x1350/0x2e78 [l2cap] > >> ... > >> [ 6070.030303] Backtrace: > >> [ 6070.032806] [<bf1c2880>] (l2cap_add_conf_opt+0x0/0xf0 [l2cap]) from > >> [<bf1c6624>] (l2cap_recv_frame+0x1350/0x2e78 [l2cap]) > >> [ 6070.043823] =A0r8:dc5d3100 r7:df2a91d6 r6:00000001 r5:df2a8000 r4:0= 0000200 > >> [ 6070.050659] [<bf1c52d4>] (l2cap_recv_frame+0x0/0x2e78 [l2cap]) from > >> [<bf1c8408>] (l2cap_recv_acldata+0x2bc/0x350 [l2cap]) > >> [ 6070.061798] [<bf1c814c>] (l2cap_recv_acldata+0x0/0x350 [l2cap]) from > >> [<bf0037a4>] (hci_rx_task+0x244/0x478 [bluetooth]) > >> [ 6070.072631] =A0r6:dc647700 r5:00000001 r4:df2ab740 > >> [ 6070.077362] [<bf003560>] (hci_rx_task+0x0/0x478 [bluetooth]) from > >> [<c006b9fc>] (tasklet_action+0x78/0xd8) > >> [ 6070.087005] [<c006b984>] (tasklet_action+0x0/0xd8) from [<c006c160>] > >> > >> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@nokia.com> > >> --- > >> =A0net/bluetooth/l2cap.c | =A0 =A06 ++++++ > >> =A01 files changed, 6 insertions(+), 0 deletions(-) > >> > >> diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c > >> index 400efa2..69b7280 100644 > >> --- a/net/bluetooth/l2cap.c > >> +++ b/net/bluetooth/l2cap.c > >> @@ -2830,6 +2830,12 @@ static inline int l2cap_config_rsp(struct > >> l2cap_conn *conn, struct l2cap_cmd_hdr > >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 int len =3D cmd->len - siz= eof(*rsp); > >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 char req[64]; > >> > >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (len > sizeof(req) - size= of(struct l2cap_conf_req)) { > >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 BT_ERR("Conf= ig response is too big"); > > > > Remove the BT_ERR, normally we don't print any error on > > l2cap_send_disconn_req. > > > > > >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 l2cap_send_d= isconn_req(conn, sk); > >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto done; > >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 } > >> + > >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* throw out any old store= d conf requests */ > >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 result =3D L2CAP_CONF_SUCC= ESS; > >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 len =3D l2cap_parse_conf_r= sp(sk, rsp->data, > > > > Also, this is very rare crash. Even if we add all configure options to > > req we won't overwrite it. Doesn't make sense to me send a > > configuration option more than once in the same config_{rsp,req}. The > > only way to crash l2cap is using a bugous remote stack or a stress test > > tools. We need to be protected against those bugous stacks so I'll > > ack your patch after you send the updated patch without the BT_ERR. > > >=20 > I am attaching patch to the mail, otherwise it may be screwed up by maile= rs. Acked-by: Gustavo F. Padovan <gustavo@padovan.org> >=20 > Regards, > Andrei --=20 Gustavo F. Padovan http://padovan.org ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Bluetooth: Fix kernel crash on BT stress tests. 2010-03-15 15:05 ` Gustavo F. Padovan @ 2010-03-19 8:31 ` Andrei Emeltchenko 0 siblings, 0 replies; 12+ messages in thread From: Andrei Emeltchenko @ 2010-03-19 8:31 UTC (permalink / raw) To: Gustavo F. Padovan; +Cc: linux-bluetooth, marcel [-- Attachment #1: Type: text/plain, Size: 3905 bytes --] Hi, On Mon, Mar 15, 2010 at 5:05 PM, Gustavo F. Padovan <gustavo@padovan.org> wrote: > * Andrei Emeltchenko <andrei.emeltchenko.news@gmail.com> [2010-03-15 12:57:18 +0200]: > >> Hi Gustavo >> >> On Fri, Mar 12, 2010 at 12:41 AM, Gustavo F. Padovan >> <padovan@profusion.mobi> wrote: >> > Hi Andrei, >> > >> > * Andrei Emeltchenko <andrei.emeltchenko.news@gmail.com> [2010-02-16 12:36:47 +0200]: >> > >> >> From 0135f732cb45e5e91062aca84a61a40b172200a4 Mon Sep 17 00:00:00 2001 >> >> From: Andrei Emeltchenko <andrei.emeltchenko@nokia.com> >> >> Date: Tue, 16 Feb 2010 10:52:33 +0200 >> >> Subject: [PATCH] Bluetooth: Fix kernel crash on BT stress tests. >> >> >> >> Added very simple check that req buffer has enough space to >> >> fit configuration parameters. Shall be enough to reject packets >> >> with configuration size more than req buffer. >> >> >> >> Crash trace below >> >> >> >> [ 6069.659393] Unable to handle kernel paging request at virtual >> >> address 02000205 >> >> [ 6069.673034] Internal error: Oops: 805 [#1] PREEMPT >> >> ... >> >> [ 6069.727172] PC is at l2cap_add_conf_opt+0x70/0xf0 [l2cap] >> >> [ 6069.732604] LR is at l2cap_recv_frame+0x1350/0x2e78 [l2cap] >> >> ... >> >> [ 6070.030303] Backtrace: >> >> [ 6070.032806] [<bf1c2880>] (l2cap_add_conf_opt+0x0/0xf0 [l2cap]) from >> >> [<bf1c6624>] (l2cap_recv_frame+0x1350/0x2e78 [l2cap]) >> >> [ 6070.043823] r8:dc5d3100 r7:df2a91d6 r6:00000001 r5:df2a8000 r4:00000200 >> >> [ 6070.050659] [<bf1c52d4>] (l2cap_recv_frame+0x0/0x2e78 [l2cap]) from >> >> [<bf1c8408>] (l2cap_recv_acldata+0x2bc/0x350 [l2cap]) >> >> [ 6070.061798] [<bf1c814c>] (l2cap_recv_acldata+0x0/0x350 [l2cap]) from >> >> [<bf0037a4>] (hci_rx_task+0x244/0x478 [bluetooth]) >> >> [ 6070.072631] r6:dc647700 r5:00000001 r4:df2ab740 >> >> [ 6070.077362] [<bf003560>] (hci_rx_task+0x0/0x478 [bluetooth]) from >> >> [<c006b9fc>] (tasklet_action+0x78/0xd8) >> >> [ 6070.087005] [<c006b984>] (tasklet_action+0x0/0xd8) from [<c006c160>] >> >> >> >> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@nokia.com> >> >> --- >> >> net/bluetooth/l2cap.c | 6 ++++++ >> >> 1 files changed, 6 insertions(+), 0 deletions(-) >> >> >> >> diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c >> >> index 400efa2..69b7280 100644 >> >> --- a/net/bluetooth/l2cap.c >> >> +++ b/net/bluetooth/l2cap.c >> >> @@ -2830,6 +2830,12 @@ static inline int l2cap_config_rsp(struct >> >> l2cap_conn *conn, struct l2cap_cmd_hdr >> >> int len = cmd->len - sizeof(*rsp); >> >> char req[64]; >> >> >> >> + if (len > sizeof(req) - sizeof(struct l2cap_conf_req)) { >> >> + BT_ERR("Config response is too big"); >> > >> > Remove the BT_ERR, normally we don't print any error on >> > l2cap_send_disconn_req. >> > >> > >> >> + l2cap_send_disconn_req(conn, sk); >> >> + goto done; >> >> + } >> >> + >> >> /* throw out any old stored conf requests */ >> >> result = L2CAP_CONF_SUCCESS; >> >> len = l2cap_parse_conf_rsp(sk, rsp->data, >> > >> > Also, this is very rare crash. Even if we add all configure options to >> > req we won't overwrite it. Doesn't make sense to me send a >> > configuration option more than once in the same config_{rsp,req}. The >> > only way to crash l2cap is using a bugous remote stack or a stress test >> > tools. We need to be protected against those bugous stacks so I'll >> > ack your patch after you send the updated patch without the BT_ERR. >> > >> >> I am attaching patch to the mail, otherwise it may be screwed up by mailers. > > Acked-by: Gustavo F. Padovan <gustavo@padovan.org> Thanks. I am attaching patch. Regards, Andrei [-- Attachment #2: 0001-Bluetooth-Fix-kernel-crash-on-BT-stress-tests.patch --] [-- Type: text/x-patch, Size: 2178 bytes --] From af8cab3087cf4d4865ad88fb25d130ebea108db0 Mon Sep 17 00:00:00 2001 From: Andrei Emeltchenko <andrei.emeltchenko@nokia.com> Date: Tue, 16 Feb 2010 10:52:33 +0200 Subject: [PATCH] Bluetooth: Fix kernel crash on BT stress tests. Added very simple check that req buffer has enough space to fit configuration parameters. Shall be enough to reject packets with configuration size more than req buffer. Crash trace below [ 6069.659393] Unable to handle kernel paging request at virtual address 02000205 [ 6069.673034] Internal error: Oops: 805 [#1] PREEMPT ... [ 6069.727172] PC is at l2cap_add_conf_opt+0x70/0xf0 [l2cap] [ 6069.732604] LR is at l2cap_recv_frame+0x1350/0x2e78 [l2cap] ... [ 6070.030303] Backtrace: [ 6070.032806] [<bf1c2880>] (l2cap_add_conf_opt+0x0/0xf0 [l2cap]) from [<bf1c6624>] (l2cap_recv_frame+0x1350/0x2e78 [l2cap]) [ 6070.043823] r8:dc5d3100 r7:df2a91d6 r6:00000001 r5:df2a8000 r4:00000200 [ 6070.050659] [<bf1c52d4>] (l2cap_recv_frame+0x0/0x2e78 [l2cap]) from [<bf1c8408>] (l2cap_recv_acldata+0x2bc/0x350 [l2cap]) [ 6070.061798] [<bf1c814c>] (l2cap_recv_acldata+0x0/0x350 [l2cap]) from [<bf0037a4>] (hci_rx_task+0x244/0x478 [bluetooth]) [ 6070.072631] r6:dc647700 r5:00000001 r4:df2ab740 [ 6070.077362] [<bf003560>] (hci_rx_task+0x0/0x478 [bluetooth]) from [<c006b9fc>] (tasklet_action+0x78/0xd8) [ 6070.087005] [<c006b984>] (tasklet_action+0x0/0xd8) from [<c006c160>] Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@nokia.com> Acked-by: Gustavo F. Padovan <gustavo@padovan.org> --- net/bluetooth/l2cap.c | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-) diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c index 930f987..f4258bc 100644 --- a/net/bluetooth/l2cap.c +++ b/net/bluetooth/l2cap.c @@ -2897,6 +2897,11 @@ static inline int l2cap_config_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hdr int len = cmd->len - sizeof(*rsp); char req[64]; + if (len > sizeof(req) - sizeof(struct l2cap_conf_req)) { + l2cap_send_disconn_req(conn, sk); + goto done; + } + /* throw out any old stored conf requests */ result = L2CAP_CONF_SUCCESS; len = l2cap_parse_conf_rsp(sk, rsp->data, -- 1.6.0.4 ^ permalink raw reply related [flat|nested] 12+ messages in thread
end of thread, other threads:[~2010-03-19 8:31 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-02-16 10:36 [PATCH] Bluetooth: Fix kernel crash on BT stress tests Andrei Emeltchenko 2010-02-16 11:34 ` Gustavo F. Padovan 2010-02-16 11:58 ` Andrei Emeltchenko 2010-02-16 12:16 ` Andrei Emeltchenko 2010-02-17 23:57 ` Gustavo F. Padovan 2010-02-18 16:18 ` Andrei Emeltchenko 2010-03-11 14:54 ` Andrei Emeltchenko 2010-03-11 17:16 ` Marcel Holtmann 2010-03-11 22:41 ` Gustavo F. Padovan 2010-03-15 10:57 ` Andrei Emeltchenko 2010-03-15 15:05 ` Gustavo F. Padovan 2010-03-19 8:31 ` Andrei Emeltchenko
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox