Bluetooth: ISO: fix leaking sk after socket release

iso_sock_kill() tests !sock_flag(sk, SOCK_ZAPPED) || sk->sk_socket ||
sock_flag(sk, SOCK_DEAD) for early return, but this is always true since
sock_orphan(sk) sets SOCK_DEAD, so the sk reference released by socket
always leaks, iso_sock_destruct is never called.

The socket reference also leaks when __iso_sock_close() does not set
SOCK_ZAPPED, since iso_conn_del() does not call iso_sock_kill() after
zapping.

Fix by replacing SOCK_DEAD by BT_SK_KILLED flag that is not used for
something else, and lock_sock to ensure iso_sock_kill() puts sk only
after socket release only once. Release and iso_conn_del may run
concurrently. Call iso_sock_kill() from iso_conn_del() to clean sk up
after zapping.

Remove call to iso_sock_kill() from iso_sock_close(), as it's generally
no-op there.

Fixes: ccf74f2390d6 ("Bluetooth: Add BTPROTO_ISO socket type")
Signed-off-by: Pauli Virtanen <pav@iki.fi>
Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
index 80a58275891d5144337e26ce3321e1aa73711d62..5f0f45a573a7669019e8460b3728030633cf61dd 100644 (file)
--- a/net/bluetooth/iso.c
+++ b/net/bluetooth/iso.c
@@ -62,6 +62,7 @@ static void iso_sock_kill(struct sock *sk);
 enum {
        BT_SK_BIG_SYNC,
        BT_SK_PA_SYNC,
+       BT_SK_KILLED,
 };
 
 struct iso_pinfo {
@@ -295,6 +296,7 @@ static void iso_conn_del(struct hci_conn *hcon, int err)
        iso_sock_clear_timer(sk);
        iso_chan_del(sk, err);
        release_sock(sk);
+       iso_sock_kill(sk);
        sock_put(sk);
 }
 
@@ -798,24 +800,29 @@ static void iso_sock_cleanup_listen(struct sock *parent)
  */
 static void iso_sock_kill(struct sock *sk)
 {
+       lock_sock(sk);
+
        if (!sock_flag(sk, SOCK_ZAPPED) || sk->sk_socket ||
-           sock_flag(sk, SOCK_DEAD))
+           test_bit(BT_SK_KILLED, &iso_pi(sk)->flags)) {
+               release_sock(sk);
                return;
+       }
 
        BT_DBG("sk %p state %d", sk, sk->sk_state);
 
        /* Sock is dead, so set conn->sk to NULL to avoid possible UAF */
-       lock_sock(sk);
        if (iso_pi(sk)->conn) {
                iso_conn_lock(iso_pi(sk)->conn);
                iso_pi(sk)->conn->sk = NULL;
                iso_conn_unlock(iso_pi(sk)->conn);
        }
-       release_sock(sk);
 
        /* Kill poor orphan */
        bt_sock_unlink(&iso_sk_list, sk);
        sock_set_flag(sk, SOCK_DEAD);
+       set_bit(BT_SK_KILLED, &iso_pi(sk)->flags);
+
+       release_sock(sk);
        sock_put(sk);
 }
 
@@ -892,7 +899,6 @@ static void iso_sock_close(struct sock *sk)
        iso_sock_clear_timer(sk);
        __iso_sock_close(sk);
        release_sock(sk);
-       iso_sock_kill(sk);
 }
 
 static void iso_sock_init(struct sock *sk, struct sock *parent)
@@ -2040,8 +2046,16 @@ static int iso_sock_release(struct socket *sock)
                release_sock(sk);
        }
 
+       /* Make sure sk is valid even if iso_conn_del() is concurrent */
+       sock_hold(sk);
+
+       lock_sock(sk);
        sock_orphan(sk);
+       release_sock(sk);
+
        iso_sock_kill(sk);
+
+       sock_put(sk);
        return err;
 }