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@xxxxxxxxxxxxxxxx>
Signed-off-by: Olivier L'Heureux <olivier.lheureux@xxxxxxx>
---
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
