Commit 6c08fc89 authored by Manish Mandlik's avatar Manish Mandlik Committed by Marcel Holtmann
Browse files

Bluetooth: Fix refcount use-after-free issue 



There is no lock preventing both l2cap_sock_release() and
chan->ops->close() from running at the same time.

If we consider Thread A running l2cap_chan_timeout() and Thread B running
l2cap_sock_release(), expected behavior is:
  A::l2cap_chan_timeout()->l2cap_chan_close()->l2cap_sock_teardown_cb()
  A::l2cap_chan_timeout()->l2cap_sock_close_cb()->l2cap_sock_kill()
  B::l2cap_sock_release()->sock_orphan()
  B::l2cap_sock_release()->l2cap_sock_kill()

where,
sock_orphan() clears "sk->sk_socket" and l2cap_sock_teardown_cb() marks
socket as SOCK_ZAPPED.

In l2cap_sock_kill(), there is an "if-statement" that checks if both
sock_orphan() and sock_teardown() has been run i.e. sk->sk_socket is NULL
and socket is marked as SOCK_ZAPPED. Socket is killed if the condition is
satisfied.

In the race condition, following occurs:
  A::l2cap_chan_timeout()->l2cap_chan_close()->l2cap_sock_teardown_cb()
  B::l2cap_sock_release()->sock_orphan()
  B::l2cap_sock_release()->l2cap_sock_kill()
  A::l2cap_chan_timeout()->l2cap_sock_close_cb()->l2cap_sock_kill()

In this scenario, "if-statement" is true in both B::l2cap_sock_kill() and
A::l2cap_sock_kill() and we hit "refcount: underflow; use-after-free" bug.

Similar condition occurs at other places where teardown/sock_kill is
happening:
  l2cap_disconnect_rsp()->l2cap_chan_del()->l2cap_sock_teardown_cb()
  l2cap_disconnect_rsp()->l2cap_sock_close_cb()->l2cap_sock_kill()

  l2cap_conn_del()->l2cap_chan_del()->l2cap_sock_teardown_cb()
  l2cap_conn_del()->l2cap_sock_close_cb()->l2cap_sock_kill()

  l2cap_disconnect_req()->l2cap_chan_del()->l2cap_sock_teardown_cb()
  l2cap_disconnect_req()->l2cap_sock_close_cb()->l2cap_sock_kill()

  l2cap_sock_cleanup_listen()->l2cap_chan_close()->l2cap_sock_teardown_cb()
  l2cap_sock_cleanup_listen()->l2cap_sock_kill()

Protect teardown/sock_kill and orphan/sock_kill by adding hold_lock on
l2cap channel to ensure that the socket is killed only after marked as
zapped and orphan.

Signed-off-by: default avatarManish Mandlik <mmandlik@google.com>
Signed-off-by: default avatarMarcel Holtmann <marcel@holtmann.org>
parent 151129df

net/bluetooth/l2cap_core.c

+15 −11
Original line number Diff line number Diff line
@@ -419,6 +419,9 @@ static void l2cap_chan_timeout(struct work_struct *work) 
	BT_DBG("chan %p state %s", chan, state_to_string(chan->state));



	mutex_lock(&conn->chan_lock);

	/* __set_chan_timer() calls l2cap_chan_hold(chan) while scheduling

	 * this work. No need to call l2cap_chan_hold(chan) here again.

	 */

	l2cap_chan_lock(chan);



	if (chan->state == BT_CONNECTED || chan->state == BT_CONFIG)
@@ -431,12 +434,12 @@ static void l2cap_chan_timeout(struct work_struct *work) 


	l2cap_chan_close(chan, reason);



	l2cap_chan_unlock(chan);



	chan->ops->close(chan);

	mutex_unlock(&conn->chan_lock);



	l2cap_chan_unlock(chan);

	l2cap_chan_put(chan);



	mutex_unlock(&conn->chan_lock);

}



struct l2cap_chan *l2cap_chan_create(void)
@@ -1737,9 +1740,9 @@ static void l2cap_conn_del(struct hci_conn *hcon, int err) 


		l2cap_chan_del(chan, err);



		l2cap_chan_unlock(chan);



		chan->ops->close(chan);



		l2cap_chan_unlock(chan);

		l2cap_chan_put(chan);

	}
@@ -4405,6 +4408,7 @@ static inline int l2cap_disconnect_req(struct l2cap_conn *conn, 
		return 0;

	}



	l2cap_chan_hold(chan);

	l2cap_chan_lock(chan);



	rsp.dcid = cpu_to_le16(chan->scid);
@@ -4413,12 +4417,11 @@ static inline int l2cap_disconnect_req(struct l2cap_conn *conn, 


	chan->ops->set_shutdown(chan);



	l2cap_chan_hold(chan);

	l2cap_chan_del(chan, ECONNRESET);



	l2cap_chan_unlock(chan);



	chan->ops->close(chan);



	l2cap_chan_unlock(chan);

	l2cap_chan_put(chan);



	mutex_unlock(&conn->chan_lock);
@@ -4450,20 +4453,21 @@ static inline int l2cap_disconnect_rsp(struct l2cap_conn *conn, 
		return 0;

	}



	l2cap_chan_hold(chan);

	l2cap_chan_lock(chan);



	if (chan->state != BT_DISCONN) {

		l2cap_chan_unlock(chan);

		l2cap_chan_put(chan);

		mutex_unlock(&conn->chan_lock);

		return 0;

	}



	l2cap_chan_hold(chan);

	l2cap_chan_del(chan, 0);



	l2cap_chan_unlock(chan);



	chan->ops->close(chan);



	l2cap_chan_unlock(chan);

	l2cap_chan_put(chan);



	mutex_unlock(&conn->chan_lock);

net/bluetooth/l2cap_sock.c

+13 −3
Original line number Diff line number Diff line
@@ -1042,7 +1042,7 @@ done: 
}



/* Kill socket (only if zapped and orphan)

 * Must be called on unlocked socket.

 * Must be called on unlocked socket, with l2cap channel lock.

 */

static void l2cap_sock_kill(struct sock *sk)

{
@@ -1203,8 +1203,15 @@ static int l2cap_sock_release(struct socket *sock) 


	err = l2cap_sock_shutdown(sock, 2);



	l2cap_chan_hold(l2cap_pi(sk)->chan);

	l2cap_chan_lock(l2cap_pi(sk)->chan);



	sock_orphan(sk);

	l2cap_sock_kill(sk);



	l2cap_chan_unlock(l2cap_pi(sk)->chan);

	l2cap_chan_put(l2cap_pi(sk)->chan);



	return err;

}
@@ -1222,12 +1229,15 @@ static void l2cap_sock_cleanup_listen(struct sock *parent) 
		BT_DBG("child chan %p state %s", chan,

		       state_to_string(chan->state));



		l2cap_chan_hold(chan);

		l2cap_chan_lock(chan);



		__clear_chan_timer(chan);

		l2cap_chan_close(chan, ECONNRESET);

		l2cap_chan_unlock(chan);



		l2cap_sock_kill(sk);



		l2cap_chan_unlock(chan);

		l2cap_chan_put(chan);

	}

}

CRA Git | Maintained and supported by SUSTech CRA and CCSE