netdev - Re: [PATCH] udp:nat:vxlan tx after nat should recsum if vxlan tx offload on

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-next>] [day] [month] [year] [list]

Date:   Sun, 2 Apr 2023 14:18:11 -0400
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Fei Cheng <chenwei.0515@...edance.com>
Cc:     dsahern@...nel.org, davem@...emloft.net,
        netfilter-devel@...r.kernel.org,
        Edward Cree <ecree@...arflare.com>,
        Network Development <netdev@...r.kernel.org>
Subject: Re: [PATCH] udp:nat:vxlan tx after nat should recsum if vxlan tx
 offload on

On Fri, Mar 31, 2023 at 10:31 PM Fei Cheng <chenwei.0515@...edance.com> wrote:
>
> From: "chenwei.0515" <chenwei.0515@...edance.com>
>
>     If vxlan-dev enable tx csum offload, there are two case of CHECKSUM_PARTIAL,
>     but udp->check donot have the both meanings.
>
>     1. vxlan-dev disable tx csum offload, udp->check is just pseudo hdr.
>     2. vxlan-dev enable tx csum offload, udp->check is pseudo hdr and
>        csum from outter l4 to innner l4.
>
>     Unfortunately if there is a nat process after vxlan tx，udp_manip_pkt just use
>     CSUM_PARTIAL to re csum PKT, which is just right on vxlan tx csum disable offload.

The issue is that for encapsulated traffic with local checksum offload,
netfilter incorrectly recomputes the outer UDP checksum as if it is an
unencapsulated CHECKSUM_PARTIAL packet, correct?

So the underlying issue is that the two types of packets are
indistinguishable after udp_set_csum:

        } else if (skb->ip_summed == CHECKSUM_PARTIAL) {
                uh->check = 0;
                uh->check = udp_v4_check(len, saddr, daddr, lco_csum(skb));
                if (uh->check == 0)
                        uh->check = CSUM_MANGLED_0;
        } else {
                skb->ip_summed = CHECKSUM_PARTIAL;
                skb->csum_start = skb_transport_header(skb) - skb->head;
                skb->csum_offset = offsetof(struct udphdr, check);
                uh->check = ~udp_v4_check(len, saddr, daddr, 0);
        }

Clearly their ip_summed will be the same.

>
>     This patch use skb->csum_local flag to identify two case, which will csum lco_csum if valid.
>
> Signed-off-by: chenwei.0515 <chenwei.0515@...edance.com>
> ---
>  include/linux/skbuff.h       | 1 +
>  net/ipv4/udp.c               | 1 +
>  net/netfilter/nf_nat_proto.c | 9 +++++++++
>  3 files changed, 11 insertions(+)
>
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index ff7ad331fb82..62996d8d0b4d 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -990,6 +990,7 @@ struct sk_buff {
>         __u8                    slow_gro:1;
>         __u8                    csum_not_inet:1;
>         __u8                    scm_io_uring:1;
> +       __u8                    csum_local:1;

sk_buff are in space constrained.

I hope we can disambiguate the packets somehow without having
to resort to a new flag.

>
>  #ifdef CONFIG_NET_SCHED
>         __u16                   tc_index;       /* traffic control index */
> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> index c605d171eb2d..86bad0bbb76e 100644
> --- a/net/ipv4/udp.c
> +++ b/net/ipv4/udp.c
> @@ -889,6 +889,7 @@ void udp_set_csum(bool nocheck, struct sk_buff *skb,
>                 uh->check = udp_v4_check(len, saddr, daddr, lco_csum(skb));
>                 if (uh->check == 0)
>                         uh->check = CSUM_MANGLED_0;
> +               skb->csum_local = 1;
>         } else {
>                 skb->ip_summed = CHECKSUM_PARTIAL;
>                 skb->csum_start = skb_transport_header(skb) - skb->head;
> diff --git a/net/netfilter/nf_nat_proto.c b/net/netfilter/nf_nat_proto.c
> index 48cc60084d28..a0261fe2d932 100644
> --- a/net/netfilter/nf_nat_proto.c
> +++ b/net/netfilter/nf_nat_proto.c
> @@ -25,6 +25,7 @@
>  #include <net/ip6_route.h>
>  #include <net/xfrm.h>
>  #include <net/ipv6.h>
> +#include <net/udp.h>
>
>  #include <net/netfilter/nf_conntrack_core.h>
>  #include <net/netfilter/nf_conntrack.h>
> @@ -75,6 +76,14 @@ static bool udp_manip_pkt(struct sk_buff *skb,
>         hdr = (struct udphdr *)(skb->data + hdroff);
>         __udp_manip_pkt(skb, iphdroff, hdr, tuple, maniptype, !!hdr->check);
>
> +       if (skb->csum_local) {
> +               hdr->check = 0;
> +               hdr->check = udp_v4_check(htons(hdr->len), tuple->src.u3.ip, tuple->dst.u3.ip,
> +                                         lco_csum(skb));
> +               if (hdr->check == 0)
> +                       hdr->check = CSUM_MANGLED_0;
> +       }
> +
>         return true;
>  }
>
> --
> 2.11.0
>