[PATCH] work around for l2cap NULL dereference in l2cap_conn_start

[PATCH] work around for l2cap NULL dereference in l2cap_conn_start

[David Fries]

bt_sk(sk)->parent can be NULL in l2cap_conn_start in state BT_CONNECT2
at least when a headset device pairs and the play button was pressed
right before pairing.

Signed-off-by: David Fries <david@fries.net>
---
I removed the printk, can this be merged to the bluetooth next tree?

On Mon, Feb 14, 2011 at 03:40:46PM -0600, Andrei Warkentin wrote:
> FWIW still need it in 2.6.36.

Andrei, I'm curious, what's your hardware hardware and bluetooth
device that's trigginer the crash?

> On Mon, Feb 14, 2011 at 8:56 AM, Gustavo F. Padovan
> <padovan@profusion.mobi> wrote:
> > Hi David,
> >
> > * David Fries <david@fries.net> [2011-02-10 21:53:09 -0600]:
> >
> >> Here's a patch to avoid a very repeatable crash in the N900.  If I
> >> take a Motorola S305 bluetooth headset that was previously paried with
> >> the N900, turn it on, and press the play button before the headphones
> >> automatically pair with the cell phone, the N900 will crash (and
> >> reboot) in pairing.  If I wait until after they have paired there
> >> isn't any problem.  The patch is against the kernel-power
> >> 2.6.28-maemo46 by Thomas Tanner, the stock Nokia PR1.2 oops looked
> >> the same, I just haven't gone back to that kernel.
> >
> > This is a very old kernel. You need to check this issue against
> > bluetooth-next-2.6.

 net/bluetooth/l2cap.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
index ed83c1f..a7aa4d9 100644
--- a/net/bluetooth/l2cap.c
+++ b/net/bluetooth/l2cap.c
@@ -408,7 +408,8 @@ 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)
+						parent->sk_data_ready(parent,0);
 
 				} else {
 					sk->sk_state = BT_CONFIG;
-- 
1.7.2.3

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

[Liang Bao]

Hi, David, Andrew et al.

2011/2/21 David Fries <david@fries.net>:
> bt_sk(sk)->parent can be NULL in l2cap_conn_start in state BT_CONNECT2
> at least when a headset device pairs and the play button was pressed
> right before pairing.
>
> Signed-off-by: David Fries <david@fries.net>
> ---
> I removed the printk, can this be merged to the bluetooth next tree?
>
> On Mon, Feb 14, 2011 at 03:40:46PM -0600, Andrei Warkentin wrote:
>> FWIW still need it in 2.6.36.
>
> Andrei, I'm curious, what's your hardware hardware and bluetooth
> device that's trigginer the crash?
I  once submitted an issue observed with Android+Motorola S305 stereo
headset. It's still open in launchpad:
https://bugs.launchpad.net/ubuntu/+source/bluez/+bug/513642. Hope this
helps. Thanks.
>
>> On Mon, Feb 14, 2011 at 8:56 AM, Gustavo F. Padovan
>> <padovan@profusion.mobi> wrote:
>> > Hi David,
>> >
>> > * David Fries <david@fries.net> [2011-02-10 21:53:09 -0600]:
>> >
>> >> Here's a patch to avoid a very repeatable crash in the N900.  If I
>> >> take a Motorola S305 bluetooth headset that was previously paried with
>> >> the N900, turn it on, and press the play button before the headphones
>> >> automatically pair with the cell phone, the N900 will crash (and
>> >> reboot) in pairing.  If I wait until after they have paired there
>> >> isn't any problem.  The patch is against the kernel-power
>> >> 2.6.28-maemo46 by Thomas Tanner, the stock Nokia PR1.2 oops looked
>> >> the same, I just haven't gone back to that kernel.
>> >
>> > This is a very old kernel. You need to check this issue against
>> > bluetooth-next-2.6.
>
>  net/bluetooth/l2cap.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
> index ed83c1f..a7aa4d9 100644
> --- a/net/bluetooth/l2cap.c
> +++ b/net/bluetooth/l2cap.c
> @@ -408,7 +408,8 @@ 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)
> +                                               parent->sk_data_ready(parent,0);
>
>                                } else {
>                                        sk->sk_state = BT_CONFIG;
> --
> 1.7.2.3
>
> --
> 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
>

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

[Gustavo F. Padovan]

Hi David and Liang,

* Liang Bao <tim.bao@gmail.com> [2011-02-21 14:41:29 +0800]:

> Hi, David, Andrew et al.
> 
> 2011/2/21 David Fries <david@fries.net>:
> > bt_sk(sk)->parent can be NULL in l2cap_conn_start in state BT_CONNECT2
> > at least when a headset device pairs and the play button was pressed
> > right before pairing.
> >
> > Signed-off-by: David Fries <david@fries.net>
> > ---
> > I removed the printk, can this be merged to the bluetooth next tree?
> >
> > On Mon, Feb 14, 2011 at 03:40:46PM -0600, Andrei Warkentin wrote:
> >> FWIW still need it in 2.6.36.
> >
> > Andrei, I'm curious, what's your hardware hardware and bluetooth
> > device that's trigginer the crash?
> I  once submitted an issue observed with Android+Motorola S305 stereo
> headset. It's still open in launchpad:
> https://bugs.launchpad.net/ubuntu/+source/bluez/+bug/513642. Hope this
> helps. Thanks.
> >
> >> On Mon, Feb 14, 2011 at 8:56 AM, Gustavo F. Padovan
> >> <padovan@profusion.mobi> wrote:
> >> > Hi David,
> >> >
> >> > * David Fries <david@fries.net> [2011-02-10 21:53:09 -0600]:
> >> >
> >> >> Here's a patch to avoid a very repeatable crash in the N900.  If I
> >> >> take a Motorola S305 bluetooth headset that was previously paried with
> >> >> the N900, turn it on, and press the play button before the headphones
> >> >> automatically pair with the cell phone, the N900 will crash (and
> >> >> reboot) in pairing.  If I wait until after they have paired there
> >> >> isn't any problem.  The patch is against the kernel-power
> >> >> 2.6.28-maemo46 by Thomas Tanner, the stock Nokia PR1.2 oops looked
> >> >> the same, I just haven't gone back to that kernel.
> >> >
> >> > This is a very old kernel. You need to check this issue against
> >> > bluetooth-next-2.6.
> >
> >  net/bluetooth/l2cap.c |    3 ++-
> >  1 files changed, 2 insertions(+), 1 deletions(-)
> >
> > diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
> > index ed83c1f..a7aa4d9 100644
> > --- a/net/bluetooth/l2cap.c
> > +++ b/net/bluetooth/l2cap.c
> > @@ -408,7 +408,8 @@ 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)
> > +                                               parent->sk_data_ready(parent,0);
> >
> >                                } else {
> >                                        sk->sk_state = BT_CONFIG;

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:

commit 743400e01a33779f93b79c84a1b0d1a2d27338c8
Author: Gustavo F. Padovan <padovan@profusion.mobi>
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@fries.net>
    Reported-by: Liang Bao <tim.bao@gmail.com>
    Signed-off-by: Gustavo F. Padovan <padovan@profusion.mobi>

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

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

[David Fries]

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.  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@fries.net>
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@fries.net>
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@profusion.mobi>
> 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@fries.net>
>     Reported-by: Liang Bao <tim.bao@gmail.com>
>     Signed-off-by: Gustavo F. Padovan <padovan@profusion.mobi>
> 
> 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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
David Fries <david@fries.net>
http://fries.net/~david/ (PGP encryption key available)

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

[Gustavo F. Padovan]

Hi David,

* David Fries <david@fries.net> [2011-02-27 23:03:40 -0600]:

> 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.  I added back the
> l2cap_conn_start parent check and some debugging in af_bluetooth.c
> dmesg debug output and patches follow.

I want to see a test with this patch and a recent kernel. We added many fixes
to stack in the last two years. Can you test this scenario?

-- 
Gustavo F. Padovan
http://profusion.mobi

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

[David Fries]

On Mon, Feb 28, 2011 at 02:30:22PM -0300, Gustavo F. Padovan wrote:
> Hi David,
> 
> * David Fries <david@fries.net> [2011-02-27 23:03:40 -0600]:
> 
> > 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.  I added back the
> > l2cap_conn_start parent check and some debugging in af_bluetooth.c
> > dmesg debug output and patches follow.
> 
> I want to see a test with this patch and a recent kernel. We added many fixes
> to stack in the last two years. Can you test this scenario?

I'm sorry, but apparently not, at least this post says 2.6.37 isn't
going to happen for the N900 and Maemo.
http://forums.internettablettalk.com/showthread.php?t=70082

I tried 2.6.37-n900 from
git://gitorious.org/nokia-n900-kernel/nokia-n900-kernel.git anyway,
but the display visibly degrades like it isn't being updated and
doesn't apparently get any further.  I don't have anyway to debug it
further.

-- 
David Fries <david@fries.net>
http://fries.net/~david/ (PGP encryption key available)

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

[Gustavo F. Padovan]

Hi David,

* David Fries <david@fries.net> [2011-03-02 00:19:10 -0600]:

> On Mon, Feb 28, 2011 at 02:30:22PM -0300, Gustavo F. Padovan wrote:
> > Hi David,
> > 
> > * David Fries <david@fries.net> [2011-02-27 23:03:40 -0600]:
> > 
> > > 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.  I added back the
> > > l2cap_conn_start parent check and some debugging in af_bluetooth.c
> > > dmesg debug output and patches follow.
> > 
> > I want to see a test with this patch and a recent kernel. We added many fixes
> > to stack in the last two years. Can you test this scenario?
> 
> I'm sorry, but apparently not, at least this post says 2.6.37 isn't
> going to happen for the N900 and Maemo.
> http://forums.internettablettalk.com/showthread.php?t=70082
> 
> I tried 2.6.37-n900 from
> git://gitorious.org/nokia-n900-kernel/nokia-n900-kernel.git anyway,
> but the display visibly degrades like it isn't being updated and
> doesn't apparently get any further.  I don't have anyway to debug it
> further.

I think you can test this in a desktop machine.

-- 
Gustavo F. Padovan
http://profusion.mobi

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

[David Fries]

On Fri, Mar 04, 2011 at 11:12:57PM -0300, Gustavo F. Padovan wrote:
> Hi David,
> 
> * David Fries <david@fries.net> [2011-03-02 00:19:10 -0600]:
> 
> > On Mon, Feb 28, 2011 at 02:30:22PM -0300, Gustavo F. Padovan wrote:
> > > Hi David,
> > > 
> > > * David Fries <david@fries.net> [2011-02-27 23:03:40 -0600]:
> > > 
> > > > 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.  I added back the
> > > > l2cap_conn_start parent check and some debugging in af_bluetooth.c
> > > > dmesg debug output and patches follow.
> > > 
> > > I want to see a test with this patch and a recent kernel. We added many fixes
> > > to stack in the last two years. Can you test this scenario?
> > 
> > I'm sorry, but apparently not, at least this post says 2.6.37 isn't
> > going to happen for the N900 and Maemo.
> > http://forums.internettablettalk.com/showthread.php?t=70082
> > 
> > I tried 2.6.37-n900 from
> > git://gitorious.org/nokia-n900-kernel/nokia-n900-kernel.git anyway,
> > but the display visibly degrades like it isn't being updated and
> > doesn't apparently get any further.  I don't have anyway to debug it
> > further.
> 
> I think you can test this in a desktop machine.

I've not been able to reproduce the bug on my desktop, and not for a
lack of trying.
2.6.28, l2cap_conn_start doesn't dereference parent (so it wouldn't
crash there anyway)  N900 must have some backported patches.
2.6.30 first kernel with that code
2.6.30, 2.6.37+, 2.6.38-rc7+, with a debug patch to print
the sk and parent in l2cap_conn_start, only executes the BT_CONNECT2
path in l2cap_conn_start maybe only one in five or less times and I
have yet to see it (on the desktop) have a NULL parent.
This is with the following USB Bluetooth dongle,
Cambridge Silicon Radio, Ltd Bluetooth Dongle (HCI mode)

Looks like I'm not going to be any more help verifying it is or isn't
fixed with a newer bluetooth stack.  Here's a post from Liang Bao.

On Tue, Mar 15, 2011 at 10:42:07PM +0800, Liang Bao wrote:
> Hi,
>
> Sorry for get back so late. I am really crazy busy with my project. I tested
> with 2.6.35-27 kernel + ubuntu 10.10 just now and seems the issue is really
> gone. Hcidump attached for your reference. It's more than one year so it
> might need some more time to figure out the difference of logs but as said,
> I am really hard to find out that time. Wondering if you would like to
> compare this with the one I attached into the mailing list a year ago.

-- 
David Fries <david@fries.net>
http://fries.net/~david/ (PGP encryption key available)

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

[Andrei Emeltchenko]

Hi Gustavo,

On Mon, Feb 28, 2011 at 7:03 AM, David Fries <david@fries.net> 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@fries.net>
> 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@fries.net>
> 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@profusion.mobi>
>> 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@fries.net>
>>     Reported-by: Liang Bao <tim.bao@gmail.com>
>>     Signed-off-by: Gustavo F. Padovan <padovan@profusion.mobi>
>>
>> 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@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
> --
> David Fries <david@fries.net>
> 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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

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

[Andrei Warkentin]

Hi all,

I don't have an S305 headset at the moment to play with this, but, our
tree (2.6.36) has
a fix like this for this issue.


 				if (bt_sk(sk)->defer_setup) {
 					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)
+						parent->sk_data_ready(parent, 0);

 				} else {
 					sk->sk_state = BT_CONFIG;
 					rsp.result = cpu_to_le16(L2CAP_CR_SUCCESS);

The comment is:

    Bluetooth: Hack: Don't dereference null pointer.

    This avoids the S305 panic during incoming connection.

    S305 sends PSM 25 L2CAP connection request before the L2CAP info response.
    When we receive that info response we crash on null pointer here.

Sorry for the wait,
A

On Sun, Feb 20, 2011 at 10:36 PM, David Fries <david@fries.net> wrote:
> bt_sk(sk)->parent can be NULL in l2cap_conn_start in state BT_CONNECT2
> at least when a headset device pairs and the play button was pressed
> right before pairing.
>
> Signed-off-by: David Fries <david@fries.net>
> ---
> I removed the printk, can this be merged to the bluetooth next tree?
>
> On Mon, Feb 14, 2011 at 03:40:46PM -0600, Andrei Warkentin wrote:
>> FWIW still need it in 2.6.36.
>
> Andrei, I'm curious, what's your hardware hardware and bluetooth
> device that's trigginer the crash?
>
>> On Mon, Feb 14, 2011 at 8:56 AM, Gustavo F. Padovan
>> <padovan@profusion.mobi> wrote:
>> > Hi David,
>> >
>> > * David Fries <david@fries.net> [2011-02-10 21:53:09 -0600]:
>> >
>> >> Here's a patch to avoid a very repeatable crash in the N900.  If I
>> >> take a Motorola S305 bluetooth headset that was previously paried with
>> >> the N900, turn it on, and press the play button before the headphones
>> >> automatically pair with the cell phone, the N900 will crash (and
>> >> reboot) in pairing.  If I wait until after they have paired there
>> >> isn't any problem.  The patch is against the kernel-power
>> >> 2.6.28-maemo46 by Thomas Tanner, the stock Nokia PR1.2 oops looked
>> >> the same, I just haven't gone back to that kernel.
>> >
>> > This is a very old kernel. You need to check this issue against
>> > bluetooth-next-2.6.
>
>  net/bluetooth/l2cap.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
> index ed83c1f..a7aa4d9 100644
> --- a/net/bluetooth/l2cap.c
> +++ b/net/bluetooth/l2cap.c
> @@ -408,7 +408,8 @@ 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)
> +                                               parent->sk_data_ready(parent,0);
>
>                                } else {
>                                        sk->sk_state = BT_CONFIG;
> --
> 1.7.2.3
>
>