Re: [PATCH] work around for l2cap NULL dereference in l2cap_conn_start

From: Andrei Emeltchenko
Date: Thu Mar 24 2011 - 11:37:46 EST

Next message: Serge E. Hallyn: "Re: [PATCH v2] net: don't allow CAP_NET_ADMIN to load non-netdevkernel modules"
Previous message: Dan Magenheimer: "RE: linux-next: manual merge of the cleancache tree with Linus' tree"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Gustavo,

On Mon, Feb 28, 2011 at 7:03 AM, David Fries <david@xxxxxxxxx> wrote:
> On Sun, Feb 27, 2011 at 04:15:45PM -0300, Gustavo F. Padovan wrote:
>> I pushed the following patch to bluetooth-2.6 tree. It should fix the problem
>> by avoiding connections to be accepted before a L2CAP info response comes:
>
> Is
> git://git.kernel.org/pub/scm/linux/kernel/git/padovan/bluetooth-2.6.git
> the bluetooth-2.6 tree you mentioned? I don't see your patch there.
> As a side note, the inline patch in your e-mail has the tabs replaced by
> spaces, once I changed them, it applied cleanly.
>
> I first reverted to the base N900 kernel-power-2.6.28 46 (none of my
> changes or debugging), it crashed as expected. I then applied your
> patch 743400e0, and it still crashed.

the same for me. Your patch with adding BT_CONNECT2 check seems
have no effect since sk_state == BT_CONNECT2 for my case.

I've posted series of patches which fixes the issue but I believe it is better
to keep check for parent.

Search my patches by "[RFCv1 0/3] Set of patches fixing kernel crash"

Regards,
Andrei

> I added back the
> l2cap_conn_start parent check and some debugging in af_bluetooth.c
> dmesg debug output and patches follow.
>
> I haven't at all looked into the bluetooth protocol, but what connect
> sequence difference does it make if I power on the bluetooth headset
> and press play on the headset before it automatically pairs with the
> N900, vs power on bluetooth headset, wait for it to pair then press
> play? I ask this partly because I'm curiouse, but mostly how I
> trigger the bug. This is with pulse audio running, but no
> applications playing audio or responding to a play event from the
> headset.
>
> [ 443.424560] bt_accept_dequeue, parent cd54ba00 newsock c81f0180, defer_setup && BT_CONNECT2
> [ 443.427368] avoided crash in l2cap_conn_start sk c6d3f600 result 1 status 2
> [ 443.518463] bt_accept_dequeue, parent cdee9c00 newsock c81f0000, BT_CONNECTED
> [ 443.729736] bt_accept_dequeue, parent cd54be00 newsock c81f0000, BT_CONNECTED
> [ 443.813537] bt_accept_dequeue, parent cd54b600 newsock c81f0180, defer_setup && BT_CONNECT2
>
> From 5bc80fafac43b6698e271f1246cb24e596bf2ef1 Mon Sep 17 00:00:00 2001
> From: David Fries <david@xxxxxxxxx>
> Date: Sun, 6 Feb 2011 14:34:49 -0600
> Subject: [PATCH 1/2] work around for l2cap NULL dereference in l2cap_conn_start print sk
>
> ---
> net/bluetooth/l2cap.c | 11 ++++++++++-
> 1 files changed, 10 insertions(+), 1 deletions(-)
>
> diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
> index fda7741..ff05f51 100644
> --- a/net/bluetooth/l2cap.c
> +++ b/net/bluetooth/l2cap.c
> @@ -400,7 +400,16 @@ static void l2cap_conn_start(struct l2cap_conn *conn)
> struct sock *parent = bt_sk(sk)->parent;
> rsp.result = cpu_to_le16(L2CAP_CR_PEND);
> rsp.status = cpu_to_le16(L2CAP_CS_AUTHOR_PEND);
> - parent->sk_data_ready(parent, 0);
> + if(!parent) {
> + printk(KERN_DEBUG "avoided "
> + "crash in %s sk %p "
> + "result %d status %d\n",
> + __func__, sk,
> + rsp.result, rsp.status);
> + } else {
> + parent->sk_data_ready(parent,
> + 0);
> + }
>
> } else {
> sk->sk_state = BT_CONFIG;
> --
> 1.7.2.3
>
>
> From 42b9a6ef68a1cd0ef025b826afcfb0ef23342fe5 Mon Sep 17 00:00:00 2001
> From: David Fries <david@xxxxxxxxx>
> Date: Sun, 27 Feb 2011 21:50:14 -0600
> Subject: [PATCH 2/2] af_bluetooth.c debug
>
> ---
> net/bluetooth/af_bluetooth.c | 12 ++++++++++++
> 1 files changed, 12 insertions(+), 0 deletions(-)
>
> diff --git a/net/bluetooth/af_bluetooth.c b/net/bluetooth/af_bluetooth.c
> index 8e910f1..57cd360 100644
> --- a/net/bluetooth/af_bluetooth.c
> +++ b/net/bluetooth/af_bluetooth.c
> @@ -211,6 +211,18 @@ struct sock *bt_accept_dequeue(struct sock *parent, struct socket *newsock)
> continue;
> }
>
> + if (bt_sk(parent)->defer_setup && sk->sk_state == BT_CONNECT2)
> + printk("%s, parent %p newsock %p, "
> + "defer_setup && BT_CONNECT2\n", __func__,
> + parent, newsock);
> + if (sk->sk_state == BT_CONNECTED)
> + printk("%s, parent %p newsock %p, "
> + "BT_CONNECTED\n", __func__,
> + parent, newsock);
> + if (!newsock)
> + printk("%s, parent %p newsock %p, "
> + "!newsock\n", __func__,
> + parent, newsock);
> if ((bt_sk(parent)->defer_setup && sk->sk_state == BT_CONNECT2)
> || sk->sk_state == BT_CONNECTED || !newsock) {
> bt_accept_unlink(sk);
> --
> 1.7.2.3
>
>
>> commit 743400e01a33779f93b79c84a1b0d1a2d27338c8
>> Author: Gustavo F. Padovan <padovan@xxxxxxxxxxxxxx>
>> Date: Sun Feb 27 16:05:07 2011 -0300
>>
>> Bluetooth: Don't accept l2cap connection before info_rsp
>>
>> When using defer_setup accepting a connection before receive the L2CAP
>> Info Response for the connection lead us to a crash in l2cap_conn_start(.
>>
>> Reported-by: David Fries <david@xxxxxxxxx>
>> Reported-by: Liang Bao <tim.bao@xxxxxxxxx>
>> Signed-off-by: Gustavo F. Padovan <padovan@xxxxxxxxxxxxxx>
>>
>> diff --git a/net/bluetooth/af_bluetooth.c b/net/bluetooth/af_bluetooth.c
>> index c4cf3f5..a8ca42b 100644
>> --- a/net/bluetooth/af_bluetooth.c
>> +++ b/net/bluetooth/af_bluetooth.c
>> @@ -211,8 +211,8 @@ struct sock *bt_accept_dequeue(struct sock *parent, struct socket *newsock)
>> continue;
>> }
>>
>> - if (sk->sk_state == BT_CONNECTED || !newsock ||
>> - bt_sk(parent)->defer_setup) {
>> + if ((bt_sk(parent)->defer_setup && sk->sk_state == BT_CONNECT2)
>> + || sk->sk_state == BT_CONNECTED || !newsock) {
>> bt_accept_unlink(sk);
>> if (newsock)
>> sock_graft(sk, newsock);
>>
>>
>> --
>> Gustavo F. Padovan
>> http://profusion.mobi
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
> --
> David Fries <david@xxxxxxxxx>
> http://fries.net/~david/ (PGP encryption key available)
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Serge E. Hallyn: "Re: [PATCH v2] net: don't allow CAP_NET_ADMIN to load non-netdevkernel modules"
Previous message: Dan Magenheimer: "RE: linux-next: manual merge of the cleancache tree with Linus' tree"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]