Commit 79a2d467 authored by Pauli Virtanen's avatar Pauli Virtanen Committed by Luiz Augusto von Dentz
Browse files

Bluetooth: hci_core: lookup hci_conn on RX path on protocol side 



The hdev lock/lookup/unlock/use pattern in the packet RX path doesn't
ensure hci_conn* is not concurrently modified/deleted. This locking
appears to be leftover from before conn_hash started using RCU
commit bf4c6325 ("Bluetooth: convert conn hash to RCU")
and not clear if it had purpose since then.

Currently, there are code paths that delete hci_conn* from elsewhere
than the ordered hdev->workqueue where the RX work runs in. E.g.
commit 5af1f84e ("Bluetooth: hci_sync: Fix UAF on hci_abort_conn_sync")
introduced some of these, and there probably were a few others before
it.  It's better to do the locking so that even if these run
concurrently no UAF is possible.

Move the lookup of hci_conn and associated socket-specific conn to
protocol recv handlers, and do them within a single critical section
to cover hci_conn* usage and lookup.

syzkaller has reported a crash that appears to be this issue:

    [Task hdev->workqueue]          [Task 2]
                                    hci_disconnect_all_sync
    l2cap_recv_acldata(hcon)
                                      hci_conn_get(hcon)
                                      hci_abort_conn_sync(hcon)
                                        hci_dev_lock
      hci_dev_lock
                                        hci_conn_del(hcon)
      v-------------------------------- hci_dev_unlock
                                      hci_conn_put(hcon)
      conn = hcon->l2cap_data (UAF)

Fixes: 5af1f84e ("Bluetooth: hci_sync: Fix UAF on hci_abort_conn_sync")
Reported-by: default avatar <syzbot+d32d77220b92eddd89ad@syzkaller.appspotmail.com>
Closes: https://syzkaller.appspot.com/bug?extid=d32d77220b92eddd89ad


Signed-off-by: default avatarPauli Virtanen <pav@iki.fi>
Signed-off-by: default avatarLuiz Augusto von Dentz <luiz.von.dentz@intel.com>
parent 89bb6135

include/net/bluetooth/hci_core.h

+14 −6
Original line number Diff line number Diff line
@@ -856,11 +856,12 @@ extern struct mutex hci_cb_list_lock; 
/* ----- HCI interface to upper protocols ----- */

int l2cap_connect_ind(struct hci_dev *hdev, bdaddr_t *bdaddr);

int l2cap_disconn_ind(struct hci_conn *hcon);

void l2cap_recv_acldata(struct hci_conn *hcon, struct sk_buff *skb, u16 flags);

int l2cap_recv_acldata(struct hci_dev *hdev, u16 handle, struct sk_buff *skb,

		       u16 flags);



#if IS_ENABLED(CONFIG_BT_BREDR)

int sco_connect_ind(struct hci_dev *hdev, bdaddr_t *bdaddr, __u8 *flags);

void sco_recv_scodata(struct hci_conn *hcon, struct sk_buff *skb);

int sco_recv_scodata(struct hci_dev *hdev, u16 handle, struct sk_buff *skb);

#else

static inline int sco_connect_ind(struct hci_dev *hdev, bdaddr_t *bdaddr,

				  __u8 *flags)
@@ -868,23 +869,30 @@ static inline int sco_connect_ind(struct hci_dev *hdev, bdaddr_t *bdaddr, 
	return 0;

}



static inline void sco_recv_scodata(struct hci_conn *hcon, struct sk_buff *skb)

static inline int sco_recv_scodata(struct hci_dev *hdev, u16 handle,

				   struct sk_buff *skb)

{

	kfree_skb(skb);

	return -ENOENT;

}

#endif



#if IS_ENABLED(CONFIG_BT_LE)

int iso_connect_ind(struct hci_dev *hdev, bdaddr_t *bdaddr, __u8 *flags);

void iso_recv(struct hci_conn *hcon, struct sk_buff *skb, u16 flags);

int iso_recv(struct hci_dev *hdev, u16 handle, struct sk_buff *skb,

	     u16 flags);

#else

static inline int iso_connect_ind(struct hci_dev *hdev, bdaddr_t *bdaddr,

				  __u8 *flags)

{

	return 0;

}

static inline void iso_recv(struct hci_conn *hcon, struct sk_buff *skb,

			    u16 flags)



static inline int iso_recv(struct hci_dev *hdev, u16 handle,

			   struct sk_buff *skb, u16 flags)

{

	kfree_skb(skb);

	return -ENOENT;

}

#endif

net/bluetooth/hci_core.c

+25 −48
Original line number Diff line number Diff line
@@ -3832,13 +3832,14 @@ static void hci_tx_work(struct work_struct *work) 
static void hci_acldata_packet(struct hci_dev *hdev, struct sk_buff *skb)

{

	struct hci_acl_hdr *hdr;

	struct hci_conn *conn;

	__u16 handle, flags;

	int err;



	hdr = skb_pull_data(skb, sizeof(*hdr));

	if (!hdr) {

		bt_dev_err(hdev, "ACL packet too small");

		goto drop;

		kfree_skb(skb);

		return;

	}



	handle = __le16_to_cpu(hdr->handle);
@@ -3850,36 +3851,27 @@ static void hci_acldata_packet(struct hci_dev *hdev, struct sk_buff *skb) 


	hdev->stat.acl_rx++;



	hci_dev_lock(hdev);

	conn = hci_conn_hash_lookup_handle(hdev, handle);

	hci_dev_unlock(hdev);



	if (conn) {

		hci_conn_enter_active_mode(conn, BT_POWER_FORCE_ACTIVE_OFF);



		/* Send to upper protocol */

		l2cap_recv_acldata(conn, skb, flags);

		return;

	} else {

	err = l2cap_recv_acldata(hdev, handle, skb, flags);

	if (err == -ENOENT)

		bt_dev_err(hdev, "ACL packet for unknown connection handle %d",

			   handle);

	}



drop:

	kfree_skb(skb);

	else if (err)

		bt_dev_dbg(hdev, "ACL packet recv for handle %d failed: %d",

			   handle, err);

}



/* SCO data packet */

static void hci_scodata_packet(struct hci_dev *hdev, struct sk_buff *skb)

{

	struct hci_sco_hdr *hdr;

	struct hci_conn *conn;

	__u16 handle, flags;

	int err;



	hdr = skb_pull_data(skb, sizeof(*hdr));

	if (!hdr) {

		bt_dev_err(hdev, "SCO packet too small");

		goto drop;

		kfree_skb(skb);

		return;

	}



	handle = __le16_to_cpu(hdr->handle);
@@ -3891,34 +3883,28 @@ static void hci_scodata_packet(struct hci_dev *hdev, struct sk_buff *skb) 


	hdev->stat.sco_rx++;



	hci_dev_lock(hdev);

	conn = hci_conn_hash_lookup_handle(hdev, handle);

	hci_dev_unlock(hdev);



	if (conn) {

		/* Send to upper protocol */

	hci_skb_pkt_status(skb) = flags & 0x03;

		sco_recv_scodata(conn, skb);

		return;

	} else {



	err = sco_recv_scodata(hdev, handle, skb);

	if (err == -ENOENT)

		bt_dev_err_ratelimited(hdev, "SCO packet for unknown connection handle %d",

				       handle);

	}



drop:

	kfree_skb(skb);

	else if (err)

		bt_dev_dbg(hdev, "SCO packet recv for handle %d failed: %d",

			   handle, err);

}



static void hci_isodata_packet(struct hci_dev *hdev, struct sk_buff *skb)

{

	struct hci_iso_hdr *hdr;

	struct hci_conn *conn;

	__u16 handle, flags;

	int err;



	hdr = skb_pull_data(skb, sizeof(*hdr));

	if (!hdr) {

		bt_dev_err(hdev, "ISO packet too small");

		goto drop;

		kfree_skb(skb);

		return;

	}



	handle = __le16_to_cpu(hdr->handle);
@@ -3928,22 +3914,13 @@ static void hci_isodata_packet(struct hci_dev *hdev, struct sk_buff *skb) 
	bt_dev_dbg(hdev, "len %d handle 0x%4.4x flags 0x%4.4x", skb->len,

		   handle, flags);



	hci_dev_lock(hdev);

	conn = hci_conn_hash_lookup_handle(hdev, handle);

	hci_dev_unlock(hdev);



	if (!conn) {

	err = iso_recv(hdev, handle, skb, flags);

	if (err == -ENOENT)

		bt_dev_err(hdev, "ISO packet for unknown connection handle %d",

			   handle);

		goto drop;

	}



	/* Send to upper protocol */

	iso_recv(conn, skb, flags);

	return;



drop:

	kfree_skb(skb);

	else if (err)

		bt_dev_dbg(hdev, "ISO packet recv for handle %d failed: %d",

			   handle, err);

}



static bool hci_req_is_complete(struct hci_dev *hdev)

net/bluetooth/iso.c

+25 −5
Original line number Diff line number Diff line
@@ -2314,14 +2314,31 @@ static void iso_disconn_cfm(struct hci_conn *hcon, __u8 reason) 
	iso_conn_del(hcon, bt_to_errno(reason));

}



void iso_recv(struct hci_conn *hcon, struct sk_buff *skb, u16 flags)

int iso_recv(struct hci_dev *hdev, u16 handle, struct sk_buff *skb, u16 flags)

{

	struct iso_conn *conn = hcon->iso_data;

	struct hci_conn *hcon;

	struct iso_conn *conn;

	struct skb_shared_hwtstamps *hwts;

	__u16 pb, ts, len, sn;



	if (!conn)

		goto drop;

	hci_dev_lock(hdev);



	hcon = hci_conn_hash_lookup_handle(hdev, handle);

	if (!hcon) {

		hci_dev_unlock(hdev);

		kfree_skb(skb);

		return -ENOENT;

	}



	conn = iso_conn_hold_unless_zero(hcon->iso_data);

	hcon = NULL;



	hci_dev_unlock(hdev);



	if (!conn) {

		kfree_skb(skb);

		return -EINVAL;

	}



	pb     = hci_iso_flags_pb(flags);

	ts     = hci_iso_flags_ts(flags);
@@ -2377,7 +2394,7 @@ void iso_recv(struct hci_conn *hcon, struct sk_buff *skb, u16 flags) 
			hci_skb_pkt_status(skb) = flags & 0x03;

			hci_skb_pkt_seqnum(skb) = sn;

			iso_recv_frame(conn, skb);

			return;

			goto done;

		}



		if (pb == ISO_SINGLE) {
@@ -2455,6 +2472,9 @@ void iso_recv(struct hci_conn *hcon, struct sk_buff *skb, u16 flags) 


drop:

	kfree_skb(skb);

done:

	iso_conn_put(conn);

	return 0;

}



static struct hci_cb iso_cb = {

net/bluetooth/l2cap_core.c

+18 −5
Original line number Diff line number Diff line
@@ -7510,13 +7510,24 @@ struct l2cap_conn *l2cap_conn_hold_unless_zero(struct l2cap_conn *c) 
	return c;

}



void l2cap_recv_acldata(struct hci_conn *hcon, struct sk_buff *skb, u16 flags)

int l2cap_recv_acldata(struct hci_dev *hdev, u16 handle,

		       struct sk_buff *skb, u16 flags)

{

	struct hci_conn *hcon;

	struct l2cap_conn *conn;

	int len;



	/* Lock hdev to access l2cap_data to avoid race with l2cap_conn_del */

	hci_dev_lock(hcon->hdev);

	/* Lock hdev for hci_conn, and race on l2cap_data vs. l2cap_conn_del */

	hci_dev_lock(hdev);



	hcon = hci_conn_hash_lookup_handle(hdev, handle);

	if (!hcon) {

		hci_dev_unlock(hdev);

		kfree_skb(skb);

		return -ENOENT;

	}



	hci_conn_enter_active_mode(hcon, BT_POWER_FORCE_ACTIVE_OFF);



	conn = hcon->l2cap_data;
@@ -7524,12 +7535,13 @@ void l2cap_recv_acldata(struct hci_conn *hcon, struct sk_buff *skb, u16 flags) 
		conn = l2cap_conn_add(hcon);



	conn = l2cap_conn_hold_unless_zero(conn);

	hcon = NULL;



	hci_dev_unlock(hcon->hdev);

	hci_dev_unlock(hdev);



	if (!conn) {

		kfree_skb(skb);

		return;

		return -EINVAL;

	}



	BT_DBG("conn %p len %u flags 0x%x", conn, skb->len, flags);
@@ -7643,6 +7655,7 @@ void l2cap_recv_acldata(struct hci_conn *hcon, struct sk_buff *skb, u16 flags) 
unlock:

	mutex_unlock(&conn->lock);

	l2cap_conn_put(conn);

	return 0;

}



static struct hci_cb l2cap_cb = {

net/bluetooth/sco.c

+26 −9
Original line number Diff line number Diff line
@@ -1458,22 +1458,39 @@ static void sco_disconn_cfm(struct hci_conn *hcon, __u8 reason) 
	sco_conn_del(hcon, bt_to_errno(reason));

}



void sco_recv_scodata(struct hci_conn *hcon, struct sk_buff *skb)

int sco_recv_scodata(struct hci_dev *hdev, u16 handle, struct sk_buff *skb)

{

	struct sco_conn *conn = hcon->sco_data;

	struct hci_conn *hcon;

	struct sco_conn *conn;



	if (!conn)

		goto drop;

	hci_dev_lock(hdev);



	BT_DBG("conn %p len %u", conn, skb->len);

	hcon = hci_conn_hash_lookup_handle(hdev, handle);

	if (!hcon) {

		hci_dev_unlock(hdev);

		kfree_skb(skb);

		return -ENOENT;

	}



	if (skb->len) {

		sco_recv_frame(conn, skb);

		return;

	conn = sco_conn_hold_unless_zero(hcon->sco_data);

	hcon = NULL;



	hci_dev_unlock(hdev);



	if (!conn) {

		kfree_skb(skb);

		return -EINVAL;

	}



drop:

	BT_DBG("conn %p len %u", conn, skb->len);



	if (skb->len)

		sco_recv_frame(conn, skb);

	else

		kfree_skb(skb);



	sco_conn_put(conn);

	return 0;

}



static struct hci_cb sco_cb = {