Thanks, I’ve addressed these points in v6 and will post the full revised series shortly when the timing gate opens.
On Mon, Sep 21, 2026 at 6:44 PM Michael S. Tsirkin <[email protected]> wrote: > On Sun, Sep 20, 2026 at 10:53:40PM -0400, Paulos Yibelo wrote: > > __virtio_net_hdr_to_skb() rejects a CHECKSUM_PARTIAL start smaller than > > an estimated minimum network-header length. Its input offsets are > relative > > to skb->data. > > > > Using skb_network_offset() here is unsafe. TUN/TAP, virtio-net, and UML > > parse a received virtio header before skb->network_header is established. > > On an skb with headroom, the resulting negative offset enlarges the > > apparent distance to the transport header and can admit a checksum start > > inside the network header. > > > > Pass the data-relative L3 offset to the converter explicitly. IFF_TUN > uses > > zero, AF_PACKET supplies its established network offset, and Ethernet > > receive paths parse Ethernet and nested VLAN headers with > > skb_header_pointer(), without changing skb state. Use the same origin for > > tunnel-offset validation, and make UML propagate conversion failures. > > > > This does not require a virtual-machine guest. A TUN or TAP device with > > virtio-net header support is sufficient to reach these paths. > > > > 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] > > Signed-off-by: Paulos Yibelo <[email protected]> > > --- > > Changes in v5: > > - Replace the not-yet-established skb network-header offset with an > > explicit data-relative L3 origin. > > - Cover all in-tree callers, including Ethernet/VLAN receive paths, > > tunnel metadata, and UML error propagation. > > - Drop the prior Acked-by and Reviewed-by tags because the code changed. > > > > Changes in v4: > > - State that a TUN device is sufficient and no guest is required, as > > noted by Michael S. Tsirkin. > > > > Changes in v3: > > - Keep the network-relative comparison on one line for readability, as > > requested by David Ahern. > > > > Changes in v2: > > - Make nh_min_len an int and remove the casts, as suggested by Michael S. > > Tsirkin. > > > > arch/um/drivers/vector_transports.c | 10 +++- > > drivers/net/tun_vnet.h | 28 ++++++++++- > > drivers/net/virtio_net.c | 8 ++- > > include/linux/virtio_net.h | 76 +++++++++++++++++++++++------ > > net/packet/af_packet.c | 6 ++- > > 5 files changed, 106 insertions(+), 22 deletions(-) > > > > diff --git a/arch/um/drivers/vector_transports.c > b/arch/um/drivers/vector_transports.c > > index ddd127ee9..79bc05fc6 100644 > > --- a/arch/um/drivers/vector_transports.c > > +++ b/arch/um/drivers/vector_transports.c > > @@ -197,6 +197,7 @@ static int raw_verify_header( > > uint8_t *header, struct sk_buff *skb, struct vector_private *vp) > > { > > struct virtio_net_hdr *vheader = (struct virtio_net_hdr *) header; > > + int network_offset; > > > > if ((vheader->gso_type != VIRTIO_NET_HDR_GSO_NONE) && > > (vp->req_size != 65536)) { > > @@ -209,8 +210,13 @@ static int raw_verify_header( > > if ((vheader->flags & VIRTIO_NET_HDR_F_DATA_VALID) > 0) > > return 1; > > > > - virtio_net_hdr_to_skb(skb, vheader, > virtio_legacy_is_little_endian()); > > - return 0; > > + network_offset = virtio_net_hdr_get_l3_offset(skb, vheader); > > + if (network_offset < 0) > > + return network_offset; > > + > > + return virtio_net_hdr_to_skb(skb, vheader, > > + virtio_legacy_is_little_endian(), > > + network_offset); > > } > > > > static bool get_uint_param( > > diff --git a/drivers/net/tun_vnet.h b/drivers/net/tun_vnet.h > > index f4c652b1f..1c83c359d 100644 > > --- a/drivers/net/tun_vnet.h > > +++ b/drivers/net/tun_vnet.h > > @@ -177,10 +177,27 @@ static inline int tun_vnet_hdr_put(int sz, struct > iov_iter *iter, > > return __tun_vnet_hdr_put(sz, 0, iter, hdr); > > } > > > > +static inline int > > +tun_vnet_hdr_get_l3_offset(unsigned int flags, const struct sk_buff > *skb, > > + const struct virtio_net_hdr *hdr) > > +{ > > + if ((flags & TUN_TYPE_MASK) != IFF_TAP) > > + return 0; > > + > > + return virtio_net_hdr_get_l3_offset(skb, hdr); > > +} > > + > > static inline int tun_vnet_hdr_to_skb(unsigned int flags, struct > sk_buff *skb, > > const struct virtio_net_hdr *hdr) > > { > > - return virtio_net_hdr_to_skb(skb, hdr, > tun_vnet_is_little_endian(flags)); > > + int network_offset = tun_vnet_hdr_get_l3_offset(flags, skb, hdr); > > + > > + if (network_offset < 0) > > + return network_offset; > > + > > + return virtio_net_hdr_to_skb(skb, hdr, > > + tun_vnet_is_little_endian(flags), > > + network_offset); > > } > > > > /* > > @@ -199,10 +216,17 @@ 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) > > { > > + const struct virtio_net_hdr *vnet_hdr = (const struct > virtio_net_hdr *)hdr; > > + int network_offset = tun_vnet_hdr_get_l3_offset(flags, skb, > vnet_hdr); > > + > > + if (network_offset < 0) > > + return network_offset; > > + > > return virtio_net_hdr_tnl_to_skb(skb, hdr, > > features & NETIF_F_GSO_UDP_TUNNEL, > > features & NETIF_F_GSO_UDP_TUNNEL_CSUM, > > - tun_vnet_is_little_endian(flags)); > > + tun_vnet_is_little_endian(flags), > > + network_offset); > > } > > > > static inline int tun_vnet_hdr_from_skb(unsigned int flags, > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > > index e34c52d05..059eeb18e 100644 > > --- a/drivers/net/virtio_net.c > > +++ b/drivers/net/virtio_net.c > > @@ -2502,6 +2502,7 @@ static void virtnet_receive_done(struct > virtnet_info *vi, struct receive_queue * > > { > > struct virtio_net_common_hdr *hdr; > > struct net_device *dev = vi->dev; > > + int network_offset; > > > > hdr = skb_vnet_common_hdr(skb); > > if (dev->features & NETIF_F_RXHASH && vi->has_rss_hash_report) > > @@ -2515,9 +2516,12 @@ static void virtnet_receive_done(struct > virtnet_info *vi, struct receive_queue * > > goto frame_err; > > } > > > > - if (virtio_net_hdr_tnl_to_skb(skb, &hdr->tnl_hdr, vi->rx_tnl, > > + network_offset = virtio_net_hdr_get_l3_offset(skb, &hdr->hdr); > > + if (network_offset < 0 || > > + virtio_net_hdr_tnl_to_skb(skb, &hdr->tnl_hdr, vi->rx_tnl, > > vi->rx_tnl_csum, > > - virtio_is_little_endian(vi->vdev))) { > > + virtio_is_little_endian(vi->vdev), > > + network_offset)) { > > net_warn_ratelimited("%s: bad gso: type: %x, size: %u, > flags %x tunnel %d tnl csum %d\n", > > dev->name, hdr->hdr.gso_type, > > hdr->hdr.gso_size, hdr->hdr.flags, > > diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h > > index c381b916c..a4c005796 100644 > > --- a/include/linux/virtio_net.h > > +++ b/include/linux/virtio_net.h > > @@ -48,11 +48,49 @@ static inline int virtio_net_hdr_set_proto(struct > sk_buff *skb, > > return 0; > > } > > > > +/* > > + * Return the L3 offset of an Ethernet frame starting at skb->data. > > + * The offset is unused without NEEDS_CSUM, so avoid parsing and return > zero. > > + */ > > +static inline int > > +virtio_net_hdr_get_l3_offset(const struct sk_buff *skb, > > + const struct virtio_net_hdr *hdr) > > +{ > > + unsigned int parse_depth = VLAN_MAX_DEPTH; > > + const struct ethhdr *eth; > > + struct ethhdr ethbuf; > > + __be16 protocol; > > + int depth = ETH_HLEN; > > + > > + if (!(hdr->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM)) > > + return 0; > > + > > + eth = skb_header_pointer(skb, 0, sizeof(ethbuf), ðbuf); > > + if (!eth) > > + return -EINVAL; > > + > > + protocol = eth->h_proto; > > + while (eth_type_vlan(protocol)) { > > + const struct vlan_hdr *vh; > > + struct vlan_hdr vhdr; > > + > > + vh = skb_header_pointer(skb, depth, sizeof(vhdr), &vhdr); > > + if (!vh || !--parse_depth) > > + return -EINVAL; > > + > > + protocol = vh->h_vlan_encapsulated_proto; > > + depth += VLAN_HLEN; > > + } > > + > > + return depth; > > +} > > + > > 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, > > + int network_offset) > > { > > - unsigned int nh_min_len = sizeof(struct iphdr); > > + int nh_min_len = sizeof(struct iphdr); > > > So why is this IPv4 specific assumption still here? Confused. > > > > unsigned int gso_type = 0; > > unsigned int thlen = 0; > > unsigned int p_off = 0; > > @@ -98,16 +136,20 @@ 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 transport_offset; > > > > 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) > > + > > + transport_offset = skb_transport_offset(skb); > > + if (transport_offset < nh_min_len || network_offset < 0 || > > + network_offset > transport_offset - nh_min_len) > > return -EINVAL; > > > > - nh_min_len = skb_transport_offset(skb); > > + nh_min_len = transport_offset; > > p_off = nh_min_len + thlen; > > if (!pskb_may_pull(skb, p_off)) > > return -EINVAL; > > @@ -206,9 +248,11 @@ static inline int __virtio_net_hdr_to_skb(struct > sk_buff *skb, > > > > static inline int virtio_net_hdr_to_skb(struct sk_buff *skb, > > const struct virtio_net_hdr *hdr, > > - bool little_endian) > > + bool little_endian, > > + int network_offset) > > { > > - 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, > > + network_offset); > > } > > > > /* This function must be called after virtio_net_hdr_from_skb(). */ > > @@ -287,7 +331,7 @@ static inline int virtio_net_hdr_from_skb(const > struct sk_buff *skb, > > return 0; > > } > > > > -static inline unsigned int virtio_l3min(bool is_ipv6) > > +static inline int virtio_l3min(bool is_ipv6) > > { > > return is_ipv6 ? sizeof(struct ipv6hdr) : sizeof(struct iphdr); > > } > > @@ -297,18 +341,19 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb, > > const struct virtio_net_hdr_v1_hash_tunnel *vhdr, > > bool tnl_hdr_negotiated, > > bool tnl_csum_negotiated, > > - bool little_endian) > > + bool little_endian, int network_offset) > > { > > const struct virtio_net_hdr *hdr = (const struct virtio_net_hdr > *)vhdr; > > - unsigned int inner_nh, outer_th, inner_th; > > - unsigned int inner_l3min, outer_l3min; > > u8 gso_inner_type, gso_tunnel_type; > > bool outer_isv6, inner_isv6; > > + int inner_nh, outer_th, inner_th; > > + int inner_l3min, outer_l3min; > > int ret; > > > > gso_tunnel_type = hdr->gso_type & VIRTIO_NET_HDR_GSO_UDP_TUNNEL; > > if (!gso_tunnel_type) > > - return virtio_net_hdr_to_skb(skb, hdr, little_endian); > > + return virtio_net_hdr_to_skb(skb, hdr, little_endian, > > + network_offset); > > > > /* Tunnel not supported/negotiated, but the hdr asks for it. */ > > if (!tnl_hdr_negotiated) > > @@ -332,19 +377,22 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb, > > outer_isv6 = gso_tunnel_type & VIRTIO_NET_HDR_GSO_UDP_TUNNEL_IPV6; > > inner_isv6 = gso_inner_type == VIRTIO_NET_HDR_GSO_TCPV6; > > inner_l3min = virtio_l3min(inner_isv6); > > - outer_l3min = ETH_HLEN + virtio_l3min(outer_isv6); > > + outer_l3min = virtio_l3min(outer_isv6); > > > > inner_th = __virtio16_to_cpu(little_endian, hdr->csum_start); > > inner_nh = le16_to_cpu(vhdr->inner_nh_offset); > > outer_th = le16_to_cpu(vhdr->outer_th_offset); > > - if (outer_th < outer_l3min || > > + if (network_offset < 0 || > > + outer_th < outer_l3min || > > + network_offset > outer_th - outer_l3min || > > inner_nh < outer_th + sizeof(struct udphdr) || > > inner_th < inner_nh + inner_l3min) > > return -EINVAL; > > > > /* 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, > > + network_offset); > > if (ret) > > return ret; > > > > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c > > index 50cae32ae..04c80e23d 100644 > > --- a/net/packet/af_packet.c > > +++ b/net/packet/af_packet.c > > @@ -2901,7 +2901,8 @@ static int tpacket_snd(struct packet_sock *po, > struct msghdr *msg) > > } > > > > if (has_vnet_hdr) { > > - if (virtio_net_hdr_to_skb(skb, &vnet_hdr, > vio_le())) { > > + if (virtio_net_hdr_to_skb(skb, &vnet_hdr, vio_le(), > > + > skb_network_offset(skb))) { > > tp_len = -EINVAL; > > goto tpacket_error; > > } > > Will this do the right thing for ETH_P_IP sockets? even with vlan tags > in the frame? > > > > @@ -3103,7 +3104,8 @@ static int packet_snd(struct socket *sock, struct > msghdr *msg, size_t len) > > packet_parse_headers(skb, sock); > > > > if (vnet_hdr_sz) { > > - err = virtio_net_hdr_to_skb(skb, &vnet_hdr, vio_le()); > > + err = virtio_net_hdr_to_skb(skb, &vnet_hdr, vio_le(), > > + skb_network_offset(skb)); > > if (err) > > goto out_free; > > len += vnet_hdr_sz; > > -- > > 2.46.0 > > -- PGP Key ID: 53911E7F16446D4C :)
0002-ip-reject-partial-checksums-covering-network-headers.patch
Description: Binary data
0001-net-validate-virtio-checksum-start-after-network-hea.patch
Description: Binary data
