Jason Wang wrote:

On Mon, Sep 9, 2024 at 4:44 AM Michael S. Tsirkin [off-list ref] wrote:

quoted

On Sun, Sep 08, 2024 at 04:09:43PM -0400, Willem de Bruijn wrote:

quoted

Willem de Bruijn wrote:

quoted

Szabolcs Nagy wrote:

quoted

The 07/29/2024 16:10, Willem de Bruijn wrote:

quoted

From: Willem de Bruijn <willemb@google.com>

Tighten csum_start and csum_offset checks in virtio_net_hdr_to_skb
for GSO packets.

The function already checks that a checksum requested with
VIRTIO_NET_HDR_F_NEEDS_CSUM is in skb linear. But for GSO packets
this might not hold for segs after segmentation.

Syzkaller demonstrated to reach this warning in skb_checksum_help

        offset = skb_checksum_start_offset(skb);
        ret = -EINVAL;
        if (WARN_ON_ONCE(offset >= skb_headlen(skb)))

By injecting a TSO packet:

WARNING: CPU: 1 PID: 3539 at net/core/dev.c:3284 skb_checksum_help+0x3d0/0x5b0
 ip_do_fragment+0x209/0x1b20 net/ipv4/ip_output.c:774
 ip_finish_output_gso net/ipv4/ip_output.c:279 [inline]
 __ip_finish_output+0x2bd/0x4b0 net/ipv4/ip_output.c:301
 iptunnel_xmit+0x50c/0x930 net/ipv4/ip_tunnel_core.c:82
 ip_tunnel_xmit+0x2296/0x2c70 net/ipv4/ip_tunnel.c:813
 __gre_xmit net/ipv4/ip_gre.c:469 [inline]
 ipgre_xmit+0x759/0xa60 net/ipv4/ip_gre.c:661
 __netdev_start_xmit include/linux/netdevice.h:4850 [inline]
 netdev_start_xmit include/linux/netdevice.h:4864 [inline]
 xmit_one net/core/dev.c:3595 [inline]
 dev_hard_start_xmit+0x261/0x8c0 net/core/dev.c:3611
 __dev_queue_xmit+0x1b97/0x3c90 net/core/dev.c:4261
 packet_snd net/packet/af_packet.c:3073 [inline]

The geometry of the bad input packet at tcp_gso_segment:

[   52.003050][ T8403] skb len=12202 headroom=244 headlen=12093 tailroom=0
[   52.003050][ T8403] mac=(168,24) mac_len=24 net=(192,52) trans=244
[   52.003050][ T8403] shinfo(txflags=0 nr_frags=1 gso(size=1552 type=3 segs=0))
[   52.003050][ T8403] csum(0x60000c7 start=199 offset=1536
ip_summed=3 complete_sw=0 valid=0 level=0)

Mitigate with stricter input validation.

csum_offset: for GSO packets, deduce the correct value from gso_type.
This is already done for USO. Extend it to TSO. Let UFO be:
udp[46]_ufo_fragment ignores these fields and always computes the
checksum in software.

csum_start: finding the real offset requires parsing to the transport
header. Do not add a parser, use existing segmentation parsing. Thanks
to SKB_GSO_DODGY, that also catches bad packets that are hw offloaded.
Again test both TSO and USO. Do not test UFO for the above reason, and
do not test UDP tunnel offload.

GSO packet are almost always CHECKSUM_PARTIAL. USO packets may be
CHECKSUM_NONE since commit 10154dbded6d6 ("udp: Allow GSO transmit
from devices with no checksum offload"), but then still these fields
are initialized correctly in udp4_hwcsum/udp6_hwcsum_outgoing. So no
need to test for ip_summed == CHECKSUM_PARTIAL first.

This revises an existing fix mentioned in the Fixes tag, which broke
small packets with GSO offload, as detected by kselftests.

Link: https://syzkaller.appspot.com/bug?extid=e1db31216c789f552871
Link: https://lore.kernel.org/netdev/20240723223109.2196886-1-kuba@kernel.org (local)
Fixes: e269d79c7d35 ("net: missing check virtio")
Cc: stable@vger.kernel.org
Signed-off-by: Willem de Bruijn <willemb@google.com>

---

v1->v2
  - skb_transport_header instead of skb->transport_header (edumazet@)
  - typo: migitate -> mitigate
---

this breaks booting from nfs root on an arm64 fvp
model for me.

i see two fixup commits

commit 30b03f2a0592eee1267298298eac9dd655f55ab2
Author:     Jakub Sitnicki [off-list ref]
AuthorDate: 2024-08-08 11:56:22 +0200
Commit:     Jakub Kicinski [off-list ref]
CommitDate: 2024-08-09 21:58:08 -0700

    udp: Fall back to software USO if IPv6 extension headers are present

and

commit b128ed5ab27330deeeaf51ea8bb69f1442a96f7f
Author:     Felix Fietkau [off-list ref]
AuthorDate: 2024-08-19 17:06:21 +0200
Commit:     Jakub Kicinski [off-list ref]
CommitDate: 2024-08-21 17:15:05 -0700

    udp: fix receiving fraglist GSO packets

but they don't fix the issue for me,
at the boot console i see

...
[    3.686846] Sending DHCP requests ., OK
[    3.687302] IP-Config: Got DHCP answer from 172.20.51.254, my address is 172.20.51.1
[    3.687423] IP-Config: Complete:
[    3.687482]      device=eth0, hwaddr=ea:0d:79:71:af:cd, ipaddr=172.20.51.1, mask=255.255.255.0, gw=172.20.51.254
[    3.687631]      host=172.20.51.1, domain=, nis-domain=(none)
[    3.687719]      bootserver=172.20.51.254, rootserver=10.2.80.41, rootpath=
[    3.687771]      nameserver0=172.20.51.254, nameserver1=172.20.51.252, nameserver2=172.20.51.251
[    3.689075] clk: Disabling unused clocks
[    3.689167] PM: genpd: Disabling unused power domains
[    3.689258] ALSA device list:
[    3.689330]   No soundcards found.
[    3.716297] VFS: Mounted root (nfs4 filesystem) on device 0:24.
[    3.716843] devtmpfs: mounted
[    3.734352] Freeing unused kernel memory: 10112K
[    3.735178] Run /sbin/init as init process
[    3.743770] eth0: bad gso: type: 1, size: 1440
[    3.744186] eth0: bad gso: type: 1, size: 1440
...
[  154.610991] eth0: bad gso: type: 1, size: 1440
[  185.330941] nfs: server 10.2.80.41 not responding, still trying
...

the "bad gso" message keeps repeating and init
is not executed.

if i revert the 3 patches above on 6.11-rc6 then
init runs without "bad gso" error.

this affects testing the arm64-gcs patches on
top of 6.11-rc3 and 6.11-rc6

not sure if this is an fvp or kernel bug.

Thanks for the report, sorry that you're encountering this breakage.

Makes sense that this commit introduced it

        if (virtio_net_hdr_to_skb(skb, &hdr->hdr,
                                  virtio_is_little_endian(vi->vdev))) {
                net_warn_ratelimited("%s: bad gso: type: %u, size: %u\n",
                                     dev->name, hdr->hdr.gso_type,
                                     hdr->hdr.gso_size);
                goto frame_err;
        }

Type 1 is VIRTIO_NET_HDR_GSO_TCPV4

Most likely this application is inserting a packet with flag
VIRTIO_NET_HDR_F_NEEDS_CSUM and a wrong csum_start. Or is requesting
TSO without checksum offload at all. In which case the kernel goes out
of its way to find the right offset, but may fail.

Which nfs-client is this? I'd like to take a look at the sourcecode.

Unfortunately the kernel warning lacks a few useful pieces of data,
such as the other virtio_net_hdr fields and the packet length.

This happens on the virtio-net receive path, so the bad data is
received from the hypervisor.

quoted

From what I gather that arm64 fvp (Fixed Virtual Platforms) hypervisor

is closed source?

Disabling GRO on this device will likely work as temporary workaround.

What we can do is instead of dropping packets to correct their offset:

                case SKB_GSO_TCPV4:
                case SKB_GSO_TCPV6:
-                        if (skb->csum_offset != offsetof(struct tcphdr, check))
-                                return -EINVAL;
+                        if (skb->csum_offset != offsetof(struct tcphdr, check)) {
+                                DEBUG_NET_WARN_ON_ONCE(1);
+                                skb->csum_offset = offsetof(struct tcphdr, check);
+                        }

If the issue is in csum_offset. If the new csum_start check fails,
that won't help.

It would be helpful to see these values at this point, e.g., with
skb_dump(KERN_INFO, skb, false);

It's an iteresting question whether when VIRTIO_NET_F_GUEST_HDRLEN
is not negotiated, csum_offset can be relied upon for GRO.

Jason, WDYT?

I don't see how it connects. GUEST_HDRLEN is about transmission but
not for receiving, current Linux driver doesn't use hdrlen at all so
hardened csum_offset looks like a must.

And we have

"""
If one of the VIRTIO_NET_F_GUEST_TSO4, TSO6, UFO, USO4 or USO6 options
have been negotiated, the driver MAY use hdr_len only as a hint about
the transport header size. The driver MUST NOT rely on hdr_len to be
correct. Note: This is due to various bugs in implementations.
"""

I think I made a mistake in assuming that virtio_net_hdr_to_skb is
only used in the tx path, and that the GSO flags thus imply GSO, which
requires CHECKSUM_PARTIAL.

In virtnet_receive, this is the rx path and those flags imply GRO.
That can use CHECKSUM_UNNECESSARY, as virtnet does:

       if (flags & VIRTIO_NET_HDR_F_DATA_VALID)
                skb->ip_summed = CHECKSUM_UNNECESSARY;

So I guess VIRTIO_NET_HDR_GSO_* without VIRTIO_NET_HDR_F_DATA_VALID
would be wrong on rx.

But the new check

        if (hdr->gso_type != VIRTIO_NET_HDR_GSO_NONE) {

                [...]

                case SKB_GSO_TCPV4:
                case SKB_GSO_TCPV6:
                        if (skb->csum_offset != offsetof(struct tcphdr, check))
                                return -EINVAL;

should be limited to callers of virtio_net_hdr_to_skb on the tx/GSO path.

Looking what the cleanest/minimal patch is to accomplish that.