Re: [PATCH] Bluetooth: L2CAP: access chan->conn safely in get/setsockopt

Re: [PATCH] Bluetooth: L2CAP: access chan->conn safely in get/setsockopt

[syzbot]

> Since commit b66774b48dd9 ("Bluetooth: L2CAP: Fix UAF in channel timeout by holding conn ref")
> l2cap_chan::conn has held reference and remains non-NULL also after the
> corresponding hci_conn is deleted.  In this state accessing various
> fields eg. hci_conn::hdev is invalid, which leads to KASAN crash in
> l2cap_sock_setsockopt() access of conn->hcon->hdev.
>
> Check l2cap_chan::conn.hcon corresponds to an alive hci_conn before
> trying to use it in l2cap_sock.c.  This can be guaranteed by
> synchronizing with l2cap_sock_teardown_cb().  Make l2cap_chan::conn
> readable without l2cap_chan::lock, so we can access with only sk lock.
> Also move sock_set_flag() inside lock_sock() since it is not atomic.
>
> Fixes: b66774b48dd9 ("Bluetooth: L2CAP: Fix UAF in channel timeout by holding conn ref")
> Reported-by: syzbot+b106284c2a0b7bc80cf9@syzkaller.appspotmail.com
> ---
>
> #syz test

This crash does not have a reproducer. I cannot test it.

>
>  net/bluetooth/l2cap_core.c |  2 +-
>  net/bluetooth/l2cap_sock.c | 64 ++++++++++++++++++++++++++++----------
>  2 files changed, 48 insertions(+), 18 deletions(-)
>
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index ee459dd411f5..7327ebb1e710 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -624,7 +624,7 @@ void __l2cap_chan_add(struct l2cap_conn *conn, struct l2cap_chan *chan)
>  
>  	conn->disc_reason = HCI_ERROR_REMOTE_USER_TERM;
>  
> -	chan->conn = l2cap_conn_get(conn);
> +	WRITE_ONCE(chan->conn, l2cap_conn_get(conn));
>  
>  	switch (chan->chan_type) {
>  	case L2CAP_CHAN_CONN_ORIENTED:
> diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
> index 735167f73f31..c3d9f7721963 100644
> --- a/net/bluetooth/l2cap_sock.c
> +++ b/net/bluetooth/l2cap_sock.c
> @@ -436,11 +436,33 @@ static int l2cap_get_mode(struct l2cap_chan *chan)
>  	return -EINVAL;
>  }
>  
> +static struct l2cap_conn *l2cap_sock_conn(struct sock *sk)
> +{
> +	struct l2cap_chan *chan = l2cap_pi(sk)->chan;
> +	struct l2cap_conn *conn = READ_ONCE(chan->conn);
> +
> +	lockdep_assert(lockdep_sock_is_held(sk));
> +
> +	/* chan holds refcount on conn during its lifetime, so if non-NULL
> +	 * observed, it is valid.
> +	 *
> +	 * conn holds refcount on conn->hcon, but the hci_conn may be in deleted
> +	 * state.  l2cap_sock_teardown_cb() is called before associated hci_conn
> +	 * is deleted, so it is alive if sk is not zapped, as long as sk lock is
> +	 * held.
> +	 */
> +	if (sock_flag(sk, SOCK_ZAPPED))
> +		return NULL;
> +
> +	return conn;
> +}
> +
>  static int l2cap_sock_getsockopt_old(struct socket *sock, int optname,
>  				     sockopt_t *sopt)
>  {
>  	struct sock *sk = sock->sk;
>  	struct l2cap_chan *chan = l2cap_pi(sk)->chan;
> +	struct l2cap_conn *conn;
>  	struct l2cap_options opts;
>  	struct l2cap_conninfo cinfo;
>  	int err = 0;
> @@ -537,9 +559,15 @@ static int l2cap_sock_getsockopt_old(struct socket *sock, int optname,
>  			break;
>  		}
>  
> +		conn = l2cap_sock_conn(sk);
> +		if (!conn) {
> +			err = -ENOTCONN;
> +			break;
> +		}
> +
>  		memset(&cinfo, 0, sizeof(cinfo));
> -		cinfo.hci_handle = chan->conn->hcon->handle;
> -		memcpy(cinfo.dev_class, chan->conn->hcon->dev_class, 3);
> +		cinfo.hci_handle = conn->hcon->handle;
> +		memcpy(cinfo.dev_class, conn->hcon->dev_class, 3);
>  
>  		len = min(len, sizeof(cinfo));
>  		if (copy_to_iter(&cinfo, len, &sopt->iter_out) != len)
> @@ -561,6 +589,7 @@ static int l2cap_sock_getsockopt(struct socket *sock, int level, int optname,
>  {
>  	struct sock *sk = sock->sk;
>  	struct l2cap_chan *chan = l2cap_pi(sk)->chan;
> +	struct l2cap_conn *conn;
>  	struct bt_security sec;
>  	struct bt_power pwr;
>  	int len, mode, err = 0;
> @@ -589,12 +618,14 @@ static int l2cap_sock_getsockopt(struct socket *sock, int level, int optname,
>  			break;
>  		}
>  
> +		conn = l2cap_sock_conn(sk);
> +
>  		memset(&sec, 0, sizeof(sec));
> -		if (chan->conn) {
> -			sec.level = chan->conn->hcon->sec_level;
> +		if (conn) {
> +			sec.level = conn->hcon->sec_level;
>  
>  			if (sk->sk_state == BT_CONNECTED)
> -				sec.key_size = chan->conn->hcon->enc_key_size;
> +				sec.key_size = conn->hcon->enc_key_size;
>  		} else {
>  			sec.level = chan->sec_level;
>  		}
> @@ -678,12 +709,14 @@ static int l2cap_sock_getsockopt(struct socket *sock, int level, int optname,
>  		break;
>  
>  	case BT_PHY:
> -		if (sk->sk_state != BT_CONNECTED) {
> +		conn = l2cap_sock_conn(sk);
> +
> +		if (sk->sk_state != BT_CONNECTED || !conn) {
>  			err = -ENOTCONN;
>  			break;
>  		}
>  
> -		opt = hci_conn_get_phy(chan->conn->hcon);
> +		opt = hci_conn_get_phy(conn->hcon);
>  
>  		if (copy_to_iter(&opt, sizeof(opt), &sopt->iter_out) !=
>  		    sizeof(opt))
> @@ -938,11 +971,10 @@ static int l2cap_sock_setsockopt(struct socket *sock, int level, int optname,
>  
>  		chan->sec_level = sec.level;
>  
> -		if (!chan->conn)
> +		conn = l2cap_sock_conn(sk);
> +		if (!conn)
>  			break;
>  
> -		conn = chan->conn;
> -
>  		/* change security for LE channels */
>  		if (chan->scid == L2CAP_CID_ATT) {
>  			if (smp_conn_security(conn->hcon, sec.level)) {
> @@ -997,7 +1029,8 @@ static int l2cap_sock_setsockopt(struct socket *sock, int level, int optname,
>  		}
>  
>  		if (opt == BT_FLUSHABLE_OFF) {
> -			conn = chan->conn;
> +			conn = l2cap_sock_conn(sk);
> +
>  			/* proceed further only when we have l2cap_conn and
>  			   No Flush support in the LM */
>  			if (!conn || !lmp_no_flush_capable(conn->hcon->hdev)) {
> @@ -1083,7 +1116,8 @@ static int l2cap_sock_setsockopt(struct socket *sock, int level, int optname,
>  		break;
>  
>  	case BT_PHY:
> -		if (sk->sk_state != BT_CONNECTED) {
> +		conn = l2cap_sock_conn(sk);
> +		if (sk->sk_state != BT_CONNECTED || !conn) {
>  			err = -ENOTCONN;
>  			break;
>  		}
> @@ -1093,10 +1127,6 @@ static int l2cap_sock_setsockopt(struct socket *sock, int level, int optname,
>  		if (err)
>  			break;
>  
> -		if (!chan->conn)
> -			break;
> -
> -		conn = chan->conn;
>  		err = hci_conn_set_phy(conn->hcon, phys);
>  		break;
>  
> @@ -1716,11 +1746,11 @@ static void l2cap_sock_teardown_cb(struct l2cap_chan *chan, int err)
>  
>  		break;
>  	}
> -	release_sock(sk);
>  
>  	/* Only zap after cleanup to avoid use after free race */
>  	sock_set_flag(sk, SOCK_ZAPPED);
>  
> +	release_sock(sk);
>  }
>  
>  static void l2cap_sock_state_change_cb(struct l2cap_chan *chan, int state,
> -- 
> 2.55.0
>

[PATCH] Bluetooth: L2CAP: access chan->conn safely in get/setsockopt

[Pauli Virtanen]

Since commit b66774b48dd9 ("Bluetooth: L2CAP: Fix UAF in channel timeout by holding conn ref")
l2cap_chan::conn has held reference and remains non-NULL also after the
corresponding hci_conn is deleted.  In this state accessing various
fields eg. hci_conn::hdev is invalid, which leads to KASAN crash in
l2cap_sock_setsockopt() access of conn->hcon->hdev.

Check l2cap_chan::conn.hcon corresponds to an alive hci_conn before
trying to use it in l2cap_sock.c.  Hold l2cap_chan_lock() in
getsockopt/setsockopt to ensure it stays alive, and to avoid data races
in l2cap_chan fields.

Fixes: b66774b48dd9 ("Bluetooth: L2CAP: Fix UAF in channel timeout by holding conn ref")
Reported-by: syzbot+b106284c2a0b7bc80cf9@syzkaller.appspotmail.com
Link: https://syzkaller.appspot.com/bug?extid=b106284c2a0b7bc80cf9
Signed-off-by: Pauli Virtanen <pav@iki.fi>
---
 net/bluetooth/l2cap_sock.c | 64 ++++++++++++++++++++++++++++----------
 1 file changed, 48 insertions(+), 16 deletions(-)

diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index 735167f73f31..cca6201f9cdb 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -436,11 +436,26 @@ static int l2cap_get_mode(struct l2cap_chan *chan)
 	return -EINVAL;
 }
 
+static struct l2cap_conn *l2cap_chan_conn(struct l2cap_chan *chan)
+{
+	lockdep_assert_held(&chan->lock);
+
+	/* l2cap_conn_del() sets FLAG_DEL while holding chan->lock before
+	 * conn->hcon is deleted. If not set and conn is non-NULL, conn->hcon
+	 * remains alive during this chan->lock critical section.
+	 */
+	if (test_bit(FLAG_DEL, &chan->flags))
+		return NULL;
+
+	return chan->conn;
+}
+
 static int l2cap_sock_getsockopt_old(struct socket *sock, int optname,
 				     sockopt_t *sopt)
 {
 	struct sock *sk = sock->sk;
 	struct l2cap_chan *chan = l2cap_pi(sk)->chan;
+	struct l2cap_conn *conn;
 	struct l2cap_options opts;
 	struct l2cap_conninfo cinfo;
 	int err = 0;
@@ -451,6 +466,7 @@ static int l2cap_sock_getsockopt_old(struct socket *sock, int optname,
 
 	len = sopt->optlen;
 
+	l2cap_chan_lock(chan);
 	lock_sock(sk);
 
 	switch (optname) {
@@ -537,9 +553,15 @@ static int l2cap_sock_getsockopt_old(struct socket *sock, int optname,
 			break;
 		}
 
+		conn = l2cap_chan_conn(chan);
+		if (!conn) {
+			err = -ENOTCONN;
+			break;
+		}
+
 		memset(&cinfo, 0, sizeof(cinfo));
-		cinfo.hci_handle = chan->conn->hcon->handle;
-		memcpy(cinfo.dev_class, chan->conn->hcon->dev_class, 3);
+		cinfo.hci_handle = conn->hcon->handle;
+		memcpy(cinfo.dev_class, conn->hcon->dev_class, 3);
 
 		len = min(len, sizeof(cinfo));
 		if (copy_to_iter(&cinfo, len, &sopt->iter_out) != len)
@@ -553,6 +575,8 @@ static int l2cap_sock_getsockopt_old(struct socket *sock, int optname,
 	}
 
 	release_sock(sk);
+	l2cap_chan_unlock(chan);
+
 	return err;
 }
 
@@ -561,6 +585,7 @@ static int l2cap_sock_getsockopt(struct socket *sock, int level, int optname,
 {
 	struct sock *sk = sock->sk;
 	struct l2cap_chan *chan = l2cap_pi(sk)->chan;
+	struct l2cap_conn *conn;
 	struct bt_security sec;
 	struct bt_power pwr;
 	int len, mode, err = 0;
@@ -578,6 +603,7 @@ static int l2cap_sock_getsockopt(struct socket *sock, int level, int optname,
 
 	len = sopt->optlen;
 
+	l2cap_chan_lock(chan);
 	lock_sock(sk);
 
 	switch (optname) {
@@ -589,12 +615,14 @@ static int l2cap_sock_getsockopt(struct socket *sock, int level, int optname,
 			break;
 		}
 
+		conn = l2cap_chan_conn(chan);
+
 		memset(&sec, 0, sizeof(sec));
-		if (chan->conn) {
-			sec.level = chan->conn->hcon->sec_level;
+		if (conn) {
+			sec.level = conn->hcon->sec_level;
 
 			if (sk->sk_state == BT_CONNECTED)
-				sec.key_size = chan->conn->hcon->enc_key_size;
+				sec.key_size = conn->hcon->enc_key_size;
 		} else {
 			sec.level = chan->sec_level;
 		}
@@ -678,12 +706,14 @@ static int l2cap_sock_getsockopt(struct socket *sock, int level, int optname,
 		break;
 
 	case BT_PHY:
-		if (sk->sk_state != BT_CONNECTED) {
+		conn = l2cap_chan_conn(chan);
+
+		if (sk->sk_state != BT_CONNECTED || !conn) {
 			err = -ENOTCONN;
 			break;
 		}
 
-		opt = hci_conn_get_phy(chan->conn->hcon);
+		opt = hci_conn_get_phy(conn->hcon);
 
 		if (copy_to_iter(&opt, sizeof(opt), &sopt->iter_out) !=
 		    sizeof(opt))
@@ -719,6 +749,7 @@ static int l2cap_sock_getsockopt(struct socket *sock, int level, int optname,
 	}
 
 	release_sock(sk);
+	l2cap_chan_unlock(chan);
 	return err;
 }
 
@@ -749,6 +780,7 @@ static int l2cap_sock_setsockopt_old(struct socket *sock, int optname,
 
 	BT_DBG("sk %p", sk);
 
+	l2cap_chan_lock(chan);
 	lock_sock(sk);
 
 	switch (optname) {
@@ -850,6 +882,7 @@ static int l2cap_sock_setsockopt_old(struct socket *sock, int optname,
 	}
 
 	release_sock(sk);
+	l2cap_chan_unlock(chan);
 	return err;
 }
 
@@ -913,6 +946,7 @@ static int l2cap_sock_setsockopt(struct socket *sock, int level, int optname,
 	if (level != SOL_BLUETOOTH)
 		return -ENOPROTOOPT;
 
+	l2cap_chan_lock(chan);
 	lock_sock(sk);
 
 	switch (optname) {
@@ -938,11 +972,10 @@ static int l2cap_sock_setsockopt(struct socket *sock, int level, int optname,
 
 		chan->sec_level = sec.level;
 
-		if (!chan->conn)
+		conn = l2cap_chan_conn(chan);
+		if (!conn)
 			break;
 
-		conn = chan->conn;
-
 		/* change security for LE channels */
 		if (chan->scid == L2CAP_CID_ATT) {
 			if (smp_conn_security(conn->hcon, sec.level)) {
@@ -997,7 +1030,8 @@ static int l2cap_sock_setsockopt(struct socket *sock, int level, int optname,
 		}
 
 		if (opt == BT_FLUSHABLE_OFF) {
-			conn = chan->conn;
+			conn = l2cap_chan_conn(chan);
+
 			/* proceed further only when we have l2cap_conn and
 			   No Flush support in the LM */
 			if (!conn || !lmp_no_flush_capable(conn->hcon->hdev)) {
@@ -1083,7 +1117,8 @@ static int l2cap_sock_setsockopt(struct socket *sock, int level, int optname,
 		break;
 
 	case BT_PHY:
-		if (sk->sk_state != BT_CONNECTED) {
+		conn = l2cap_chan_conn(chan);
+		if (sk->sk_state != BT_CONNECTED || !conn) {
 			err = -ENOTCONN;
 			break;
 		}
@@ -1093,10 +1128,6 @@ static int l2cap_sock_setsockopt(struct socket *sock, int level, int optname,
 		if (err)
 			break;
 
-		if (!chan->conn)
-			break;
-
-		conn = chan->conn;
 		err = hci_conn_set_phy(conn->hcon, phys);
 		break;
 
@@ -1139,6 +1170,7 @@ static int l2cap_sock_setsockopt(struct socket *sock, int level, int optname,
 	}
 
 	release_sock(sk);
+	l2cap_chan_unlock(chan);
 	return err;
 }
 
-- 
2.55.0