diff mbox series

[net,v2] net: drop bad gso csum_start and offset in virtio_net_hdr

Message ID 20240729201108.1615114-1-willemdebruijn.kernel@gmail.com (mailing list archive)
State Accepted
Commit 89add40066f9ed9abe5f7f886fe5789ff7e0c50e
Delegated to: Netdev Maintainers
Headers show
Series [net,v2] net: drop bad gso csum_start and offset in virtio_net_hdr | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net, async
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag present in non-next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 42 this patch: 42
netdev/build_tools success Errors and warnings before: 10 this patch: 10
netdev/cc_maintainers warning 4 maintainers not CCed: virtualization@lists.linux.dev xuanzhuo@linux.alibaba.com eperezma@redhat.com dsahern@kernel.org
netdev/build_clang success Errors and warnings before: 43 this patch: 43
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 61 this patch: 54
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 53 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest warning net-next-2024-07-30--03-00 (tests: 675)

Commit Message

Willem de Bruijn July 29, 2024, 8:10 p.m. UTC 
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
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
---
 include/linux/virtio_net.h | 16 +++++-----------
 net/ipv4/tcp_offload.c     |  3 +++
 net/ipv4/udp_offload.c     |  4 ++++
 3 files changed, 12 insertions(+), 11 deletions(-)

Comments

patchwork-bot+netdevbpf@kernel.org July 31, 2024, 2 a.m. UTC | #1 
Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Mon, 29 Jul 2024 16:10:12 -0400 you wrote:
> 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.
> 
> [...]

Here is the summary with links:
  - [net,v2] net: drop bad gso csum_start and offset in virtio_net_hdr
    https://git.kernel.org/netdev/net/c/89add40066f9

You are awesome, thank you!
Szabolcs Nagy Sept. 6, 2024, 2:35 p.m. UTC | #2 
The 07/29/2024 16:10, Willem de Bruijn wrote:
> 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
> 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 <jakub@cloudflare.com>
AuthorDate: 2024-08-08 11:56:22 +0200
Commit:     Jakub Kicinski <kuba@kernel.org>
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 <nbd@nbd.name>
AuthorDate: 2024-08-19 17:06:21 +0200
Commit:     Jakub Kicinski <kuba@kernel.org>
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.
Willem de Bruijn Sept. 6, 2024, 3:52 p.m. UTC | #3 
Szabolcs Nagy wrote:
> The 07/29/2024 16:10, Willem de Bruijn wrote:
> > 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
> > 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 <jakub@cloudflare.com>
> AuthorDate: 2024-08-08 11:56:22 +0200
> Commit:     Jakub Kicinski <kuba@kernel.org>
> 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 <nbd@nbd.name>
> AuthorDate: 2024-08-19 17:06:21 +0200
> Commit:     Jakub Kicinski <kuba@kernel.org>
> 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.
Willem de Bruijn Sept. 8, 2024, 8:09 p.m. UTC | #4 
Willem de Bruijn wrote:
> Szabolcs Nagy wrote:
> > The 07/29/2024 16:10, Willem de Bruijn wrote:
> > > 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
> > > 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 <jakub@cloudflare.com>
> > AuthorDate: 2024-08-08 11:56:22 +0200
> > Commit:     Jakub Kicinski <kuba@kernel.org>
> > 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 <nbd@nbd.name>
> > AuthorDate: 2024-08-19 17:06:21 +0200
> > Commit:     Jakub Kicinski <kuba@kernel.org>
> > 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.

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);
Michael S. Tsirkin Sept. 8, 2024, 8:43 p.m. UTC | #5 
On Sun, Sep 08, 2024 at 04:09:43PM -0400, Willem de Bruijn wrote:
> Willem de Bruijn wrote:
> > Szabolcs Nagy wrote:
> > > The 07/29/2024 16:10, Willem de Bruijn wrote:
> > > > 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
> > > > 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 <jakub@cloudflare.com>
> > > AuthorDate: 2024-08-08 11:56:22 +0200
> > > Commit:     Jakub Kicinski <kuba@kernel.org>
> > > 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 <nbd@nbd.name>
> > > AuthorDate: 2024-08-19 17:06:21 +0200
> > > Commit:     Jakub Kicinski <kuba@kernel.org>
> > > 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.
> 
> >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?
Jason Wang Sept. 9, 2024, 2:33 a.m. UTC | #6 
On Mon, Sep 9, 2024 at 4:44 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Sun, Sep 08, 2024 at 04:09:43PM -0400, Willem de Bruijn wrote:
> > Willem de Bruijn wrote:
> > > Szabolcs Nagy wrote:
> > > > The 07/29/2024 16:10, Willem de Bruijn wrote:
> > > > > 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
> > > > > 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 <jakub@cloudflare.com>
> > > > AuthorDate: 2024-08-08 11:56:22 +0200
> > > > Commit:     Jakub Kicinski <kuba@kernel.org>
> > > > 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 <nbd@nbd.name>
> > > > AuthorDate: 2024-08-19 17:06:21 +0200
> > > > Commit:     Jakub Kicinski <kuba@kernel.org>
> > > > 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.
> >
> > >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.
"""

Thanks

>
> --
> MST
>
Willem de Bruijn Sept. 9, 2024, 3:02 a.m. UTC | #7 
Jason Wang wrote:
> On Mon, Sep 9, 2024 at 4:44 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Sun, Sep 08, 2024 at 04:09:43PM -0400, Willem de Bruijn wrote:
> > > Willem de Bruijn wrote:
> > > > Szabolcs Nagy wrote:
> > > > > The 07/29/2024 16:10, Willem de Bruijn wrote:
> > > > > > 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
> > > > > > 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 <jakub@cloudflare.com>
> > > > > AuthorDate: 2024-08-08 11:56:22 +0200
> > > > > Commit:     Jakub Kicinski <kuba@kernel.org>
> > > > > 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 <nbd@nbd.name>
> > > > > AuthorDate: 2024-08-19 17:06:21 +0200
> > > > > Commit:     Jakub Kicinski <kuba@kernel.org>
> > > > > 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.
> > >
> > > >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.
Jason Wang Sept. 9, 2024, 3:24 a.m. UTC | #8 
On Mon, Sep 9, 2024 at 11:02 AM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> Jason Wang wrote:
> > On Mon, Sep 9, 2024 at 4:44 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Sun, Sep 08, 2024 at 04:09:43PM -0400, Willem de Bruijn wrote:
> > > > Willem de Bruijn wrote:
> > > > > Szabolcs Nagy wrote:
> > > > > > The 07/29/2024 16:10, Willem de Bruijn wrote:
> > > > > > > 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
> > > > > > > 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 <jakub@cloudflare.com>
> > > > > > AuthorDate: 2024-08-08 11:56:22 +0200
> > > > > > Commit:     Jakub Kicinski <kuba@kernel.org>
> > > > > > 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 <nbd@nbd.name>
> > > > > > AuthorDate: 2024-08-19 17:06:21 +0200
> > > > > > Commit:     Jakub Kicinski <kuba@kernel.org>
> > > > > > 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.
> > > >
> > > > >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.
>

virtio_net_hdr_to_skb() translates virtio-net header to skb metadata,
so it's RX. For TX the helper should be virtio_net_hdr_from_skb()
which translates skb metadata to virtio hdr.

Thanks
Willem de Bruijn Sept. 9, 2024, 3:39 a.m. UTC | #9 
Jason Wang wrote:
> On Mon, Sep 9, 2024 at 11:02 AM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> >
> > Jason Wang wrote:
> > > On Mon, Sep 9, 2024 at 4:44 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > >
> > > > On Sun, Sep 08, 2024 at 04:09:43PM -0400, Willem de Bruijn wrote:
> > > > > Willem de Bruijn wrote:
> > > > > > Szabolcs Nagy wrote:
> > > > > > > The 07/29/2024 16:10, Willem de Bruijn wrote:
> > > > > > > > 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
> > > > > > > > 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 <jakub@cloudflare.com>
> > > > > > > AuthorDate: 2024-08-08 11:56:22 +0200
> > > > > > > Commit:     Jakub Kicinski <kuba@kernel.org>
> > > > > > > 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 <nbd@nbd.name>
> > > > > > > AuthorDate: 2024-08-19 17:06:21 +0200
> > > > > > > Commit:     Jakub Kicinski <kuba@kernel.org>
> > > > > > > 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.
> > > > >
> > > > > >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.
> >
> 
> virtio_net_hdr_to_skb() translates virtio-net header to skb metadata,
> so it's RX. For TX the helper should be virtio_net_hdr_from_skb()
> which translates skb metadata to virtio hdr.

virtio_net_hdr_to_skb is used by PF_PACKET, tun and tap when injecting
a packet into the egress path.
Jason Wang Sept. 9, 2024, 4:14 a.m. UTC | #10 
On Mon, Sep 9, 2024 at 11:40 AM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> Jason Wang wrote:
> > On Mon, Sep 9, 2024 at 11:02 AM Willem de Bruijn
> > <willemdebruijn.kernel@gmail.com> wrote:
> > >
> > > Jason Wang wrote:
> > > > On Mon, Sep 9, 2024 at 4:44 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > >
> > > > > On Sun, Sep 08, 2024 at 04:09:43PM -0400, Willem de Bruijn wrote:
> > > > > > Willem de Bruijn wrote:
> > > > > > > Szabolcs Nagy wrote:
> > > > > > > > The 07/29/2024 16:10, Willem de Bruijn wrote:
> > > > > > > > > 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
> > > > > > > > > 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 <jakub@cloudflare.com>
> > > > > > > > AuthorDate: 2024-08-08 11:56:22 +0200
> > > > > > > > Commit:     Jakub Kicinski <kuba@kernel.org>
> > > > > > > > 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 <nbd@nbd.name>
> > > > > > > > AuthorDate: 2024-08-19 17:06:21 +0200
> > > > > > > > Commit:     Jakub Kicinski <kuba@kernel.org>
> > > > > > > > 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.
> > > > > >
> > > > > > >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.
> > >
> >
> > virtio_net_hdr_to_skb() translates virtio-net header to skb metadata,
> > so it's RX. For TX the helper should be virtio_net_hdr_from_skb()
> > which translates skb metadata to virtio hdr.
>
> virtio_net_hdr_to_skb is used by PF_PACKET, tun and tap

Exactly.

> when injecting a packet into the egress path.

For tuntap it's still the RX path. For PF_PACEKT and macvtap, it's the tx.

Maybe a new parameter to virtio_net_hdr_to_skb()?

Thanks

>
Szabolcs Nagy Sept. 9, 2024, 9:45 a.m. UTC | #11 
* Willem de Bruijn <willemdebruijn.kernel@gmail.com> [2024-09-08 16:09:43 -0400]:
> Willem de Bruijn wrote:
> > Szabolcs Nagy wrote:
> > > The 07/29/2024 16:10, Willem de Bruijn wrote:
> > > > 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
> > > > 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 <jakub@cloudflare.com>
> > > AuthorDate: 2024-08-08 11:56:22 +0200
> > > Commit:     Jakub Kicinski <kuba@kernel.org>
> > > 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 <nbd@nbd.name>
> > > AuthorDate: 2024-08-19 17:06:21 +0200
> > > Commit:     Jakub Kicinski <kuba@kernel.org>
> > > 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.
> 
> >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);

thanks for the quick response.

i sent this issue on my last day at the company so
i won't be able to help with this any more, sorry.

fvp is closed source but has freely available binaries
for x86_64 glibc based linux systems (behind registration
and license agreements) so in principle the issue can be
reproduced outside of arm but using fvp is not obvious.

hopefully somebody at arm can pick it up or at least
report this thread to the fvp team internally.
Yury Khrustalev Sept. 9, 2024, 10:13 a.m. UTC | #12 
On Fri, Sep 06, 2024 at 11:52:34AM -0400, Willem de Bruijn wrote:
> Szabolcs Nagy wrote:
> > The 07/29/2024 16:10, Willem de Bruijn wrote:
> > > 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
> > > 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 <jakub@cloudflare.com>
> > AuthorDate: 2024-08-08 11:56:22 +0200
> > Commit:     Jakub Kicinski <kuba@kernel.org>
> > 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 <nbd@nbd.name>
> > AuthorDate: 2024-08-19 17:06:21 +0200
> > Commit:     Jakub Kicinski <kuba@kernel.org>
> > 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.

fwiw, this is not nfs specific: if you boot a system with this kernel,
them most network operations will fail (e.g. scp-ing something on this
system or trying to curl or wget something from within) with the above
mentioned error eth0: bad gso: type: 1, size: 1440

Kind regards,
Yury


IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
Mark Brown Sept. 9, 2024, 11:14 a.m. UTC | #13 
On Mon, Sep 09, 2024 at 11:45:27AM +0200, Szabolcs Nagy wrote:

> fvp is closed source but has freely available binaries
> for x86_64 glibc based linux systems (behind registration
> and license agreements) so in principle the issue can be
> reproduced outside of arm but using fvp is not obvious.

> hopefully somebody at arm can pick it up or at least
> report this thread to the fvp team internally.

FWIW there's a tool called shrinkwrap which makes it quite a lot easier
to get going:

   https://gitlab.arm.com/tooling/shrinkwrap

though since the models are very flexibile valid configurations that
people see issues with aren't always covered by shrinkwrap.
Sudeep Holla Sept. 9, 2024, 3:14 p.m. UTC | #14 
On Mon, Sep 09, 2024 at 12:14:28PM +0100, Mark Brown wrote:
> On Mon, Sep 09, 2024 at 11:45:27AM +0200, Szabolcs Nagy wrote:
> 
> > fvp is closed source but has freely available binaries
> > for x86_64 glibc based linux systems (behind registration
> > and license agreements) so in principle the issue can be
> > reproduced outside of arm but using fvp is not obvious.
> 
> > hopefully somebody at arm can pick it up or at least
> > report this thread to the fvp team internally.
> 
> FWIW there's a tool called shrinkwrap which makes it quite a lot easier
> to get going:
> 
>    https://gitlab.arm.com/tooling/shrinkwrap
> 
> though since the models are very flexibile valid configurations that
> people see issues with aren't always covered by shrinkwrap.

It is fairly trivial to change the default config and use virtio-net to
reproduce this issue. If anyone tries the above tool, they can apply below
diff and should be able to reproduce the issue.

--
Regards,
Sudeep

diff --git i/config/FVP_Base_RevC-2xAEMvA-base.yaml w/config/FVP_Base_RevC-2xAEMvA-base.yaml
index 86d8cf9cb0f8..9951c5a948bb 100644
--- i/config/FVP_Base_RevC-2xAEMvA-base.yaml
+++ w/config/FVP_Base_RevC-2xAEMvA-base.yaml
@@ -39,8 +39,10 @@ description: >-
     # Networking. By default use user-space networking, mapping port 22 in the
     # FVP to a user-specified port on the host (see rtvar:LOCAL_NET_PORT). This
     # enables ssh.
-    -C bp.smsc_91c111.enabled: 1
-    -C bp.hostbridge.userNetworking: 1
+    -C bp.smsc_91c111.enabled: 0
+    -C bp.hostbridge.userNetworking: 0
+    -C bp.virtio_net.enabled: 1
+    -C bp.virtio_net.hostbridge.userNetworking: 1
     -C bp.hostbridge.userNetPorts: ${rtvar:LOCAL_NET_PORT}=22

     # FVP Performance tweaks.
Willem de Bruijn Sept. 9, 2024, 3:20 p.m. UTC | #15 
> > > > 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.
> > > >
> > >
> > > virtio_net_hdr_to_skb() translates virtio-net header to skb metadata,
> > > so it's RX. For TX the helper should be virtio_net_hdr_from_skb()
> > > which translates skb metadata to virtio hdr.
> >
> > virtio_net_hdr_to_skb is used by PF_PACKET, tun and tap
> 
> Exactly.
> 
> > when injecting a packet into the egress path.
> 
> For tuntap it's still the RX path. For PF_PACEKT and macvtap, it's the tx.
> 
> Maybe a new parameter to virtio_net_hdr_to_skb()?

This is the most straightforward approach. But requires changse to all
callers, in a patch targeting all the stable branches.

I'd prefer if we can detect ingress vs egress directly.

Based on ip_summed, pkt_type, is_skb_wmem or so. But so far have not
found a suitable condition.

I noticed something else: as you point out TUN is ingress. Unlike
virtnet_receive, it does not set ip_summed to CHECKSUM_UNNECESSARY if
VIRTIO_NET_HDR_F_DATA_VALID. It probably should. GRO expects packets
to have had their integrity verified. CHECKSUM_NONE on ingress is not
correct for GRO.

And also related: no GRO should be generated by a device unless
VIRTIO_NET_HDR_F_DATA_VALID is also passed? I have to check the spec
if it says anything about this.
Willem de Bruijn Sept. 9, 2024, 3:21 p.m. UTC | #16 
Mark Brown wrote:
> On Mon, Sep 09, 2024 at 11:45:27AM +0200, Szabolcs Nagy wrote:
> 
> > fvp is closed source but has freely available binaries
> > for x86_64 glibc based linux systems (behind registration
> > and license agreements) so in principle the issue can be
> > reproduced outside of arm but using fvp is not obvious.
> 
> > hopefully somebody at arm can pick it up or at least
> > report this thread to the fvp team internally.
> 
> FWIW there's a tool called shrinkwrap which makes it quite a lot easier
> to get going:
> 
>    https://gitlab.arm.com/tooling/shrinkwrap
> 
> though since the models are very flexibile valid configurations that
> people see issues with aren't always covered by shrinkwrap.

Thanks both.

From what I gather this affects any device passing packets with GRO,
so should not be limited to fvp even.
Willem de Bruijn Sept. 9, 2024, 9:09 p.m. UTC | #17 
Willem de Bruijn wrote:
> > > > > 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.
> > > > >
> > > >
> > > > virtio_net_hdr_to_skb() translates virtio-net header to skb metadata,
> > > > so it's RX. For TX the helper should be virtio_net_hdr_from_skb()
> > > > which translates skb metadata to virtio hdr.
> > >
> > > virtio_net_hdr_to_skb is used by PF_PACKET, tun and tap
> > 
> > Exactly.
> > 
> > > when injecting a packet into the egress path.
> > 
> > For tuntap it's still the RX path. For PF_PACEKT and macvtap, it's the tx.
> > 
> > Maybe a new parameter to virtio_net_hdr_to_skb()?
> 
> This is the most straightforward approach. But requires changse to all
> callers, in a patch targeting all the stable branches.
> 
> I'd prefer if we can detect ingress vs egress directly.

Not doing this, because both on ingress and egress the allowed
ip_summed types are more relaxed than I imagined.

Let's just make the check more narrow to avoid such false positives.

GRO indeed allows CHECKSUM_NONE.

But TSO also accepts packets that are not CHECKSUM_PARTIAL, and will
fix up csum_start/csum_off. In tcp4_gso_segment:

        if (unlikely(skb->ip_summed != CHECKSUM_PARTIAL)) {
                const struct iphdr *iph = ip_hdr(skb);
                struct tcphdr *th = tcp_hdr(skb);

                /* Set up checksum pseudo header, usually expect stack to
                 * have done this already.
                 */

                th->check = 0;
                skb->ip_summed = CHECKSUM_PARTIAL;
                __tcp_v4_send_check(skb, iph->saddr, iph->daddr);
        }

With __tcp_v4_send_check:

	void __tcp_v4_send_check(struct sk_buff *skb, __be32 saddr, __be32 daddr)
	{
		struct tcphdr *th = tcp_hdr(skb);

		th->check = ~tcp_v4_check(skb->len, saddr, daddr, 0);
		skb->csum_start = skb_transport_header(skb) - skb->head;
		skb->csum_offset = offsetof(struct tcphdr, check);
	}    

That means that we can relax the check on input from userspace to
bad CHECKSUM_PARTIAL input:

@@ -173,7 +173,8 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
                        break;
                case SKB_GSO_TCPV4:
                case SKB_GSO_TCPV6:
-                       if (skb->csum_offset != offsetof(struct tcphdr, check))
+                       if (skb->ip_summed == CHECKSUM_PARTIAL &&
+                           skb->csum_offset != offsetof(struct tcphdr, check))
                                return -EINVAL;

I've verified that this test still catches the bad packet from the
syzkaller report in the Link in the commit.




> Based on ip_summed, pkt_type, is_skb_wmem or so. But so far have not
> found a suitable condition.
> 
> I noticed something else: as you point out TUN is ingress. Unlike
> virtnet_receive, it does not set ip_summed to CHECKSUM_UNNECESSARY if
> VIRTIO_NET_HDR_F_DATA_VALID. It probably should. GRO expects packets
> to have had their integrity verified. CHECKSUM_NONE on ingress is not
> correct for GRO.
> 
> And also related: no GRO should be generated by a device unless
> VIRTIO_NET_HDR_F_DATA_VALID is also passed? I have to check the spec
> if it says anything about this.
Willem de Bruijn Sept. 10, 2024, 1:12 a.m. UTC | #18 
Willem de Bruijn wrote:
> Willem de Bruijn wrote:
> > > > > > 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.
> > > > > >
> > > > >
> > > > > virtio_net_hdr_to_skb() translates virtio-net header to skb metadata,
> > > > > so it's RX. For TX the helper should be virtio_net_hdr_from_skb()
> > > > > which translates skb metadata to virtio hdr.
> > > >
> > > > virtio_net_hdr_to_skb is used by PF_PACKET, tun and tap
> > > 
> > > Exactly.
> > > 
> > > > when injecting a packet into the egress path.
> > > 
> > > For tuntap it's still the RX path. For PF_PACEKT and macvtap, it's the tx.
> > > 
> > > Maybe a new parameter to virtio_net_hdr_to_skb()?
> > 
> > This is the most straightforward approach. But requires changse to all
> > callers, in a patch targeting all the stable branches.
> > 
> > I'd prefer if we can detect ingress vs egress directly.
> 
> Not doing this, because both on ingress and egress the allowed
> ip_summed types are more relaxed than I imagined.
> 
> Let's just make the check more narrow to avoid such false positives.
> 
> GRO indeed allows CHECKSUM_NONE.
> 
> But TSO also accepts packets that are not CHECKSUM_PARTIAL, and will
> fix up csum_start/csum_off. In tcp4_gso_segment:
> 
>         if (unlikely(skb->ip_summed != CHECKSUM_PARTIAL)) {
>                 const struct iphdr *iph = ip_hdr(skb);
>                 struct tcphdr *th = tcp_hdr(skb);
> 
>                 /* Set up checksum pseudo header, usually expect stack to
>                  * have done this already.
>                  */
> 
>                 th->check = 0;
>                 skb->ip_summed = CHECKSUM_PARTIAL;
>                 __tcp_v4_send_check(skb, iph->saddr, iph->daddr);
>         }
> 
> With __tcp_v4_send_check:
> 
> 	void __tcp_v4_send_check(struct sk_buff *skb, __be32 saddr, __be32 daddr)
> 	{
> 		struct tcphdr *th = tcp_hdr(skb);
> 
> 		th->check = ~tcp_v4_check(skb->len, saddr, daddr, 0);
> 		skb->csum_start = skb_transport_header(skb) - skb->head;
> 		skb->csum_offset = offsetof(struct tcphdr, check);
> 	}    
> 
> That means that we can relax the check on input from userspace to
> bad CHECKSUM_PARTIAL input:
> 
> @@ -173,7 +173,8 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
>                         break;
>                 case SKB_GSO_TCPV4:
>                 case SKB_GSO_TCPV6:
> -                       if (skb->csum_offset != offsetof(struct tcphdr, check))
> +                       if (skb->ip_summed == CHECKSUM_PARTIAL &&
> +                           skb->csum_offset != offsetof(struct tcphdr, check))
>                                 return -EINVAL;
> 
> I've verified that this test still catches the bad packet from the
> syzkaller report in the Link in the commit.

Sent: https://lore.kernel.org/netdev/20240910004033.530313-1-willemdebruijn.kernel@gmail.com/T/#u
 
> > Based on ip_summed, pkt_type, is_skb_wmem or so. But so far have not
> > found a suitable condition.
> > 
> > I noticed something else: as you point out TUN is ingress. Unlike
> > virtnet_receive, it does not set ip_summed to CHECKSUM_UNNECESSARY if
> > VIRTIO_NET_HDR_F_DATA_VALID. It probably should. GRO expects packets
> > to have had their integrity verified. CHECKSUM_NONE on ingress is not
> > correct for GRO.

Actually CHECKSUM_NONE is allowed. It just triggers software checksum
validation.

Tun by default does not use GRO, only if enabling IFF_NAPI.

If a packet arrives at GRO with CHECKSUM_PARTIAL, then its checksum is
assumed valid, per __skb_gro_checksum_validate_needed. So that would
be one way for tun users today to get efficient GRO.

> > And also related: no GRO should be generated by a device unless
> > VIRTIO_NET_HDR_F_DATA_VALID is also passed? I have to check the spec
> > if it says anything about this.

Given that GRO handles !CHECKSUM_UNNECESSARY, probably no need to
require VIRTIO_NET_HDR_F_DATA_VALID with virtio GRO either.
diff mbox series

Patch

diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
index d1d7825318c32..6c395a2600e8d 100644
--- a/include/linux/virtio_net.h
+++ b/include/linux/virtio_net.h
@@ -56,7 +56,6 @@  static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
 	unsigned int thlen = 0;
 	unsigned int p_off = 0;
 	unsigned int ip_proto;
-	u64 ret, remainder, gso_size;
 
 	if (hdr->gso_type != VIRTIO_NET_HDR_GSO_NONE) {
 		switch (hdr->gso_type & ~VIRTIO_NET_HDR_GSO_ECN) {
@@ -99,16 +98,6 @@  static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
 		u32 off = __virtio16_to_cpu(little_endian, hdr->csum_offset);
 		u32 needed = start + max_t(u32, thlen, off + sizeof(__sum16));
 
-		if (hdr->gso_size) {
-			gso_size = __virtio16_to_cpu(little_endian, hdr->gso_size);
-			ret = div64_u64_rem(skb->len, gso_size, &remainder);
-			if (!(ret && (hdr->gso_size > needed) &&
-						((remainder > needed) || (remainder == 0)))) {
-				return -EINVAL;
-			}
-			skb_shinfo(skb)->tx_flags |= SKBFL_SHARED_FRAG;
-		}
-
 		if (!pskb_may_pull(skb, needed))
 			return -EINVAL;
 
@@ -182,6 +171,11 @@  static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
 			if (gso_type != SKB_GSO_UDP_L4)
 				return -EINVAL;
 			break;
+		case SKB_GSO_TCPV4:
+		case SKB_GSO_TCPV6:
+			if (skb->csum_offset != offsetof(struct tcphdr, check))
+				return -EINVAL;
+			break;
 		}
 
 		/* Kernel has a special handling for GSO_BY_FRAGS. */
diff --git a/net/ipv4/tcp_offload.c b/net/ipv4/tcp_offload.c
index 4b791e74529e1..e4ad3311e1489 100644
--- a/net/ipv4/tcp_offload.c
+++ b/net/ipv4/tcp_offload.c
@@ -140,6 +140,9 @@  struct sk_buff *tcp_gso_segment(struct sk_buff *skb,
 	if (thlen < sizeof(*th))
 		goto out;
 
+	if (unlikely(skb_checksum_start(skb) != skb_transport_header(skb)))
+		goto out;
+
 	if (!pskb_may_pull(skb, thlen))
 		goto out;
 
diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
index aa2e0a28ca613..bc8a9da750fed 100644
--- a/net/ipv4/udp_offload.c
+++ b/net/ipv4/udp_offload.c
@@ -278,6 +278,10 @@  struct sk_buff *__udp_gso_segment(struct sk_buff *gso_skb,
 	if (gso_skb->len <= sizeof(*uh) + mss)
 		return ERR_PTR(-EINVAL);
 
+	if (unlikely(skb_checksum_start(gso_skb) !=
+		     skb_transport_header(gso_skb)))
+		return ERR_PTR(-EINVAL);
+
 	if (skb_gso_ok(gso_skb, features | NETIF_F_GSO_ROBUST)) {
 		/* Packet is from an untrusted source, reset gso_segs. */
 		skb_shinfo(gso_skb)->gso_segs = DIV_ROUND_UP(gso_skb->len - sizeof(*uh),