diff mbox series

[3/7] Bluetooth: L2CAP: use refcount on struct l2cap_chan->conn

Message ID 20230904221158.35425-4-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
We have detected a memory leak of "struct l2cap_conn" objects, and of
"struct hci_conn" in the bluetooth subsystem [12]. The second is a
consequence of the first. This memory leak was also detected by syzbot
[13].

"struct l2cap_conn" is allocated in "l2cap_conn_add()" [2]. "struct
l2cap_conn" is reference-counted [1] by its "ref" member [3]: every
holder of a reference to it should increment the reference counter by
calling "l2cap_conn_get()" [4], and every reference holder that stops
keeping the reference should call "l2cap_conn_put()" [5]. The "struct
l2cap_conn" is freed when the counter reaches 0, meaning there is no
reference holder any more.

This mechanism is already used by the "hidp_session_new()" [6] and
"session_free()" [7] functions that create and delete the "struct
hidp_session" [8] objects, which contains a "conn" reference to a
"struct l2cap_conn".

The same reference counting mechanism is not used for the "conn"
reference kept in the "struct l2cap_chan" [9] object. There were two
places where the reference counting was not applied:

 1. In "__l2cap_chan_add()" [11], the "struct l2cap_conn" reference is
    stored in the "conn" member of the "struct l2cap_chan" [9].
 2. In "l2cap_chan_del()" [10], the "conn" member of the "struct
    l2cap_chan" is set to NULL.

To apply the reference counting to the "conn" member of the "struct
l2cap_chan", we use "l2cap_conn_get()" in "__l2cap_chan_add()", where
we store the reference, and "l2cap_conn_put()" in "l2cap_chan_del()",
where we drop the reference.

Handling the "conn" member of the "struct l2cap_chan" with the
existing reference counter will help to fix the kernel memory leak in
a following commit.

References:
- [1]  "Adding reference counters (krefs) to kernel objects"
       <https://www.kernel.org/doc/html/v6.5/core-api/kref.html>
- [2]  "l2cap_conn_add()"
       <https://elixir.bootlin.com/linux/v6.5/source/net/bluetooth/l2cap_core.c#L7833>
- [3]  "struct l2cap_conn"
       <https://elixir.bootlin.com/linux/v6.5/source/include/net/bluetooth/l2cap.h#L674>
- [4]  "l2cap_conn_get()"
       <https://elixir.bootlin.com/linux/v6.5/source/net/bluetooth/l2cap_core.c#L1952>
- [5]  "l2cap_conn_put()"
       <https://elixir.bootlin.com/linux/v6.5/source/net/bluetooth/l2cap_core.c#L1959>
- [6]  "hidp_session_new()"
       <https://elixir.bootlin.com/linux/v6.5/source/net/bluetooth/hidp/core.c#L910>
- [7]  "session_free()"
       <https://elixir.bootlin.com/linux/v6.5/source/net/bluetooth/hidp/core.c#L979>
- [8]  "struct hidp_session"
       <https://elixir.bootlin.com/linux/v6.5/source/net/bluetooth/hidp/hidp.h#L137>
- [9]  "struct l2cap_chan"
       <https://elixir.bootlin.com/linux/v6.5/source/include/net/bluetooth/l2cap.h#L540>
- [10] "l2cap_chan_del()"
       <https://elixir.bootlin.com/linux/v6.5/source/net/bluetooth/l2cap_core.c#L642>
- [11] "__l2cap_chan_add()"
       <https://elixir.bootlin.com/linux/v6.5/source/net/bluetooth/l2cap_core.c#L583>
- [12] "ble-memleak-repro"
       <https://gitlab.com/essensium-mind/ble-memleak-repro.git>
- [13] "[syzbot] [bluetooth?] memory leak in hci_conn_add (2)"
       <https://lore.kernel.org/linux-bluetooth/0000000000000fd01206046e7410@google.com/T/#u>

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

Patch

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index c749b434df97..768632fba6f8 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -587,7 +587,7 @@  void __l2cap_chan_add(struct l2cap_conn *conn, struct l2cap_chan *chan)
 
 	conn->disc_reason = HCI_ERROR_REMOTE_USER_TERM;
 
-	chan->conn = conn;
+	chan->conn = l2cap_conn_get(conn);
 
 	switch (chan->chan_type) {
 	case L2CAP_CHAN_CONN_ORIENTED:
@@ -669,6 +669,8 @@  void l2cap_chan_del(struct l2cap_chan *chan, int err)
 
 		if (mgr && mgr->bredr_chan == chan)
 			mgr->bredr_chan = NULL;
+
+		l2cap_conn_put(conn);
 	}
 
 	if (chan->hs_hchan) {