[PATCH 7/7] Bluetooth: unlink objects to avoid use-after-free

[Olivier L'Heureux <olivier.lheureux@mind.be>]

While stressing the Bluetooth subsystem with constant scanning [1],
we have observed use-after-free in a "l2cap_conn_get()" called from
"l2cap_chan_connect()". Our log could show "l2cap_conn_add()" was
reusing an existing "struct l2cap_conn" that was freed, and which
reference counter was at 0:

  [...]
  [  782.314141] [166] l2cap_conn_put:1940: conn cb471e8c, refcount 1
  [  782.314162] [166] l2cap_conn_free:1923: kfree(conn) cb471e8c
  [...]
  [  782.314693] [166] l2cap_chan_connect:7829: 00:00:00:00:00:00 -> 00:22:a3:07:0a:00 (type 1) psm 0x0080 mode 0x00
  [  782.314721] [166] hci_get_route:692: 00:00:00:00:00:00 -> 00:22:a3:07:0a:00
  [  782.314745] [166] hci_conn_hold:1152: hcon bb2debe3 orig conn->refcnt 0
  [  782.314766] [166] l2cap_conn_add:7719: hcon bb2debe3 reuse conn cb471e8c
  [  782.314786] [166] l2cap_conn_get:1931: conn cb471e8c, refcount 0
  [  782.314802] ------------[ cut here ]------------
  [  782.322645] WARNING: CPU: 1 PID: 166 at lib/refcount.c:25 l2cap_conn_get+0x8c/0x94
  [  782.336633] [57] le_scan_cleanup:156: hci0 hcon bb2debe3
  [  782.336669] refcount_t: addition on 0; use-after-free.
  [  782.344020] Modules linked in:
  [  782.349187] CPU: 1 PID: 166 Comm: ble-memleak-rep Not tainted 5.13.0 #20
  [  782.361524] Hardware name: STM32 (Device Tree Support)
  [  782.368778] [<c010e9a4>] (unwind_backtrace) from [<c010af48>] (show_stack+0x10/0x14)
  [  782.382160] [<c010af48>] (show_stack) from [<c07f9868>] (dump_stack+0xb4/0xc8)
  [  782.395026] [<c07f9868>] (dump_stack) from [<c07f6e6c>] (__warn+0xb8/0x114)
  [  782.407785] [<c07f6e6c>] (__warn) from [<c07f6f40>] (warn_slowpath_fmt+0x78/0xac)
  [  782.421203] [<c07f6f40>] (warn_slowpath_fmt) from [<c07d2400>] (l2cap_conn_get+0x8c/0x94)
  [  782.435352] [<c07d2400>] (l2cap_conn_get) from [<c07da654>] (l2cap_chan_connect+0x278/0x984)
  [  782.449802] [<c07da654>] (l2cap_chan_connect) from [<c07e1244>] (l2cap_sock_connect+0x144/0x21c)
  [  782.464717] [<c07e1244>] (l2cap_sock_connect) from [<c065c5e0>] (__sys_connect+0xc8/0xe0)
  [  782.479054] [<c065c5e0>] (__sys_connect) from [<c0100060>] (ret_fast_syscall+0x0/0x58)
  [...]

Our proposed solution is to avoid such a situation: the "struct
l2cap_conn" was reused via a dangling pointer, since the "struct
l2cap_conn" was freed. The pointers are the double-linked pointers
between "struct hci_conn" and "struct l2cap_conn". We can at least set
the dangling pointers to NULL, just before we free an object. This
will avoid the use-after-free.

Done: just before freeing a "struct hci_conn", set the corresponding
pointer to NULL in "struct l2cap_conn", and just before freeing a
"struct l2cap_conn", set the corresponding pointer to NULL in "struct
hci_conn".

Note that we take care for NULL when dereferencing the pointers. In:

  struct hci_conn *hcon;
  struct l2cap_conn *lcon;

expressions like "hcon->l2cap_data->hcon" or "lcon->hcon->l2cap_data"
could dereference NULL pointers, because "hcon->l2cap_data" or
"lcon->hcon" could be NULL. Indeed, "l2cap_conn_free()" and
"hci_conn_free()" run in different threads, potentially in parallel.

References:
- [1] "ble-memleak-repro"
      <https://gitlab.com/essensium-mind/ble-memleak-repro.git>

Signed-off-by: Olivier L'Heureux <olivier.lheureux@fortrobotics.com>
Signed-off-by: Olivier L'Heureux <olivier.lheureux@mind.be>
---
 net/bluetooth/hci_conn.c   | 7 +++++++
 net/bluetooth/l2cap_core.c | 5 +++++
 2 files changed, 12 insertions(+)

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 755125403331..86446f482b9f 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -1136,8 +1136,15 @@ static void hci_conn_unlink(struct hci_conn *conn)
 
 void hci_conn_free(struct hci_conn *conn)
 {
+	struct l2cap_conn *lcon = conn->l2cap_data;
+
 	BT_DBG("kfree(conn %p)", conn);
 
+	if (lcon && lcon->hcon == conn) {
+		BT_DBG("conn %p conn->l2cap_data->hcon = NULL", conn);
+		lcon->hcon = NULL;
+	}
+
 	kfree(conn);
 }
 
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 5e4dd293b2a4..9c2384c32d93 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -1949,6 +1949,11 @@ static void l2cap_conn_free(struct kref *ref)
 
 	BT_DBG("kfree(conn) %p", conn);
 
+	if (conn->hcon && conn->hcon->l2cap_data == conn) {
+		BT_DBG("conn %p conn->hcon->l2cap_data = NULL", conn);
+		conn->hcon->l2cap_data = NULL;
+	}
+
 	hci_conn_put(conn->hcon);
 	kfree(conn);
 }
-- 
2.39.2