[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. 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+b10628...@syzkaller.appspotmail.com
---
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

[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+b10628...@syzkaller.appspotmail.com
> ---
>

> #syz test

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

[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. 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+b10628...@syzkaller.appspotmail.com
---

#syz test