diff mbox series

[v16,net-next,15/23] net/tcp: Add tcp_hash_fail() ratelimited logs

Message ID 20231023192217.426455-16-dima@arista.com (mailing list archive)
State Accepted
Commit 2717b5adea9e2558798c30eb0e93c01722edbb0a
Delegated to: Netdev Maintainers
Headers show
Series [v16,net-next,01/23] net/tcp: Prepare tcp_md5sig_pool for TCP-AO | expand

Checks

Context Check Description
netdev/series_format warning Pull request is its own cover letter; Series longer than 15 patches
netdev/tree_selection success Clearly marked for net-next, async
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 2350 this patch: 2350
netdev/cc_maintainers success CCed 6 of 6 maintainers
netdev/build_clang success Errors and warnings before: 1507 this patch: 1507
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 2402 this patch: 2402
netdev/checkpatch warning CHECK: Alignment should match open parenthesis WARNING: line length of 83 exceeds 80 columns WARNING: line length of 84 exceeds 80 columns WARNING: line length of 88 exceeds 80 columns
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Dmitry Safonov Oct. 23, 2023, 7:22 p.m. UTC 
Add a helper for logging connection-detailed messages for failed TCP
hash verification (both MD5 and AO).

Co-developed-by: Francesco Ruggeri <fruggeri@arista.com>
Signed-off-by: Francesco Ruggeri <fruggeri@arista.com>
Co-developed-by: Salam Noureddine <noureddine@arista.com>
Signed-off-by: Salam Noureddine <noureddine@arista.com>
Signed-off-by: Dmitry Safonov <dima@arista.com>
Acked-by: David Ahern <dsahern@kernel.org>
---
 include/net/tcp.h    | 14 ++++++++++++--
 include/net/tcp_ao.h | 29 +++++++++++++++++++++++++++++
 net/ipv4/tcp.c       | 23 +++++++++++++----------
 net/ipv4/tcp_ao.c    |  7 +++++++
 4 files changed, 61 insertions(+), 12 deletions(-)

Comments

Eric Dumazet Oct. 30, 2023, 8:16 a.m. UTC | #1 
On Mon, Oct 23, 2023 at 9:22 PM Dmitry Safonov <dima@arista.com> wrote:
>
> Add a helper for logging connection-detailed messages for failed TCP
> hash verification (both MD5 and AO).
>
> Co-developed-by: Francesco Ruggeri <fruggeri@arista.com>
> Signed-off-by: Francesco Ruggeri <fruggeri@arista.com>
> Co-developed-by: Salam Noureddine <noureddine@arista.com>
> Signed-off-by: Salam Noureddine <noureddine@arista.com>
> Signed-off-by: Dmitry Safonov <dima@arista.com>
> Acked-by: David Ahern <dsahern@kernel.org>
> ---
>  include/net/tcp.h    | 14 ++++++++++++--
>  include/net/tcp_ao.h | 29 +++++++++++++++++++++++++++++
>  net/ipv4/tcp.c       | 23 +++++++++++++----------
>  net/ipv4/tcp_ao.c    |  7 +++++++
>  4 files changed, 61 insertions(+), 12 deletions(-)
>
> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index d29c8a867f0e..c93ac6cc12c4 100644
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -2746,12 +2746,18 @@ tcp_inbound_hash(struct sock *sk, const struct request_sock *req,
>         int l3index;
>
>         /* Invalid option or two times meet any of auth options */
> -       if (tcp_parse_auth_options(th, &md5_location, &aoh))
> +       if (tcp_parse_auth_options(th, &md5_location, &aoh)) {
> +               tcp_hash_fail("TCP segment has incorrect auth options set",
> +                             family, skb, "");
>                 return SKB_DROP_REASON_TCP_AUTH_HDR;
> +       }
>
>         if (req) {
>                 if (tcp_rsk_used_ao(req) != !!aoh) {
>                         NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPAOBAD);
> +                       tcp_hash_fail("TCP connection can't start/end using TCP-AO",
> +                                     family, skb, "%s",
> +                                     !aoh ? "missing AO" : "AO signed");
>                         return SKB_DROP_REASON_TCP_AOFAILURE;
>                 }
>         }
> @@ -2768,10 +2774,14 @@ tcp_inbound_hash(struct sock *sk, const struct request_sock *req,
>                  * the last key is impossible to remove, so there's
>                  * always at least one current_key.
>                  */
> -               if (tcp_ao_required(sk, saddr, family, true))
> +               if (tcp_ao_required(sk, saddr, family, true)) {
> +                       tcp_hash_fail("AO hash is required, but not found",
> +                                       family, skb, "L3 index %d", l3index);
>                         return SKB_DROP_REASON_TCP_AONOTFOUND;
> +               }
>                 if (unlikely(tcp_md5_do_lookup(sk, l3index, saddr, family))) {
>                         NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPMD5NOTFOUND);
> +                       tcp_hash_fail("MD5 Hash not found", family, skb, "");
>                         return SKB_DROP_REASON_TCP_MD5NOTFOUND;
>                 }
>                 return SKB_NOT_DROPPED_YET;
> diff --git a/include/net/tcp_ao.h b/include/net/tcp_ao.h
> index 0c3516d1b968..4da6e3657913 100644
> --- a/include/net/tcp_ao.h
> +++ b/include/net/tcp_ao.h
> @@ -118,6 +118,35 @@ struct tcp_ao_info {
>         struct rcu_head         rcu;
>  };
>
> +#define tcp_hash_fail(msg, family, skb, fmt, ...)                      \
> +do {                                                                   \
> +       const struct tcphdr *th = tcp_hdr(skb);                         \
> +       char hdr_flags[5] = {};                                         \
> +       char *f = hdr_flags;                                            \
> +                                                                       \
> +       if (th->fin)                                                    \
> +               *f++ = 'F';                                             \
> +       if (th->syn)                                                    \
> +               *f++ = 'S';                                             \
> +       if (th->rst)                                                    \
> +               *f++ = 'R';                                             \
> +       if (th->ack)                                                    \
> +               *f++ = 'A';                                             \
> +       if (f != hdr_flags)                                             \
> +               *f = ' ';                                               \

I see no null termination of  hdr_flags[] if FIN+SYN+ACK+RST are all set.

I will send something like:

diff --git a/include/net/tcp_ao.h b/include/net/tcp_ao.h
index a375a171ef3cb37ab1d8246c72c6a3e83f5c9184..5daf96a3dbee14bd3786e19ea4972e351058e6e7
100644
--- a/include/net/tcp_ao.h
+++ b/include/net/tcp_ao.h
@@ -124,7 +124,7 @@ struct tcp_ao_info {
 #define tcp_hash_fail(msg, family, skb, fmt, ...)                      \
 do {                                                                   \
        const struct tcphdr *th = tcp_hdr(skb);                         \
-       char hdr_flags[5] = {};                                         \
+       char hdr_flags[5];                                              \
        char *f = hdr_flags;                                            \
                                                                        \
        if (th->fin)                                                    \
@@ -135,8 +135,7 @@ do {
                         \
                *f++ = 'R';                                             \
        if (th->ack)                                                    \
                *f++ = 'A';                                             \
-       if (f != hdr_flags)                                             \
-               *f = ' ';                                               \
+       *f = 0;                                                         \
        if ((family) == AF_INET) {                                      \
                net_info_ratelimited("%s for (%pI4, %d)->(%pI4, %d)
%s" fmt "\n", \
                                msg, &ip_hdr(skb)->saddr, ntohs(th->source), \
diff mbox series

Patch

diff --git a/include/net/tcp.h b/include/net/tcp.h
index d29c8a867f0e..c93ac6cc12c4 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -2746,12 +2746,18 @@  tcp_inbound_hash(struct sock *sk, const struct request_sock *req,
 	int l3index;
 
 	/* Invalid option or two times meet any of auth options */
-	if (tcp_parse_auth_options(th, &md5_location, &aoh))
+	if (tcp_parse_auth_options(th, &md5_location, &aoh)) {
+		tcp_hash_fail("TCP segment has incorrect auth options set",
+			      family, skb, "");
 		return SKB_DROP_REASON_TCP_AUTH_HDR;
+	}
 
 	if (req) {
 		if (tcp_rsk_used_ao(req) != !!aoh) {
 			NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPAOBAD);
+			tcp_hash_fail("TCP connection can't start/end using TCP-AO",
+				      family, skb, "%s",
+				      !aoh ? "missing AO" : "AO signed");
 			return SKB_DROP_REASON_TCP_AOFAILURE;
 		}
 	}
@@ -2768,10 +2774,14 @@  tcp_inbound_hash(struct sock *sk, const struct request_sock *req,
 		 * the last key is impossible to remove, so there's
 		 * always at least one current_key.
 		 */
-		if (tcp_ao_required(sk, saddr, family, true))
+		if (tcp_ao_required(sk, saddr, family, true)) {
+			tcp_hash_fail("AO hash is required, but not found",
+					family, skb, "L3 index %d", l3index);
 			return SKB_DROP_REASON_TCP_AONOTFOUND;
+		}
 		if (unlikely(tcp_md5_do_lookup(sk, l3index, saddr, family))) {
 			NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPMD5NOTFOUND);
+			tcp_hash_fail("MD5 Hash not found", family, skb, "");
 			return SKB_DROP_REASON_TCP_MD5NOTFOUND;
 		}
 		return SKB_NOT_DROPPED_YET;
diff --git a/include/net/tcp_ao.h b/include/net/tcp_ao.h
index 0c3516d1b968..4da6e3657913 100644
--- a/include/net/tcp_ao.h
+++ b/include/net/tcp_ao.h
@@ -118,6 +118,35 @@  struct tcp_ao_info {
 	struct rcu_head		rcu;
 };
 
+#define tcp_hash_fail(msg, family, skb, fmt, ...)			\
+do {									\
+	const struct tcphdr *th = tcp_hdr(skb);				\
+	char hdr_flags[5] = {};						\
+	char *f = hdr_flags;						\
+									\
+	if (th->fin)							\
+		*f++ = 'F';						\
+	if (th->syn)							\
+		*f++ = 'S';						\
+	if (th->rst)							\
+		*f++ = 'R';						\
+	if (th->ack)							\
+		*f++ = 'A';						\
+	if (f != hdr_flags)						\
+		*f = ' ';						\
+	if ((family) == AF_INET) {					\
+		net_info_ratelimited("%s for (%pI4, %d)->(%pI4, %d) %s" fmt "\n", \
+				msg, &ip_hdr(skb)->saddr, ntohs(th->source), \
+				&ip_hdr(skb)->daddr, ntohs(th->dest),	\
+				hdr_flags, ##__VA_ARGS__);		\
+	} else {							\
+		net_info_ratelimited("%s for [%pI6c]:%u->[%pI6c]:%u %s" fmt "\n", \
+				msg, &ipv6_hdr(skb)->saddr, ntohs(th->source), \
+				&ipv6_hdr(skb)->daddr, ntohs(th->dest),	\
+				hdr_flags, ##__VA_ARGS__);		\
+	}								\
+} while (0)
+
 #ifdef CONFIG_TCP_AO
 /* TCP-AO structures and functions */
 
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 846ddc023ac1..8594a4bf764e 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -4381,7 +4381,6 @@  tcp_inbound_md5_hash(const struct sock *sk, const struct sk_buff *skb,
 	 * o MD5 hash and we're not expecting one.
 	 * o MD5 hash and its wrong.
 	 */
-	const struct tcphdr *th = tcp_hdr(skb);
 	const struct tcp_sock *tp = tcp_sk(sk);
 	struct tcp_md5sig_key *key;
 	u8 newhash[16];
@@ -4391,6 +4390,7 @@  tcp_inbound_md5_hash(const struct sock *sk, const struct sk_buff *skb,
 
 	if (!key && hash_location) {
 		NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPMD5UNEXPECTED);
+		tcp_hash_fail("Unexpected MD5 Hash found", family, skb, "");
 		return SKB_DROP_REASON_TCP_MD5UNEXPECTED;
 	}
 
@@ -4406,16 +4406,19 @@  tcp_inbound_md5_hash(const struct sock *sk, const struct sk_buff *skb,
 	if (genhash || memcmp(hash_location, newhash, 16) != 0) {
 		NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPMD5FAILURE);
 		if (family == AF_INET) {
-			net_info_ratelimited("MD5 Hash failed for (%pI4, %d)->(%pI4, %d)%s L3 index %d\n",
-					saddr, ntohs(th->source),
-					daddr, ntohs(th->dest),
-					genhash ? " tcp_v4_calc_md5_hash failed"
-					: "", l3index);
+			tcp_hash_fail("MD5 Hash failed", AF_INET, skb, "%s L3 index %d",
+				      genhash ? "tcp_v4_calc_md5_hash failed"
+				      : "", l3index);
 		} else {
-			net_info_ratelimited("MD5 Hash %s for [%pI6c]:%u->[%pI6c]:%u L3 index %d\n",
-					genhash ? "failed" : "mismatch",
-					saddr, ntohs(th->source),
-					daddr, ntohs(th->dest), l3index);
+			if (genhash) {
+				tcp_hash_fail("MD5 Hash failed",
+					      AF_INET6, skb, "L3 index %d",
+					      l3index);
+			} else {
+				tcp_hash_fail("MD5 Hash mismatch",
+					      AF_INET6, skb, "L3 index %d",
+					      l3index);
+			}
 		}
 		return SKB_DROP_REASON_TCP_MD5FAILURE;
 	}
diff --git a/net/ipv4/tcp_ao.c b/net/ipv4/tcp_ao.c
index 7e14bcd4dfd4..f76fcb93499d 100644
--- a/net/ipv4/tcp_ao.c
+++ b/net/ipv4/tcp_ao.c
@@ -800,6 +800,8 @@  tcp_ao_verify_hash(const struct sock *sk, const struct sk_buff *skb,
 		NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPAOBAD);
 		atomic64_inc(&info->counters.pkt_bad);
 		atomic64_inc(&key->pkt_bad);
+		tcp_hash_fail("AO hash wrong length", family, skb,
+			      "%u != %d", maclen, tcp_ao_maclen(key));
 		return SKB_DROP_REASON_TCP_AOFAILURE;
 	}
 
@@ -814,6 +816,7 @@  tcp_ao_verify_hash(const struct sock *sk, const struct sk_buff *skb,
 		NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPAOBAD);
 		atomic64_inc(&info->counters.pkt_bad);
 		atomic64_inc(&key->pkt_bad);
+		tcp_hash_fail("AO hash mismatch", family, skb, "");
 		kfree(hash_buf);
 		return SKB_DROP_REASON_TCP_AOFAILURE;
 	}
@@ -841,6 +844,8 @@  tcp_inbound_ao_hash(struct sock *sk, const struct sk_buff *skb,
 	info = rcu_dereference(tcp_sk(sk)->ao_info);
 	if (!info) {
 		NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPAOKEYNOTFOUND);
+		tcp_hash_fail("AO key not found", family, skb,
+			      "keyid: %u", aoh->keyid);
 		return SKB_DROP_REASON_TCP_AOUNEXPECTED;
 	}
 
@@ -942,6 +947,8 @@  tcp_inbound_ao_hash(struct sock *sk, const struct sk_buff *skb,
 key_not_found:
 	NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPAOKEYNOTFOUND);
 	atomic64_inc(&info->counters.key_not_found);
+	tcp_hash_fail("Requested by the peer AO key id not found",
+		      family, skb, "");
 	return SKB_DROP_REASON_TCP_AOKEYNOTFOUND;
 }