netdev - Re: [PATCH net 4/4] udp: prevent local UDP tunnel packets from being GROed

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: <65f5be1e42018_6ef3e29485@willemb.c.googlers.com.notmuch>
Date: Sat, 16 Mar 2024 11:43:26 -0400
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Antoine Tenart <atenart@...nel.org>, 
 davem@...emloft.net, 
 kuba@...nel.org, 
 pabeni@...hat.com, 
 edumazet@...gle.com
Cc: Antoine Tenart <atenart@...nel.org>, 
 steffen.klassert@...unet.com, 
 netdev@...r.kernel.org
Subject: Re: [PATCH net 4/4] udp: prevent local UDP tunnel packets from being
 GROed

Antoine Tenart wrote:
> GRO has a fundamental issue with UDP tunnel packets as it can't detect
> those in a foolproof way and GRO could happen before they reach the
> tunnel endpoint. Previous commits have fixed issues when UDP tunnel
> packets come from a remote host, but if those packets are issued locally
> they could run into checksum issues.
> 
> If the inner packet has a partial checksum the information will be lost
> in the GRO logic, either in udp4/6_gro_complete or in
> udp_gro_complete_segment and packets will have an invalid checksum when
> leaving the host.

Before the previous patch, the tunnel code would convert ip_summed to
CHECKSUM_UNNECESSARY. After that patch CHECKSUM_PARTIAL is preserved.
Are the tunneled packets still corrupted once forwarded to the egress
path? In principle CHECKSUM partial with tunnel and GSO should work,
whether built as such or arrived at through GRO.

> Prevent local UDP tunnel packets from ever being GROed at the outer UDP
> level.
> 
> Due to skb->encapsulation being wrongly used in some drivers this is
> actually only preventing UDP tunnel packets with a partial checksum to
> be GROed (see iptunnel_handle_offloads) but those were also the packets
> triggering issues so in practice this should be sufficient.

Because of this in iptunnel_handle_offloads: 

        if (skb->ip_summed != CHECKSUM_PARTIAL) {
                skb->ip_summed = CHECKSUM_NONE;
                /* We clear encapsulation here to prevent badly-written
                 * drivers potentially deciding to offload an inner checksum
                 * if we set CHECKSUM_PARTIAL on the outer header.
                 * This should go away when the drivers are all fixed.
                 */
                skb->encapsulation = 0;
        }
 
> Fixes: 9fd1ff5d2ac7 ("udp: Support UDP fraglist GRO/GSO.")
> Fixes: 36707061d6ba ("udp: allow forwarding of plain (non-fraglisted) UDP GRO packets")
> Suggested-by: Paolo Abeni <pabeni@...hat.com>
> Signed-off-by: Antoine Tenart <atenart@...nel.org>

Choosing not to coalesce certain edge case packets that cause problems
is safe and reasonable.

Reviewed-by: Willem de Bruijn <willemb@...gle.com>