On Tue, Oct 13, 2026 at 5:06 AM Michael S. Tsirkin <[email protected]> wrote:
>
> On Tue, Oct 13, 2026 at 06:58:41AM -0400, Michael S. Tsirkin wrote:
> > A virtio net header can place CHECKSUM_PARTIAL at any in-packet
> > offset. If that state is retained, later checksum completion can
> > modify the packet after protocol processing has made decisions from it.
> >
> > Let's fix that: let's validate csum_start and csum_offset and only keep
> > CHECKSUM_PARTIAL when csum_start and csum_offset match a parsed TCP or
> > UDP header, while handling VLAN headers, IPv4 options and IPv6 extension
> > headers. Complete other non-GSO checksums immediately, before any
> > protocol processing runs.
> >
> > We can't easily "complete the checksum" for GSO we don't recognize - GSO
> > assumes knowledge of the protocol. We could segment ignoring the
> > checksum then do whatever csum_start and csum_offset say, but that
> > seems like an overkill for something that should happen.
> >
> > UDP tunnel headers get the same handling on the inner header.
> >
> > Packets without VIRTIO_NET_HDR_F_NEEDS_CSUM are unaffected.
> >
> > Suggested-by: Willem de Bruijn <[email protected]>
> > Signed-off-by: Michael S. Tsirkin <[email protected]>
> > Assisted-by: LLM
>
>
> I forgot reported-by tags:
>
> Reported-by: Paulos Yibelo <[email protected]>
> Reported-by: Chris Arges <[email protected]>
>
>
> > ---
> > include/linux/virtio_net.h | 151 ++++++++++++++++++++++++++++++++-----
> > 1 file changed, 133 insertions(+), 18 deletions(-)
> >
> > diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> > index d6466f96cdd00..b181183118b3a 100644
> > --- a/include/linux/virtio_net.h
> > +++ b/include/linux/virtio_net.h
> > @@ -5,7 +5,11 @@
> > #include <linux/if_vlan.h>
> > #include <linux/ip.h>
> > #include <linux/ipv6.h>
> > +#include <linux/netdevice.h>
> > #include <linux/udp.h>
> > +#include <net/gso.h>
> > +#include <net/ip.h>
> > +#include <net/ipv6.h>
> > #include <net/tcp.h>
> > #include <uapi/linux/tcp.h>
> > #include <uapi/linux/virtio_net.h>
> > @@ -48,15 +52,111 @@ static inline int virtio_net_hdr_set_proto(struct
> > sk_buff *skb,
> > return 0;
> > }
> >
> > +/* Does checksum metadata match a known TCP or UDP header? */
> > +static inline bool
> > +virtio_net_csum_partial_ok_at_offset(struct sk_buff *skb, u8 hdr_gso_type,
> > + unsigned int nhoff, __be16 proto)
> > +{
> > + unsigned int thoff;
> > + int ip_proto;
> > +
> > + if (proto == htons(ETH_P_IP)) {
> > + const struct iphdr *iph = ip4_parse(skb, nhoff, &thoff);
> > +
> > + if (!iph || ip_is_fragment(iph))
> > + return false;
> > + ip_proto = iph->protocol;
> > + } else if (proto == htons(ETH_P_IPV6)) {
> > + const struct ipv6hdr *iph;
> > + int flags = 0;
> > +
> > + if (!pskb_may_pull(skb, nhoff + sizeof(*iph)))
> > + return false;
> > + iph = (const void *)(skb->data + nhoff);
> > + if (iph->version != 6)
> > + return false;
> > + thoff = nhoff + sizeof(*iph);
> > + ip_proto = iph->nexthdr;
> > + if (ip_proto != IPPROTO_TCP && ip_proto != IPPROTO_UDP) {
> > + thoff = nhoff;
> > + ip_proto = ipv6_find_hdr(skb, &thoff, -1, NULL,
> > + &flags);
Skip this rare case of extension headers?
> > + if (ip_proto < 0 || flags & IP6_FH_F_FRAG)
> > + return false;
> > + }
> > + } else {
> > + return false;
> > + }
> > +
> > static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb,
> > const struct virtio_net_hdr *hdr,
> > - bool little_endian, u8 hdr_gso_type)
> > + bool little_endian, u8 hdr_gso_type,
> > + bool need_csum_check)
> > {
> > - unsigned int nh_min_len = sizeof(struct iphdr);
> > + typeof(skb->transport_header) old_transport = skb->transport_header;
> > unsigned int gso_type = 0;
> > unsigned int thlen = 0;
> > unsigned int p_off = 0;
> > unsigned int ip_proto;
> > + bool csum_partial_ok = true;
> >
> > if (hdr_gso_type != VIRTIO_NET_HDR_GSO_NONE) {
> > switch (hdr_gso_type & ~VIRTIO_NET_HDR_GSO_ECN) {
> > @@ -69,7 +169,6 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff
> > *skb,
> > gso_type = SKB_GSO_TCPV6;
> > ip_proto = IPPROTO_TCP;
> > thlen = sizeof(struct tcphdr);
> > - nh_min_len = sizeof(struct ipv6hdr);
> > break;
> > case VIRTIO_NET_HDR_GSO_UDP:
> > gso_type = SKB_GSO_UDP;
> > @@ -104,11 +203,11 @@ static inline int __virtio_net_hdr_to_skb(struct
> > sk_buff *skb,
> >
> > if (!skb_partial_csum_set(skb, start, off))
> > return -EINVAL;
> > - if (skb_transport_offset(skb) < nh_min_len)
> > - return -EINVAL;
> > + if (need_csum_check)
> > + csum_partial_ok =
> > + virtio_net_csum_partial_ok(skb, hdr_gso_type);
> >
> > - nh_min_len = skb_transport_offset(skb);
> > - p_off = nh_min_len + thlen;
> > + p_off = skb_transport_offset(skb) + thlen;
> > if (!pskb_may_pull(skb, p_off))
> > return -EINVAL;
> > } else if (gso_type) {
> > @@ -158,8 +257,6 @@ static inline int __virtio_net_hdr_to_skb(struct
> > sk_buff *skb,
> > case SKB_GSO_UDP_L4:
> > if (!(hdr->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM))
> > return -EINVAL;
> > - if (skb->csum_offset != offsetof(struct udphdr,
> > check))
> > - return -EINVAL;
> > if (skb->len - p_off > gso_size * UDP_MAX_SEGMENTS)
> > return -EINVAL;
> > if (gso_type != SKB_GSO_UDP_L4)
> > @@ -167,10 +264,6 @@ static inline int __virtio_net_hdr_to_skb(struct
> > sk_buff *skb,
> > break;
> > case SKB_GSO_TCPV4:
> > case SKB_GSO_TCPV6:
> > - if (skb->ip_summed == CHECKSUM_PARTIAL &&
> > - skb->csum_offset != offsetof(struct tcphdr,
> > check))
> > - return -EINVAL;
> > -
> > BUILD_BUG_ON(TCP_MIN_GSO_SIZE * GSO_MAX_SEGS <
> > GSO_MAX_SIZE);
> > gso_size = max(gso_size, TCP_MIN_GSO_SIZE);
> > break;
> > @@ -191,6 +284,17 @@ static inline int __virtio_net_hdr_to_skb(struct
> > sk_buff *skb,
> > }
> > }
> >
> > + if ((hdr->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) && !csum_partial_ok) {
> > + int err;
> > +
> > + if (skb_is_gso(skb))
> > + return -EINVAL;
> > +
> > + err = skb_checksum_help(skb);
Instead of this variable csum_partial_ok and this branch, could
virtio_net_csum_partial_ok just call skb_checksum_help directly and
drop VIRTIO_NET_HDR_F_NEEDS_CSUM?
> > + skb->transport_header = old_transport;
> > + return err;
> > + }
> > +
> > return 0;
> > }
> >
> > @@ -198,7 +302,8 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff
> > *skb,
> > const struct virtio_net_hdr *hdr,
> > bool little_endian)
> > {
> > - return __virtio_net_hdr_to_skb(skb, hdr, little_endian,
> > hdr->gso_type);
> > + return __virtio_net_hdr_to_skb(skb, hdr, little_endian,
> > + hdr->gso_type, true);
Would be great if we can avoid the extra arg and having to update the callers.
> > }
> >
> > /* This function must be called after virtio_net_hdr_from_skb(). */
> > @@ -293,6 +398,7 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
> > unsigned int inner_nh, outer_th, inner_th;
> > unsigned int inner_l3min, outer_l3min;
> > u8 gso_inner_type, gso_tunnel_type;
> > + __be16 inner_proto;
> > bool outer_isv6, inner_isv6;
> > int ret;
> >
> > @@ -334,7 +440,7 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
> >
> > /* Let the basic parsing deal with plain GSO features. */
> > ret = __virtio_net_hdr_to_skb(skb, hdr, true,
> > - hdr->gso_type & ~gso_tunnel_type);
> > + hdr->gso_type & ~gso_tunnel_type,
> > false);
> > if (ret)
> > return ret;
> >
> > @@ -352,8 +458,8 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
> > return -EINVAL;
> > }
> >
> > - skb_set_inner_protocol(skb, inner_isv6 ? htons(ETH_P_IPV6) :
> > - htons(ETH_P_IP));
> > + inner_proto = inner_isv6 ? htons(ETH_P_IPV6) : htons(ETH_P_IP);
> > + skb_set_inner_protocol(skb, inner_proto);
Can drop
> > if (hdr->flags & VIRTIO_NET_HDR_F_UDP_TUNNEL_CSUM) {
> > if (!tnl_csum_negotiated)
> > return -EINVAL;
> > @@ -368,7 +474,16 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
> > skb->inner_mac_header = inner_nh + skb_headroom(skb);
> > skb->transport_header = outer_th + skb_headroom(skb);
> > skb->encapsulation = 1;
> > - return 0;
> > +
> > + if (virtio_net_csum_partial_ok_at_offset(skb, gso_inner_type,
> > inner_nh,
> > + inner_proto))
> > + return 0;
> > +
> > + if (skb_is_gso(skb))
> > + return -EINVAL;
This is a risk the commit message will have to call out.
I don't see a simple way around it. We discussed segmenting here too,
but that requires intrusive changes to all callers too.
Alternatively,
1. call segmentation, but only to see it remove the SKB_GSO_DODGY and
thus verify that the TSO/USO packet is valid. Still drop otherwise.
This also does not add overhead, because it would be called later
anyway.
2. a failsafe off-by-default sysctl + static_branch that skips the
drop when set. Messy, but so is the situation we're dealing with.
