On Wed, Sep 23, 2026 at 12:21 PM Michael S. Tsirkin <[email protected]> wrote: > > On Tue, Sep 22, 2026 at 09:27:26PM -0400, Willem de Bruijn wrote: > > Willem de Bruijn wrote: > > > Paulos Yibelo wrote: > > > > __virtio_net_hdr_to_skb() checks a minimum network-header length for > > > > CHECKSUM_PARTIAL packets. Its checksum start is relative to skb->data, > > > > but some callers have not established skb->network_header when they > > > > convert the virtio header. > > > > > > > > Pass the data-relative L3 origin explicitly. Ethernet receive paths > > > > parse the frame and nested VLAN headers without changing skb state. > > > > AF_PACKET uses the frame's actual L3 origin even when the socket > > > > protocol is ETH_P_IP and the raw frame carries VLAN tags. Non-Ethernet > > > > AF_PACKET devices retain their established skb network offset. > > > > > > > > Also pass the actual L3 protocol so IPv6 packets use the 40-byte base > > > > header minimum even without TCPv6 GSO. IFF_TUN obtains that protocol > > > > from the packet before skb->protocol is set. Name the Ethernet parser > > > > accordingly, use the same origin for tunnel validation, and propagate > > > > conversion failures in UML. > > > > > > > > The bound remains a minimum; fragmentation paths separately validate > > > > the parsed IPv4 or IPv6 header length before completing a checksum. > > > > > > > > Fixes: 49d14b54a527 ("net: test for not too small csum_start in > > > > virtio_net_hdr_to_skb()") > > > > Fixes: a2fb4bc4e2a6 ("net: implement virtio helpers to handle UDP GSO > > > > tunneling.") > > > > Reported-by: Paulos Yibelo <[email protected]> > > > > Link: > > > > https://lore.kernel.org/netdev/[email protected]/ > > > > Cc: [email protected] > > > > Assisted-by: LLM > > > > Signed-off-by: Paulos Yibelo <[email protected]> > > > > --- > > > > arch/um/drivers/vector_transports.c | 13 ++++- > > > > drivers/net/tun_vnet.h | 52 ++++++++++++++++- > > > > drivers/net/virtio_net.c | 10 +++- > > > > include/linux/virtio_net.h | 87 ++++++++++++++++++++++++----- > > > > net/packet/af_packet.c | 24 +++++++- > > > > 5 files changed, 163 insertions(+), 23 deletions(-) > > > > > > The fix may still miss the case IPv4 packets have options. > > > > > > This version is a very large patch. > > > > > > Untested shorter first suggestion by bot, which looks plausible as a > > > starting point for discussion. > > > > Cleaned up some more: > > > > diff --git a/drivers/net/tun_vnet.h b/drivers/net/tun_vnet.h > > index f4c652b1fa44..c24607af2aad 100644 > > --- a/drivers/net/tun_vnet.h > > +++ b/drivers/net/tun_vnet.h > > @@ -180,6 +180,9 @@ static inline int tun_vnet_hdr_put(int sz, struct > > iov_iter *iter, > > static inline int tun_vnet_hdr_to_skb(unsigned int flags, struct > > sk_buff *skb, > > const struct virtio_net_hdr *hdr) > > { > > + if ((flags & TUN_TYPE_MASK) == IFF_TUN) > > + skb_reset_network_header(skb); > > + > > return virtio_net_hdr_to_skb(skb, hdr, > > tun_vnet_is_little_endian(flags)); > > } > > > > @@ -199,6 +202,9 @@ tun_vnet_hdr_tnl_to_skb(unsigned int flags, > > netdev_features_t features, > > struct sk_buff *skb, > > const struct virtio_net_hdr_v1_hash_tunnel *hdr) > > { > > + if ((flags & TUN_TYPE_MASK) == IFF_TUN) > > + skb_reset_network_header(skb); > > + > > > > diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h > > index c381b916c1b5..02c448de0802 100644 > > --- a/include/linux/virtio_net.h > > +++ b/include/linux/virtio_net.h > > @@ -48,6 +48,42 @@ static inline int virtio_net_hdr_set_proto(struct > > sk_buff *skb, > > return 0; > > } > > > > +static inline int virtio_net_hdr_nh_min_len(const struct sk_buff *skb, > > + unsigned int nh_min_len) > > +{ > > + int thoff = skb_transport_offset(skb); > > + __be16 proto; > > + int nhoff; > > + > > + if (skb_network_header_was_set(skb)) { > > + nhoff = skb_network_offset(skb); > > + proto = skb->protocol; > > + } else { > > + if (unlikely(thoff < ETH_HLEN)) > > + return -EINVAL; > > + nhoff = ETH_HLEN; > > + proto = eth_hdr(skb)->h_proto; > > + } > > + > > + if (eth_type_vlan(proto)) { > > + proto = __vlan_get_protocol(skb, proto, &nhoff); > > + if (!proto) > > + return -EINVAL; > > + } > > + > > + if (proto == htons(ETH_P_IP)) { > > + const struct iphdr *iph = (void *)(skb->data + nhoff); > > + > > + if (unlikely(thoff < nhoff + sizeof(*iph))) > > + return -EINVAL; > > + nh_min_len = max_t(u32, iph->ihl * 4, sizeof(*iph)); > > + } else if (proto == htons(ETH_P_IPV6)) { > > + nh_min_len = sizeof(struct ipv6hdr); > > + } > > + > > + return nhoff + nh_min_len; > > +} > > + > > 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) > > @@ -98,13 +134,15 @@ static inline int __virtio_net_hdr_to_skb(struct > > sk_buff *skb, > > u32 start = __virtio16_to_cpu(little_endian, hdr->csum_start); > > u32 off = __virtio16_to_cpu(little_endian, hdr->csum_offset); > > u32 needed = start + max_t(u32, thlen, off + sizeof(__sum16)); > > + int min_thoff; > > > > if (!pskb_may_pull(skb, needed)) > > return -EINVAL; > > > > if (!skb_partial_csum_set(skb, start, off)) > > return -EINVAL; > > - if (skb_transport_offset(skb) < nh_min_len) > > + min_thoff = virtio_net_hdr_nh_min_len(skb, nh_min_len); > > + if (min_thoff < 0 || skb_transport_offset(skb) < min_thoff) > > return -EINVAL; > > > Certainly looks much better. But I'd like to ask, generally: > doesn't the net stack need to protect against weird packets? >
It does, but csum_start/csum_offset are not "weird packet" material, they are skb metadata, not bytes on the wire. For essentially every skb in the kernel, this metadata is produced by the kernel itself, from headers it has just built or just parsed, and the rest of the stack (GSO, skb_checksum_help(), fragmentation, netfilter, and every driver doing TX csum offload) consumes it as an invariant. The only producers of attacker/guest controlled CHECKSUM_PARTIAL metadata are the virtio_net_hdr_to_skb() callers: af_packet, tun/tap, virtio_net, and the UML vector driver. So this is not "virtio specific validation", this is input validation at the one trust boundary where the invariant can be violated. > It seems likely that not all drivers validate headers defensively, > and incoming packets can easily become outgoing ones. Packets coming from a real NIC are CHECKSUM_UNNECESSARY, CHECKSUM_COMPLETE or CHECKSUM_NONE. They do not carry a remote-provided csum_start. (Remote checksum offload is the rare exception, and there the offsets are computed by the stack from headers it just parsed.) Incoming packets becoming outgoing ones is precisely the problem here: a virtio_net RX skb with VIRTIO_NET_HDR_F_NEEDS_CSUM becomes an skb that can be bridged/forwarded/fragmented and then handed to a real NIC. > So do we even need virtio specific validation, or is it enough to > validate everything in the net stack, where we are poking at the > header anyway? The core stack cannot afford it. 1) Drivers program skb->csum_start / skb->csum_offset straight into the TX descriptor, and the NIC will happily write two bytes wherever it was told, e.g. into the IP header of the frame we put on the wire. Auditing/adding checks in every driver is not realistic, and drivers must not pay for it. 2) Catching this in the core would mean re-parsing L2/L3 in dev_hard_start_xmit() (or in every place that eventually looks at the transport header) for the 99.99+% of packets that were built by the stack and are known to be consistent. That is far more expensive than one check at injection time. 3) Failing at injection returns -EINVAL to the sendmsg()/writev() caller, which is the correct and testable behavior. Failing later means dropping the packet deep in the xmit path, usually with a splat: the commit being fixed here (49d14b54a527) exists exactly because such a packet reached skb_checksum_help() from ip_do_fragment() and hit the "offset (-6) >= skb_headlen() (14)" WARN. > > Or maybe it's more a defense in depth thing? > > My worries: > - more poking at the header, more cache misses, where we really > do not need that I do not expect anything measurable. af_packet and tun: we have just copied that header from user space, it is in L1. virtio_net: we call eth_type_trans() right after, and GRO parses L3/L4 immediately. The whole block is under the CHECKSUM_PARTIAL condition, where we already do pskb_may_pull() and skb_partial_csum_set(), i.e. we already touch this cache line. Reading iph->ihl from a cache line we are about to read anyway is noise. > - future protocol extensions that now will require surgery in > virtio, instead of just being passed through to the host Fair, and this is an argument about how the check is written, not about whether it exists. The rule should be: Only tighten the bound for the protocols we already parse (IPv4/IPv6), and keep the existing generic minimum for anything else. Then an unknown ethertype simply keeps flowing, no surgery is needed. Willem's version does that. A new protocol would only be impacted if it wanted csum_start to point inside what we consider the L3 header, and such a packet would not survive the rest of the stack anyway. So: not defense in depth, but validation at the trust boundary, where it is cheapest and where we can still report the error to the producer. > > What do others think? > > -- > MST >
