Re: [PATCH] Bluetooth: fix use-after-free error in lock_sock_nested()

quoted

quoted

Hi,

In my case it looks OK, this is the diff:

diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index f1b1edd0b697..32ef3328ab49 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c

@@ -1500,6 +1500,9 @@ static void l2cap_sock_close_cb(struct

l2cap_chan *chan)
  {
         struct sock *sk = chan->data;

+       if (!sk)
+               return;
+
         l2cap_sock_kill(sk);
  }

@@ -1508,6 +1511,9 @@ static void l2cap_sock_teardown_cb(struct

l2cap_chan *chan, int err)
         struct sock *sk = chan->data;
         struct sock *parent;

+       if (!sk)
+               return;
+
         BT_DBG("chan %p state %s", chan, state_to_string(chan->state));

         /* This callback can be called both for server (BT_LISTEN)

@@ -1700,6 +1706,7 @@ static void l2cap_sock_destruct(struct sock *sk)
         BT_DBG("sk %p", sk);

         if (l2cap_pi(sk)->chan)
+              l2cap_pi(sk)->chan->data = NULL;
                  l2cap_chan_put(l2cap_pi(sk)->chan);

But if it has potential risk if l2cap_sock_destruct() can not be
excuted in time ?

sk_free():

         if (refcount_dec_and_test(&sk->sk_wmem_alloc)) //is possible
this condition false ?

               __sk_free(sk)   -> ... l2cap_sock_destruct()

Dear Luiz,

Not only that, if l2cap_sock_kill() has put 'l2cap_pi(sk)->chan', how
does we avoid re-puting 'l2cap_pi(sk)->chan' if l2cap_sock_destruct()
work postponed? this will cause underflow of chan->refcount; this PATCH
4e1a720d0312 ("Bluetooth: avoid killing an already killed socket") also
may not work in any case because only sock_orphan() has excuted can this
sock be killed, but if sco_sock_release() excute first, for this sock
has been marked as SOCK_DEAD, this sock can never be killed. So should
we think put chan->data = NULL in xx_sock_kill() is a better choice ?

Not sure what do you mean by postponed? Interrupted perhaps? Even in
that case what are trying to prevent is use after free so if the
callback has not run yet that means the sk has not been freed. Anyway
I think we could do it inconditionally in l2cap_sock_kill since we
will be releasing the reference owned by l2cap_pi(sk)->chan->data that
should be reset to NULL immediatelly.

Dear  Luiz,

yes, that's right, if sk can be accessed, it also means that chan has 
not been destroyed, thanks very much.

-- Wang ShaoBo