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: <20240417165756.2531620-2-edumazet@google.com>
Date: Wed, 17 Apr 2024 16:57:55 +0000
From: Eric Dumazet <edumazet@...gle.com>
To: "David S . Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>, 
	Paolo Abeni <pabeni@...hat.com>
Cc: netdev@...r.kernel.org, Neal Cardwell <ncardwell@...gle.com>, 
	Dragos Tatulea <dtatulea@...dia.com>, eric.dumazet@...il.com, 
	Eric Dumazet <edumazet@...gle.com>, 
	"Maciej Żenczykowski" <maze@...gle.com>, Willem de Bruijn <willemb@...gle.com>, Shachar Kagan <skagan@...dia.com>
Subject: [PATCH net-next 1/2] tcp: conditionally call ip_icmp_error() from tcp_v4_err()

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>
---
 net/ipv4/tcp_ipv4.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 88c83ac4212957f19efad0f967952d2502bdbc7f..a717db99972d977a64178d7ed1109325d64a6d51 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -602,7 +602,8 @@ int tcp_v4_err(struct sk_buff *skb, u32 info)
 		if (fastopen && !fastopen->sk)
 			break;
 
-		ip_icmp_error(sk, skb, err, th->dest, info, (u8 *)th);
+		if (inet_test_bit(RECVERR, sk))
+			ip_icmp_error(sk, skb, err, th->dest, info, (u8 *)th);
 
 		if (!sock_owned_by_user(sk)) {
 			WRITE_ONCE(sk->sk_err, err);
-- 
2.44.0.683.g7961c838ac-goog


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ