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

[Eric Dumazet]

On Mon, Oct 23, 2023 at 9:22 PM Dmitry Safonov <dima@xxxxxxxxxx> wrote:
>
> Add a helper for logging connection-detailed messages for failed TCP
> hash verification (both MD5 and AO).
>
> Co-developed-by: Francesco Ruggeri <fruggeri@xxxxxxxxxx>
> Signed-off-by: Francesco Ruggeri <fruggeri@xxxxxxxxxx>
> Co-developed-by: Salam Noureddine <noureddine@xxxxxxxxxx>
> Signed-off-by: Salam Noureddine <noureddine@xxxxxxxxxx>
> Signed-off-by: Dmitry Safonov <dima@xxxxxxxxxx>
> Acked-by: David Ahern <dsahern@xxxxxxxxxx>
> ---
>  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), \