netdev - Re: [PATCH net-next 1/2] tcp: conditionally call ip_icmp_error() from tcp_v4

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAL+tcoAMEM-yyahPY=bZkj+i8niePOVGEO8OzjeuWA871xFc+g@mail.gmail.com>
Date: Thu, 18 Apr 2024 14:53:05 +0800
From: Jason Xing <kerneljasonxing@...il.com>
To: Eric Dumazet <edumazet@...gle.com>
Cc: "David S . Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>, 
	Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org, 
	Neal Cardwell <ncardwell@...gle.com>, Dragos Tatulea <dtatulea@...dia.com>, eric.dumazet@...il.com, 
	Maciej Żenczykowski <maze@...gle.com>, 
	Willem de Bruijn <willemb@...gle.com>, Shachar Kagan <skagan@...dia.com>
Subject: Re: [PATCH net-next 1/2] tcp: conditionally call ip_icmp_error() from tcp_v4_err()

On Thu, Apr 18, 2024 at 2:45 PM Eric Dumazet <edumazet@...gle.com> wrote:
>
> On Thu, Apr 18, 2024 at 5:23 AM Jason Xing <kerneljasonxing@...il.com> wrote:
> >
> > On Thu, Apr 18, 2024 at 12:59 AM Eric Dumazet <edumazet@...gle.com> wrote:
> > >
> > > Blamed commit claimed in its changelog that the new functionality
> > > was guarded by IP_RECVERR/IPV6_RECVERR :
> > >
> > >     Note that applications need to set IP_RECVERR/IPV6_RECVERR option to
> > >     enable this feature, and that the error message is only queued
> > >     while in SYN_SNT state.
> > >
> > > This was true only for IPv6, because ipv6_icmp_error() has
> > > the following check:
> > >
> > > if (!inet6_test_bit(RECVERR6, sk))
> > >     return;
> > >
> > > Other callers check IP_RECVERR by themselves, it is unclear
> > > if we could factorize these checks in ip_icmp_error()
> > >
> > > For stable backports, I chose to add the missing check in tcp_v4_err()
> > >
> > > We think this missing check was the root cause for commit
> > > 0a8de364ff7a ("tcp: no longer abort SYN_SENT when receiving
> > > some ICMP") breakage, leading to a revert.
> > >
> > > Many thanks to Dragos Tatulea for conducting the investigations.
> > >
> > > As Jakub said :
> > >
> > >     The suspicion is that SSH sees the ICMP report on the socket error queue
> > >     and tries to connect() again, but due to the patch the socket isn't
> > >     disconnected, so it gets EALREADY, and throws its hands up...
> > >
> > >     The error bubbles up to Vagrant which also becomes unhappy.
> > >
> > >     Can we skip the call to ip_icmp_error() for non-fatal ICMP errors?
> > >
> > > Fixes: 45af29ca761c ("tcp: allow traceroute -Mtcp for unpriv users")
> > > Signed-off-by: Eric Dumazet <edumazet@...gle.com>
> > > Tested-by: Dragos Tatulea <dtatulea@...dia.com>
> > > Cc: Dragos Tatulea <dtatulea@...dia.com>
> > > Cc: Maciej Żenczykowski <maze@...gle.com>
> > > Cc: Willem de Bruijn <willemb@...gle.com>
> > > Cc: Neal Cardwell <ncardwell@...gle.com>
> > > Cc: Shachar Kagan <skagan@...dia.com>
> >
> > Reviewed-by: Jason Xing <kerneljasonxing@...il.com>
> >
> > I wonder if we're supposed to move this check into ip_icmp_error()
> > like ipv6_icmp_error() does, because I notice one caller
> > rxrpc_encap_err_rcv() without checking RECVERR  bit reuses the ICMP
> > error logic which is introduced in commit b6c66c4324e7 ("rxrpc: Use
> > the core ICMP/ICMP6 parsers'')?
>
> I tried to focus on the TCP issues, and to have a stable candidate for patch #1.
>
> The refactoring can wait.

Got it. It's clear.

After this patch is applied, I can adjust a little bit (only by moving
it into ip_icmp_error()).

Thanks,
Jason

>
> >
> > Or should it be a follow-up patch (moving it inside of
> > ip_icmp_error()) to handle the rxrpc case and also prevent future
> > misuse for other people?