| Message ID | 20230904221158.35425-4-olivier.lheureux@mind.be |
|---|---|
| State | New |
| Headers | show |
| Series | Fix a memory leak in Bluetooth L2CAP | expand |
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) {
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(-)