Commit 4e37f645 authored by Pauli Virtanen's avatar Pauli Virtanen Committed by Luiz Augusto von Dentz
Browse files

Bluetooth: SCO: hold sk properly in sco_conn_ready 



sk deref in sco_conn_ready must be done either under conn->lock, or
holding a refcount, to avoid concurrent close. conn->sk and parent sk is
currently accessed without either, and without checking parent->sk_state:

    [Task 1]            [Task 2]
                        sco_sock_release
    sco_conn_ready
      sk = conn->sk
                          lock_sock(sk)
                            conn->sk = NULL
      lock_sock(sk)
                          release_sock(sk)
                          sco_sock_kill(sk)
       UAF on sk deref

and similarly for access to sco_get_sock_listen() return value.

Fix possible UAF by holding sk refcount in sco_conn_ready() and making
sco_get_sock_listen() increase refcount. Also recheck after lock_sock
that the socket is still valid.  Adjust conn->sk locking so it's
protected also by lock_sock() of the associated socket if any.

Fixes: 27c24fda ("Bluetooth: switch to lock_sock in SCO")
Signed-off-by: default avatarPauli Virtanen <pav@iki.fi>
Signed-off-by: default avatarLuiz Augusto von Dentz <luiz.von.dentz@intel.com>
parent 0a120d96

net/bluetooth/sco.c

+32 −12
Original line number Diff line number Diff line
@@ -472,9 +472,13 @@ static struct sock *sco_get_sock_listen(bdaddr_t *src) 
			sk1 = sk;

	}



	sk = sk ? sk : sk1;

	if (sk)

		sock_hold(sk);



	read_unlock(&sco_sk_list.lock);



	return sk ? sk : sk1;

	return sk;

}



static void sco_sock_destruct(struct sock *sk)
@@ -515,11 +519,13 @@ static void sco_sock_kill(struct sock *sk) 
	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 (sco_pi(sk)->conn) {

		sco_conn_lock(sco_pi(sk)->conn);

		sco_pi(sk)->conn->sk = NULL;

		sco_conn_unlock(sco_pi(sk)->conn);

	}

	release_sock(sk);



	/* Kill poor orphan */

	bt_sock_unlink(&sco_sk_list, sk);
@@ -1365,17 +1371,28 @@ static int sco_sock_release(struct socket *sock) 


static void sco_conn_ready(struct sco_conn *conn)

{

	struct sock *parent;

	struct sock *sk = conn->sk;

	struct sock *parent, *sk;



	sco_conn_lock(conn);

	sk = sco_sock_hold(conn);

	sco_conn_unlock(conn);



	BT_DBG("conn %p", conn);



	if (sk) {

		lock_sock(sk);



		/* conn->sk may have become NULL if racing with sk close, but

		 * due to held hdev->lock, it can't become different sk.

		 */

		if (conn->sk) {

			sco_sock_clear_timer(sk);

			sk->sk_state = BT_CONNECTED;

			sk->sk_state_change(sk);

		}



		release_sock(sk);

		sock_put(sk);

	} else {

		if (!conn->hcon)

			return;
@@ -1390,13 +1407,15 @@ static void sco_conn_ready(struct sco_conn *conn) 


		sco_conn_lock(conn);



		/* hdev->lock guarantees conn->sk == NULL still here */



		if (parent->sk_state != BT_LISTEN)

			goto release;



		sk = sco_sock_alloc(sock_net(parent), NULL,

				    BTPROTO_SCO, GFP_ATOMIC, 0);

		if (!sk) {

			sco_conn_unlock(conn);

			release_sock(parent);

			return;

		}

		if (!sk)

			goto release;



		sco_sock_init(sk, parent);
@@ -1415,9 +1434,10 @@ static void sco_conn_ready(struct sco_conn *conn) 
		/* Wake up parent */

		parent->sk_data_ready(parent);



release:

		sco_conn_unlock(conn);



		release_sock(parent);

		sock_put(parent);

	}

}