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

[syzbot <syzbot+b106284c2a0b7bc80cf9@syzkaller.appspotmail.com>]

> 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
>