@@ -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);
}
@@ -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);
}
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(+)