diff mbox series

[6/7] Bluetooth: L2CAP: inc refcount if reuse struct l2cap_conn

Message ID 20230904221158.35425-7-olivier.lheureux@mind.be
State New
Headers show
Series Fix a memory leak in Bluetooth L2CAP | expand

Commit Message

Olivier L'Heureux Sept. 4, 2023, 10:11 p.m. UTC
Now that the "struct l2cap_conn" memory leak [1] is fixed, we observe
use-after-free errors.

Arnout Vandecappelle has found the root cause: the

    if (conn)
            return conn;

at the beginning of "l2cap_conn_add()". It reuses an existing "struct
l2cap_conn *" instead of allocating a new one.

But there is an incoherence in the reference counting:

 1. In the normal case (no reuse), "l2cap_conn_add()" allocates a new
    "struct l2cap_conn", initiates its "ref" reference counter to 1 and
    returns it. The caller is responsible to eventually call
    "l2cap_conn_put()" to free the returned "struct l2cap_conn".
 2. If "l2cap_conn_add()" reuses an existing "struct l2cap_conn", it
    will return it immediately. But the caller, which can not know if
    "l2cap_conn_add()" reused an existing "struct l2cap_conn" or not,
    will eventually call "l2cap_conn_put()", for which there were no
    corresponding "l2cap_conn_get()".

Tracing the reuse showed the reuse was indeed the reason for the
use-after-free:

  [...]
  [  960.331756] l2cap_conn_add:7719: hcon 1f5f8bdf reuse conn b69c7ec5
  [  960.331798] ------------[ cut here ]------------
  [  960.339480] WARNING: CPU: 0 PID: 173 at lib/refcount.c:25 l2cap_conn_get+0x8c/0x94
  [  960.353863] refcount_t: addition on 0; use-after-free.
  [  960.362924] Modules linked in:
  [  960.368036] CPU: 0 PID: 173 Comm: ble-memleak-rep Not tainted 5.13.0 #19
  [  960.380245] Hardware name: STM32 (Device Tree Support)
  [  960.387449] [<c010e9a4>] (unwind_backtrace) from [<c010af48>] (show_stack+0x10/0x14)
  [  960.400742] [<c010af48>] (show_stack) from [<c07f98dc>] (dump_stack+0xb4/0xc8)
  [  960.413501] [<c07f98dc>] (dump_stack) from [<c07f6ee0>] (__warn+0xb8/0x114)
  [  960.425994] [<c07f6ee0>] (__warn) from [<c07f6fb4>] (warn_slowpath_fmt+0x78/0xac)
  [  960.439016] [<c07f6fb4>] (warn_slowpath_fmt) from [<c07d2690>] (l2cap_conn_get+0x8c/0x94)
  [  960.452752] [<c07d2690>] (l2cap_conn_get) from [<c07d92b0>] (__l2cap_chan_add+0x3c/0x1e4)
  [  960.466486] [<c07d92b0>] (__l2cap_chan_add) from [<c07da8d0>] (l2cap_chan_connect+0x514/0x9c8)
  [  960.480656] [<c07da8d0>] (l2cap_chan_connect) from [<c07e12b8>] (l2cap_sock_connect+0x144/0x21c)
  [  960.495023] [<c07e12b8>] (l2cap_sock_connect) from [<c065c5e0>] (__sys_connect+0xc8/0xe0)
  [  960.508927] [<c065c5e0>] (__sys_connect) from [<c0100060>] (ret_fast_syscall+0x0/0x58)
  [...]

The solution to the incoherence is to strictly apply the rules of
reference counting [2]. By returning a reference to the reused "struct
l2cap_conn", "l2cap_conn_add()" creates a new reference, and this
reference should be reference-counted. The:

    if (conn)
            return conn;

must thus become:

    if (conn)
            return l2cap_conn_get(conn);

References:
- [1] "ble-memleak-repro"
      <https://gitlab.com/essensium-mind/ble-memleak-repro.git>
- [2] "Adding reference counters (krefs) to kernel objects"
      <https://www.kernel.org/doc/html/latest/core-api/kref.html>

Signed-off-by: Olivier L'Heureux <olivier.lheureux@fortrobotics.com>
Signed-off-by: Olivier L'Heureux <olivier.lheureux@mind.be>
Suggested-by: Arnout Vandecappelle <arnout.vandecappelle@mind.be>
---
 net/bluetooth/l2cap_core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
diff mbox series

Patch

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index f5dcb4a4fb15..5e4dd293b2a4 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -7844,8 +7844,8 @@  static struct l2cap_conn *l2cap_conn_add(struct hci_conn *hcon)
 	struct hci_chan *hchan;
 
 	if (conn) {
-		BT_DBG("hcon %p reuse conn %p", hcon, conn);
-		return conn;
+		BT_DBG("hcon %p reuse conn %p with l2cap_conn_get()", hcon, conn);
+		return l2cap_conn_get(conn);
 	}
 
 	hchan = hci_chan_create(hcon);