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

On Fri, Jul 26, 2024 at 4:23 AM Paolo Abeni [off-list ref] wrote:

On 7/26/24 04:32, Willem de Bruijn wrot> @@ -182,6 +171,11 @@ static
inline int virtio_net_hdr_to_skb(struct sk_buff *skb,

quoted

                      if (gso_type != SKB_GSO_UDP_L4)
                              return -EINVAL;
                      break;
+             case SKB_GSO_TCPV4:
+             case SKB_GSO_TCPV6:

I think we need to add here an additional check:

                        if (!(hdr->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM))
                                return -EINVAL;

Historically this interface has been able to request
VIRTIO_NET_HDR_GSO_* without VIRTIO_NET_HDR_F_NEEDS_CSUM.

I agree that that makes little sense. Until now we have been
accommodating it, however. See the else branch if that checksum
offload flag is not set.

I would love to clamp down on this, as those packets are essentially
illegal. But we should probably leave that discussion for a separate
patch?

quoted

+                     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..9e49ffcc77071 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->csum_start != skb->transport_header))
+             goto out;

Given that for packet injected from user-space, the transport offset is
set to csum_start by skb_partial_csum_set(), do we need the above check?
If so, why don't we need another similar one for csum_offset even here?

Same point. Sadly it is not set if checksum offload is not requested.