Re: [PATCH] net: drop bad gso csum_start and offset in virtio_net_hdr

[Greg KH <gregkh@linuxfoundation.org>]

On Tue, Sep 03, 2024 at 10:42:26AM +0200, mathieu.tortuyaux@gmail.com wrote:
> From: Mathieu Tortuyaux <mtortuyaux@microsoft.com>
> 
> [ 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@kernel.org
> Fixes: e269d79 ("net: missing check virtio")
> Cc: stable@vger.kernel.org
> Signed-off-by: Willem de Bruijn <willemb@google.com>
> Link: https://patch.msgid.link/20240729201108.1615114-1-willemdebruijn.kernel@gmail.com
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> Signed-off-by: Mathieu Tortuyaux <mtortuyaux@microsoft.com>
> ---
> 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