* [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;
as well as URLs for NNTP newsgroup(s).