netdev - Re: Broken segmentation on UDP_GSO / VIRTIO_NET_HDR_GSO_UDP

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: <CAO3-PboqjhNDcCTicLPXawvwmvrC-Wj04Q72v0tJCME-cX4P8Q@mail.gmail.com>
Date: Wed, 12 Jul 2023 09:58:11 -0500
From: Yan Zhai <yan@...udflare.com>
To: Marek Majkowski <marek@...udflare.com>
Cc: network dev <netdev@...r.kernel.org>, kernel-team <kernel-team@...udflare.com>, 
	Andrew Melnychenko <andrew@...nix.com>
Subject: Re: Broken segmentation on UDP_GSO / VIRTIO_NET_HDR_GSO_UDP_L4
 forwarding path

On Wed, Jul 12, 2023 at 9:00 AM Marek Majkowski <marek@...udflare.com> wrote:
>
> Dear netdev,
>
> I encountered a puzzling problem, please help.
>
> Rootless repro:
>    https://gist.github.com/majek/5e8fd12e7a1663cea63877920fe86f18
>
> To run:
>
> ```
> $ unshare -Urn python3 udp-gro-forwarding-bug.py
> tap0  In  IP 10.0.0.2.55892 > 1.1.1.1.5021: UDP, length 4000
> lo    P   IP 10.0.0.2.55892 > 1.1.1.1.5021: UDP, bad length 4000 > 1392
> lo    P   IP 10.0.0.2.43690 > 1.1.1.1.43690: UDP, bad length 43682 > 1392
> lo    P   IP 10.0.0.2.43690 > 1.1.1.1.43690: UDP, bad length 43682 > 1200
> '''
>
> The code is really quite simple. First I create and open a tap device.
> Then I send a large (>MTU) packet with vnethdr over tap0. The
> gso_type=GSO_UDP_L4, and gso_size=1400. I expect the packet to egress
> from tap0, be forwarded somewhere, where it will eventually be
> segmented by software or hardware.
>
> The egress tap0 packet looks perfectly fine:
>
> tap0  In  IP 10.0.0.2.55892 > 1.1.1.1.5021: UDP, length 4000
>
> To simplify routing I'm doing 'tc mirred' aka `bpf_redirect()` magic,
> where I move egress tap0 packets to ingress lo, like this:
>
> > tc filter add dev tap0 ingress protocol ip u32 match ip src 10.0.0.2 action mirred egress redirect dev lo
>
> On ingress lo I see something really weird:
>
> lo    P   IP 10.0.0.2.55892 > 1.1.1.1.5021: UDP, bad length 4000 > 1392
> lo    P   IP 10.0.0.2.43690 > 1.1.1.1.43690: UDP, bad length 43682 > 1392
> lo    P   IP 10.0.0.2.43690 > 1.1.1.1.43690: UDP, bad length 43682 > 1200
>
> This looks like IPv4 fragments without the IP fragmentation bits set.
>
> I think there are two independent problems here:
>
> (1) The packet is *fragmented* and that is plain wrong here. I'm
> asking for USO not UFO in vnethdr.
>

To add some context our virtio header in hex format (12 bytes) is
01052a007805220006000000.

Some digging shows that the issue seems to come from this patch:
https://lore.kernel.org/netdev/20220907125048.396126-2-andrew@daynix.com/
At this point, skb_shared_info->gso_type is SKB_GSO_UDP_L4 |
SKB_GSO_DODGY, here the DODGY bit is set inside tun_get_user. So the
skb_gso_ok check will return true here, then the skb will fall to the
fragment code. Simple tracing confirms that __udp_gso_segment is never
called in this scenario.

So the question is really how to handle the DODGY bit. IMHO it is not
right to fall to the fragment path when the actual packet request is
segmentation. Will it be sufficient to just recompute the gso_segs
here and return the head skb instead?  The only missing bit in the
device feature is the DODGY bit here.

> (2) The fragmentation attempt is broken, the IPv4 fragmentation bits are clear.
>
> Please advise. I would assume transmitting UDP_GSO packets off tap is
> a typical thing to do.
>
> I was able to repro this on a 6.4 kernel.
>
> For a moment I thought it's a `ethtool -K X rx-udp-gro-forwarding on`
> bug, but I'm not sure anymore.
>
> Marek



-- 

Yan