All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] bluetooth: Fix NULL pointer dereference issue
@ 2010-10-28 10:52 Yuri Ershov
  2010-10-29 21:43 ` Marcel Holtmann
  0 siblings, 1 reply; 6+ messages in thread
From: Yuri Ershov @ 2010-10-28 10:52 UTC (permalink / raw)
  To: marcel, davem, padovan, jprvita
  Cc: linux-bluetooth, ville.tervo, andrei.emeltchenko, Yuri Ershov

This patch fixes NULL pointer dereference at running test with
connect-transfer-disconnect in loop. The problem conditions are the
following: there are 2 BT devices. The first one listens and
receives (l2test -r), the second one makes "connect-disconnect-
connect..." sequence (l2test -c -b 1000 -i hci0 -P 10 <addr>).
After some time this will cause the race between functions
bt_accept_dequeue and l2cap_chan_del. The function l2cap_chan_del
sets the socket state to BT_CLOSED, unlinks and kills the socket
in the middle of bt_accept_dequeue, then at running the removed code
kernel oops appears.

Signed-off-by: Yuri Ershov <ext-yuri.ershov@nokia.com>
---
 net/bluetooth/af_bluetooth.c |    7 -------
 1 files changed, 0 insertions(+), 7 deletions(-)

diff --git a/net/bluetooth/af_bluetooth.c b/net/bluetooth/af_bluetooth.c
index 421c45b..47c107e 100644
--- a/net/bluetooth/af_bluetooth.c
+++ b/net/bluetooth/af_bluetooth.c
@@ -204,13 +204,6 @@ struct sock *bt_accept_dequeue(struct sock *parent, struct socket *newsock)
 
 		lock_sock(sk);
 
-		/* FIXME: Is this check still needed */
-		if (sk->sk_state == BT_CLOSED) {
-			release_sock(sk);
-			bt_accept_unlink(sk);
-			continue;
-		}
-
 		if (sk->sk_state == BT_CONNECTED || !newsock ||
 						bt_sk(parent)->defer_setup) {
 			bt_accept_unlink(sk);
-- 
1.6.3.3

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH] bluetooth: Fix NULL pointer dereference issue
  2010-10-28 10:52 [PATCH] bluetooth: Fix NULL pointer dereference issue Yuri Ershov
@ 2010-10-29 21:43 ` Marcel Holtmann
  0 siblings, 0 replies; 6+ messages in thread
From: Marcel Holtmann @ 2010-10-29 21:43 UTC (permalink / raw)
  To: Yuri Ershov
  Cc: davem, padovan, jprvita, linux-bluetooth, ville.tervo,
	andrei.emeltchenko

Hi Yuri,

> This patch fixes NULL pointer dereference at running test with
> connect-transfer-disconnect in loop. The problem conditions are the
> following: there are 2 BT devices. The first one listens and
> receives (l2test -r), the second one makes "connect-disconnect-
> connect..." sequence (l2test -c -b 1000 -i hci0 -P 10 <addr>).
> After some time this will cause the race between functions
> bt_accept_dequeue and l2cap_chan_del. The function l2cap_chan_del
> sets the socket state to BT_CLOSED, unlinks and kills the socket
> in the middle of bt_accept_dequeue, then at running the removed code
> kernel oops appears.

it could be that we have this in here for RFCOMM. Can you retest it with
this with using RFCOMM and see if we get dangling RFCOMM sockets. You
need to test both directions, incoming and outgoing.

Regards

Marcel



^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] bluetooth: Fix NULL pointer dereference issue
  2010-10-25 12:23   ` Yuri Ershov
@ 2010-10-28  9:58     ` Gustavo F. Padovan
  0 siblings, 0 replies; 6+ messages in thread
From: Gustavo F. Padovan @ 2010-10-28  9:58 UTC (permalink / raw)
  To: Yuri Ershov
  Cc: marcel, davem, jprvita, linux-bluetooth, ville.tervo, andrei.emeltchenko

Hi Yuri,

Please no top posting in this mainling list. It's not allowed, thanks.

* Yuri Ershov <ext-yuri.ershov@nokia.com> [2010-10-25 16:23:47 +0400]:

> Hello Gustavo,
> 
> The problem appears in case of multiple connect-transfer-disconnect 
> sequence (e.g. by using l2test). The conditions are the following:
> There are 2 BT devices. The first one listens and receives (l2test -r), 
> the second one makes "connect-disconnect-connect..." sequence (l2test -c 
> -b 1000 -i hci0 -P 10 <addr>). After some time this will cause the race 
> between functions bt_accept_dequeue and l2cap_chan_del. The fail sequence:
> 
> struct sock *bt_accept_dequeue(struct sock *parent, struct socket *newsock)
> {
> ...
>     list_for_each_safe(p, n, &bt_sk(parent)->accept_q) {
>         sk = (struct sock *) list_entry(p, struct bt_sock, accept_q);
> 
>         lock_sock(sk);
> 
> 
> 
> In this time the function l2cap_chan_del sets the socket state to 
> BT_CLOSED, unlinks and kills the socket.
> 
> 
> 
>         /* FIXME: Is this check still needed */
>         if (sk->sk_state == BT_CLOSED) {
>             release_sock(sk);
>             bt_accept_unlink(sk);
>             continue;
>         }
> 
> ...
> 
>         release_sock(sk);
>     }
>     return NULL;
> }


I agree with you, just add this info to your commit message and then
resend your patch so I can apply it.

> ext Gustavo F. Padovan wrote:
> > Hi Yuri,
> >
> > * Yuri Ershov <ext-yuri.ershov@nokia.com> [2010-10-21 20:08:58 +0400]:
> >
> >   
> >> This patch fixes NULL pointer dereference at running test with
> >> connect-transfer-disconnect in loop. Sometimes sk_state is 
> >> BT_CLOSED and sk_refcnt equal to 0, so there is oops in 
> >> bt_accept_unlink. In normal case removed block is not used.
> >>     
> >
> > Question here is: Why sk_refcnt is 0 at that point of the code?  The
> > socket should be destroyed if it ref is 0, but it wasn't, so something
> > in another point of the code went is wrong. "Sometimes" is not a good
> > description of the problem, you have to show why that happened.
> >
> >   
> 

-- 
Gustavo F. Padovan
ProFUSION embedded systems - http://profusion.mobi

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] bluetooth: Fix NULL pointer dereference issue
  2010-10-22 13:58 ` Gustavo F. Padovan
@ 2010-10-25 12:23   ` Yuri Ershov
  2010-10-28  9:58     ` Gustavo F. Padovan
  0 siblings, 1 reply; 6+ messages in thread
From: Yuri Ershov @ 2010-10-25 12:23 UTC (permalink / raw)
  To: ext Gustavo F. Padovan
  Cc: marcel, davem, jprvita, linux-bluetooth, ville.tervo, andrei.emeltchenko

[-- Attachment #1: Type: text/plain, Size: 1732 bytes --]

Hello Gustavo,

The problem appears in case of multiple connect-transfer-disconnect 
sequence (e.g. by using l2test). The conditions are the following:
There are 2 BT devices. The first one listens and receives (l2test -r), 
the second one makes "connect-disconnect-connect..." sequence (l2test -c 
-b 1000 -i hci0 -P 10 <addr>). After some time this will cause the race 
between functions bt_accept_dequeue and l2cap_chan_del. The fail sequence:

struct sock *bt_accept_dequeue(struct sock *parent, struct socket *newsock)
{
...
    list_for_each_safe(p, n, &bt_sk(parent)->accept_q) {
        sk = (struct sock *) list_entry(p, struct bt_sock, accept_q);

        lock_sock(sk);



In this time the function l2cap_chan_del sets the socket state to 
BT_CLOSED, unlinks and kills the socket.



        /* FIXME: Is this check still needed */
        if (sk->sk_state == BT_CLOSED) {
            release_sock(sk);
            bt_accept_unlink(sk);
            continue;
        }

...

        release_sock(sk);
    }
    return NULL;
}
 

Regards,
Yuri



ext Gustavo F. Padovan wrote:
> Hi Yuri,
>
> * Yuri Ershov <ext-yuri.ershov@nokia.com> [2010-10-21 20:08:58 +0400]:
>
>   
>> This patch fixes NULL pointer dereference at running test with
>> connect-transfer-disconnect in loop. Sometimes sk_state is 
>> BT_CLOSED and sk_refcnt equal to 0, so there is oops in 
>> bt_accept_unlink. In normal case removed block is not used.
>>     
>
> Question here is: Why sk_refcnt is 0 at that point of the code?  The
> socket should be destroyed if it ref is 0, but it wasn't, so something
> in another point of the code went is wrong. "Sometimes" is not a good
> description of the problem, you have to show why that happened.
>
>   


[-- Attachment #2: Type: text/html, Size: 2746 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] bluetooth: Fix NULL pointer dereference issue
  2010-10-21 16:08 Yuri Ershov
@ 2010-10-22 13:58 ` Gustavo F. Padovan
  2010-10-25 12:23   ` Yuri Ershov
  0 siblings, 1 reply; 6+ messages in thread
From: Gustavo F. Padovan @ 2010-10-22 13:58 UTC (permalink / raw)
  To: Yuri Ershov
  Cc: marcel, davem, jprvita, linux-bluetooth, ville.tervo, andrei.emeltchenko

Hi Yuri,

* Yuri Ershov <ext-yuri.ershov@nokia.com> [2010-10-21 20:08:58 +0400]:

> This patch fixes NULL pointer dereference at running test with
> connect-transfer-disconnect in loop. Sometimes sk_state is 
> BT_CLOSED and sk_refcnt equal to 0, so there is oops in 
> bt_accept_unlink. In normal case removed block is not used.

Question here is: Why sk_refcnt is 0 at that point of the code?  The
socket should be destroyed if it ref is 0, but it wasn't, so something
in another point of the code went is wrong. "Sometimes" is not a good
description of the problem, you have to show why that happened.

-- 
Gustavo F. Padovan
ProFUSION embedded systems - http://profusion.mobi

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH] bluetooth: Fix NULL pointer dereference issue
@ 2010-10-21 16:08 Yuri Ershov
  2010-10-22 13:58 ` Gustavo F. Padovan
  0 siblings, 1 reply; 6+ messages in thread
From: Yuri Ershov @ 2010-10-21 16:08 UTC (permalink / raw)
  To: marcel, davem, padovan, jprvita
  Cc: linux-bluetooth, ville.tervo, andrei.emeltchenko, Yuri Ershov

This patch fixes NULL pointer dereference at running test with
connect-transfer-disconnect in loop. Sometimes sk_state is 
BT_CLOSED and sk_refcnt equal to 0, so there is oops in 
bt_accept_unlink. In normal case removed block is not used.

Signed-off-by: Yuri Ershov <ext-yuri.ershov@nokia.com>
---
 net/bluetooth/af_bluetooth.c |    7 -------
 1 files changed, 0 insertions(+), 7 deletions(-)

diff --git a/net/bluetooth/af_bluetooth.c b/net/bluetooth/af_bluetooth.c
index 421c45b..47c107e 100644
--- a/net/bluetooth/af_bluetooth.c
+++ b/net/bluetooth/af_bluetooth.c
@@ -204,13 +204,6 @@ struct sock *bt_accept_dequeue(struct sock *parent, struct socket *newsock)
 
 		lock_sock(sk);
 
-		/* FIXME: Is this check still needed */
-		if (sk->sk_state == BT_CLOSED) {
-			release_sock(sk);
-			bt_accept_unlink(sk);
-			continue;
-		}
-
 		if (sk->sk_state == BT_CONNECTED || !newsock ||
 						bt_sk(parent)->defer_setup) {
 			bt_accept_unlink(sk);
-- 
1.6.3.3

^ permalink raw reply related	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-10-29 21:43 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-10-28 10:52 [PATCH] bluetooth: Fix NULL pointer dereference issue Yuri Ershov
2010-10-29 21:43 ` Marcel Holtmann
  -- strict thread matches above, loose matches on Subject: below --
2010-10-21 16:08 Yuri Ershov
2010-10-22 13:58 ` Gustavo F. Padovan
2010-10-25 12:23   ` Yuri Ershov
2010-10-28  9:58     ` Gustavo F. Padovan

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.