On Fri, Jul 26, 2024 at 4:23 AM Paolo Abeni <pabeni@xxxxxxxxxx> 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.
