Message-ID: <CADVnQyk9maCc+tJ4-b6kufcBES9+Y2KpHPZadXssoVWX=Xr1Vw@mail.gmail.com>
Date:   Tue, 29 Jun 2021 11:59:43 -0400
From:   Neal Cardwell <ncardwell@...gle.com>
To:     Eric Dumazet <edumazet@...gle.com>
Cc:     Nguyen Dinh Phi <phind.uet@...il.com>,
        David Miller <davem@...emloft.net>,
        Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
        David Ahern <dsahern@...nel.org>,
        Jakub Kicinski <kuba@...nel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>,
        John Fastabend <john.fastabend@...il.com>, kpsingh@...nel.org,
        netdev <netdev@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        linux-kernel-mentees@...ts.linuxfoundation.org,
        syzbot+f1e24a0594d4e3a895d3@...kaller.appspotmail.com,
        Yuchung Cheng <ycheng@...gle.com>
Subject: Re: [PATCH] tcp: Do not reset the icsk_ca_initialized in tcp_init_transfer.

  On Tue, Jun 29, 2021 at 8:58 AM Eric Dumazet <edumazet@...gle.com> wrote:
> > Because the problem only happens with CDG, is adding check in its tcp_cdg_init() function Ok? And about  icsk_ca_initialized, Could I expect it to be 0 in CC's init functions?
>
> I think icsk_ca_initialized  lost its strong meaning when CDG was
> introduced (since this is the only CC allocating memory)
>
> The bug really is that before clearing icsk_ca_initialized we should
> call cc->release()
>
> Maybe we missed this cleanup in commit
> 8919a9b31eb4fb4c0a93e5fb350a626924302aa6 ("tcp: Only init congestion
> control if not initialized already")

>From my perspective, the bug was introduced when that 8919a9b31eb4
commit introduced icsk_ca_initialized and set icsk_ca_initialized to 0
in tcp_init_transfer(), missing the possibility that 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(). The 8919a9b31eb4 commit 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!).

> Although I am not sure what happens at accept() time when the listener
> socket is cloned.

It seems that for listener sockets, they cannot initialize their CC
module state, because there is no way for them to reach
tcp_init_congestion_control(), since:

(a) tcp_set_congestion_control() -> tcp_reinit_congestion_control()
will not call tcp_init_congestion_control() on a socket in CLOSE or
LISTEN

(b) tcp_init_transfer() -> tcp_init_congestion_control() can only
happen for established sockets and successful TFO SYN_RECV sockets

So it seems my previously suggested change (yesterday in this thread)
to add icsk_ca_initialized=0 in tcp_ca_openreq_child() is not needed.

> If we make any hypothesis, we need to check all CC modules to make
> sure they respect it.

AFAICT the fix is correct; it just needs a Fixes: tag and a more clear
description in the commit message.

I have cherry-picked the patch into our kernel and verified it passes
all of our internal packetdrill tests.

So the diff seems OK, but I would suggest a commit message something
like the following:

--
[PATCH] tcp: fix tcp_init_transfer() to not reset icsk_ca_initialized

This commit fixes a bug (found by syzkaller) that could cause spurious
double-initializations for congestion control modules, which could cause memory
leaks orother 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 8919a9b31eb4 commit 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(). The 8919a9b31eb4 commit 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@...kaller.appspotmail.com
Signed-off-by: Nguyen Dinh Phi <phind.uet@...il.com>
--

neal

Powered by blists - more mailing lists