On Tue, Sep 03, 2024 at 10:42:26AM +0200, mathieu.tortuyaux@xxxxxxxxx wrote:
> From: Mathieu Tortuyaux <mtortuyaux@xxxxxxxxxxxxx>
>
> [ Upstream commit 89add40066f9ed9abe5f7f886fe5789ff7e0c50e ]
>
> 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 10154db ("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@xxxxxxxxxx
> Fixes: e269d79 ("net: missing check virtio")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Willem de Bruijn <willemb@xxxxxxxxxx>
> Link: https://patch.msgid.link/20240729201108.1615114-1-willemdebruijn.kernel@xxxxxxxxx
> Signed-off-by: Jakub Kicinski <kuba@xxxxxxxxxx>
> Signed-off-by: Mathieu Tortuyaux <mtortuyaux@xxxxxxxxxxxxx>
> ---
> Hi,
>
> This patch fixes network failures on OpenStack VMs running with Kernel
> 5.15.165.
>
> In 5.15.165, the commit "net: missing check virtio" is breaking networks
> on VMs that uses virtio in some conditions.
>
> I slightly adapted the patch to have it fitting this branch (5.15.y).
>
> Once patched and compiled it has been successfully tested on Flatcar CI
> with Kernel 5.15.165.
>
> NOTE: This patch has already been backported on other stable branches
> (like 6.6.y)
>
> Thanks,
>
> Mathieu - @tormath1
How did you test this, it breaks the build for me:
net/ipv4/tcp_offload.c: In function ‘tcp_gso_segment’:
net/ipv4/tcp_offload.c:74:5: error: this ‘if’ clause does not guard... [-Werror=misleading-indentation]
74 | if (unlikely(skb_checksum_start(skb) != skb_transport_header(skb)))
| ^~
net/ipv4/tcp_offload.c:77:9: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the ‘if’
77 | if (!pskb_may_pull(skb, thlen))
| ^~
net/ipv4/udp_offload.c: In function ‘__udp_gso_segment’:
net/ipv4/udp_offload.c:282:5: error: this ‘if’ clause does not guard... [-Werror=misleading-indentation]
282 | if (unlikely(skb_checksum_start(gso_skb) !=
| ^~
net/ipv4/udp_offload.c:286:9: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the ‘if’
286 | skb_pull(gso_skb, sizeof(*uh));
| ^~~~~~~~
cc1: all warnings being treated as errors
Now dropped, please test your patches before sending them as it throws a big
wrench in our process when things break...
thanks,
greg k-h
