Re: [PATCH] Bluetooth: Fix kernel crash on BT stress tests.

[Andrei Emeltchenko <andrei.emeltchenko.news@gmail.com>]

[-- 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