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 pabeni@redhat.com 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,

...

                  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?

...

•                 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.