| From be5d1b61a2ad28c7e57fe8bfa277373e8ecffcdc Mon Sep 17 00:00:00 2001 |
| From: Nguyen Dinh Phi <phind.uet@gmail.com> |
| Date: Tue, 6 Jul 2021 07:19:12 +0800 |
| Subject: tcp: fix tcp_init_transfer() to not reset icsk_ca_initialized |
| |
| From: Nguyen Dinh Phi <phind.uet@gmail.com> |
| |
| commit be5d1b61a2ad28c7e57fe8bfa277373e8ecffcdc upstream. |
| |
| This commit fixes a bug (found by syzkaller) that could cause spurious |
| double-initializations for congestion control modules, which could cause |
| memory leaks or other problems for congestion control modules (like CDG) |
| that allocate memory in their init functions. |
| |
| The buggy scenario constructed by syzkaller was something like: |
| |
| (1) create a TCP socket |
| (2) initiate a TFO connect via sendto() |
| (3) while socket is in TCP_SYN_SENT, call setsockopt(TCP_CONGESTION), |
| which calls: |
| tcp_set_congestion_control() -> |
| tcp_reinit_congestion_control() -> |
| tcp_init_congestion_control() |
| (4) receive ACK, connection is established, call tcp_init_transfer(), |
| set icsk_ca_initialized=0 (without first calling cc->release()), |
| call tcp_init_congestion_control() again. |
| |
| Note that in this sequence tcp_init_congestion_control() is called |
| twice without a cc->release() call in between. Thus, for CC modules |
| that allocate memory in their init() function, e.g, CDG, a memory leak |
| may occur. The syzkaller tool managed to find a reproducer that |
| triggered such a leak in CDG. |
| |
| The bug was introduced when that commit 8919a9b31eb4 ("tcp: Only init |
| congestion control if not initialized already") |
| introduced icsk_ca_initialized and set icsk_ca_initialized to 0 in |
| tcp_init_transfer(), missing the possibility for a sequence like the |
| one above, where a process could call setsockopt(TCP_CONGESTION) in |
| state TCP_SYN_SENT (i.e. after the connect() or TFO open sendmsg()), |
| which would call tcp_init_congestion_control(). It did not intend to |
| reset any initialization that the user had already explicitly made; |
| it just missed the possibility of that particular sequence (which |
| syzkaller managed to find). |
| |
| Fixes: 8919a9b31eb4 ("tcp: Only init congestion control if not initialized already") |
| Reported-by: syzbot+f1e24a0594d4e3a895d3@syzkaller.appspotmail.com |
| Signed-off-by: Nguyen Dinh Phi <phind.uet@gmail.com> |
| Acked-by: Neal Cardwell <ncardwell@google.com> |
| Tested-by: Neal Cardwell <ncardwell@google.com> |
| Signed-off-by: David S. Miller <davem@davemloft.net> |
| Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> |
| --- |
| net/ipv4/tcp_input.c | 2 +- |
| 1 file changed, 1 insertion(+), 1 deletion(-) |
| |
| --- a/net/ipv4/tcp_input.c |
| +++ b/net/ipv4/tcp_input.c |
| @@ -5911,8 +5911,8 @@ void tcp_init_transfer(struct sock *sk, |
| tp->snd_cwnd = tcp_init_cwnd(tp, __sk_dst_get(sk)); |
| tp->snd_cwnd_stamp = tcp_jiffies32; |
| |
| - icsk->icsk_ca_initialized = 0; |
| bpf_skops_established(sk, bpf_op, skb); |
| + /* Initialize congestion control unless BPF initialized it already: */ |
| if (!icsk->icsk_ca_initialized) |
| tcp_init_congestion_control(sk); |
| tcp_init_buffer_space(sk); |