| Message ID | 20240613082300.294668-1-Ilia.Gavrilov@infotecs.ru (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 0b9130247f3b6a1122478471ff0e014ea96bb735 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] netrom: Fix a memory leak in nr_heartbeat_expiry() | expand |
Hello: This patch was applied to netdev/net.git (main) by David S. Miller <davem@davemloft.net>: On Thu, 13 Jun 2024 08:23:00 +0000 you wrote: > syzbot reported a memory leak in nr_create() [0]. > > Commit 409db27e3a2e ("netrom: Fix use-after-free of a listening socket.") > added sock_hold() to the nr_heartbeat_expiry() function, where > a) a socket has a SOCK_DESTROY flag or > b) a listening socket has a SOCK_DEAD flag. > > [...] Here is the summary with links: - [net] netrom: Fix a memory leak in nr_heartbeat_expiry() https://git.kernel.org/netdev/net/c/0b9130247f3b You are awesome, thank you!
diff --git a/net/netrom/nr_timer.c b/net/netrom/nr_timer.c index 4e7c968cde2d..5e3ca068f04e 100644 --- a/net/netrom/nr_timer.c +++ b/net/netrom/nr_timer.c @@ -121,7 +121,8 @@ static void nr_heartbeat_expiry(struct timer_list *t) is accepted() it isn't 'dead' so doesn't get removed. */ if (sock_flag(sk, SOCK_DESTROY) || (sk->sk_state == TCP_LISTEN && sock_flag(sk, SOCK_DEAD))) { - sock_hold(sk); + if (sk->sk_state == TCP_LISTEN) + sock_hold(sk); bh_unlock_sock(sk); nr_destroy_socket(sk); goto out;
syzbot reported a memory leak in nr_create() [0]. Commit 409db27e3a2e ("netrom: Fix use-after-free of a listening socket.") added sock_hold() to the nr_heartbeat_expiry() function, where a) a socket has a SOCK_DESTROY flag or b) a listening socket has a SOCK_DEAD flag. But in the case "a," when the SOCK_DESTROY flag is set, the file descriptor has already been closed and the nr_release() function has been called. So it makes no sense to hold the reference count because no one will call another nr_destroy_socket() and put it as in the case "b." nr_connect nr_establish_data_link nr_start_heartbeat nr_release switch (nr->state) case NR_STATE_3 nr->state = NR_STATE_2 sock_set_flag(sk, SOCK_DESTROY); nr_rx_frame nr_process_rx_frame switch (nr->state) case NR_STATE_2 nr_state2_machine() nr_disconnect() nr_sk(sk)->state = NR_STATE_0 sock_set_flag(sk, SOCK_DEAD) nr_heartbeat_expiry switch (nr->state) case NR_STATE_0 if (sock_flag(sk, SOCK_DESTROY) || (sk->sk_state == TCP_LISTEN && sock_flag(sk, SOCK_DEAD))) sock_hold() // ( !!! ) nr_destroy_socket() To fix the memory leak, let's call sock_hold() only for a listening socket. Found by InfoTeCS on behalf of Linux Verification Center (linuxtesting.org) with Syzkaller. [0]: https://syzkaller.appspot.com/bug?extid=d327a1f3b12e1e206c16 Reported-by: syzbot+d327a1f3b12e1e206c16@syzkaller.appspotmail.com Closes: https://syzkaller.appspot.com/bug?extid=d327a1f3b12e1e206c16 Fixes: 409db27e3a2e ("netrom: Fix use-after-free of a listening socket.") Signed-off-by: Gavrilov Ilia <Ilia.Gavrilov@infotecs.ru> --- net/netrom/nr_timer.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)