* Re: [PATCH net v5 1/2] net: validate virtio checksum start after network header
2026-09-21 2:53 ` [PATCH net v5 1/2] net: validate virtio checksum start after network header Paulos Yibelo
@ 2026-09-21 22:11 ` Michael S. Tsirkin
2026-09-21 22:18 ` Michael S. Tsirkin
` (2 subsequent siblings)
3 siblings, 0 replies; 8+ messages in thread
From: Michael S. Tsirkin @ 2026-09-21 22:11 UTC (permalink / raw)
To: Paulos Yibelo
Cc: netdev, richard, anton.ivanov, johannes, willemdebruijn.kernel,
jasowangio, eperezma, xuanzhuo, andrew+netdev, pablo, fw, phil,
razor, idosch, dsahern, davem, edumazet, kuba, pabeni, horms,
linux-um, virtualization, netfilter-devel, coreteam, bridge,
linux-kernel
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 <habte.yibelo@gmail.com>
> Link: https://lore.kernel.org/netdev/20260920004733.6473-2-habte.yibelo@gmail.com/
> Cc: stable@vger.kernel.org
> Signed-off-by: Paulos Yibelo <habte.yibelo@gmail.com>
v4 had assisted-by tag? same q for patch 2.
> ---
> 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);
> 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;
> }
> @@ -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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH net v5 1/2] net: validate virtio checksum start after network header
2026-09-21 2:53 ` [PATCH net v5 1/2] net: validate virtio checksum start after network header Paulos Yibelo
2026-09-21 22:11 ` Michael S. Tsirkin
@ 2026-09-21 22:18 ` Michael S. Tsirkin
2026-09-21 22:44 ` Michael S. Tsirkin
2026-09-24 8:54 ` netdev-bot+sashiko
3 siblings, 0 replies; 8+ messages in thread
From: Michael S. Tsirkin @ 2026-09-21 22:18 UTC (permalink / raw)
To: Paulos Yibelo
Cc: netdev, richard, anton.ivanov, johannes, willemdebruijn.kernel,
jasowangio, eperezma, xuanzhuo, andrew+netdev, pablo, fw, phil,
razor, idosch, dsahern, davem, edumazet, kuba, pabeni, horms,
linux-um, virtualization, netfilter-devel, coreteam, bridge,
linux-kernel
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 <habte.yibelo@gmail.com>
> Link: https://lore.kernel.org/netdev/20260920004733.6473-2-habte.yibelo@gmail.com/
> Cc: stable@vger.kernel.org
> Signed-off-by: Paulos Yibelo <habte.yibelo@gmail.com>
> ---
> 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)
+virtio_net_hdr_eth_get_l3_offset ?
since this assumes ethernet...
> +{
> + 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);
> 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;
> }
> @@ -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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH net v5 1/2] net: validate virtio checksum start after network header
2026-09-21 2:53 ` [PATCH net v5 1/2] net: validate virtio checksum start after network header Paulos Yibelo
2026-09-21 22:11 ` Michael S. Tsirkin
2026-09-21 22:18 ` Michael S. Tsirkin
@ 2026-09-21 22:44 ` Michael S. Tsirkin
2026-09-24 8:54 ` netdev-bot+sashiko
3 siblings, 0 replies; 8+ messages in thread
From: Michael S. Tsirkin @ 2026-09-21 22:44 UTC (permalink / raw)
To: Paulos Yibelo
Cc: netdev, richard, anton.ivanov, johannes, willemdebruijn.kernel,
jasowangio, eperezma, xuanzhuo, andrew+netdev, pablo, fw, phil,
razor, idosch, dsahern, davem, edumazet, kuba, pabeni, horms,
linux-um, virtualization, netfilter-devel, coreteam, bridge,
linux-kernel
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 <habte.yibelo@gmail.com>
> Link: https://lore.kernel.org/netdev/20260920004733.6473-2-habte.yibelo@gmail.com/
> Cc: stable@vger.kernel.org
> Signed-off-by: Paulos Yibelo <habte.yibelo@gmail.com>
> ---
> 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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH net v5 1/2] net: validate virtio checksum start after network header
2026-09-21 2:53 ` [PATCH net v5 1/2] net: validate virtio checksum start after network header Paulos Yibelo
` (2 preceding siblings ...)
2026-09-21 22:44 ` Michael S. Tsirkin
@ 2026-09-24 8:54 ` netdev-bot+sashiko
3 siblings, 0 replies; 8+ messages in thread
From: netdev-bot+sashiko @ 2026-09-24 8:54 UTC (permalink / raw)
To: habte.yibelo
Cc: netdev, richard, anton.ivanov, johannes, willemdebruijn.kernel,
jasowangio, mst, eperezma, xuanzhuo, andrew+netdev, pablo, fw,
phil, razor, idosch, dsahern, davem, edumazet, kuba, pabeni,
horms, linux-um, virtualization, netfilter-devel, coreteam,
bridge, linux-kernel
Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.
Critical: 0 · High: 1 · Medium: 1 · Low: 0
If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).
- [High] The two AF_PACKET call sites pass skb_network_offset(skb) as the
new `network_offset` argument, while every other converted caller…
- [Medium] In arch/um/drivers/vector_transports.c:raw_verify_header(),
the newly added virtio_net_hdr_get_l3_offset() parse and the now-…
--
net: validate virtio checksum start after network header
This adds an explicit data-relative L3 offset argument to
virtio_net_hdr_to_skb() and virtio_net_hdr_tnl_to_skb() so that a
CHECKSUM_PARTIAL start landing inside the network header is rejected. A new
helper virtio_net_hdr_get_l3_offset() parses Ethernet and nested VLAN headers
with skb_header_pointer(), and all in-tree callers are converted.
Link: https://lore.kernel.org/netdev/20260920004733.6473-2-habte.yibelo@gmail.com/
> diff --git a/arch/um/drivers/vector_transports.c b/arch/um/drivers/vector_transports.c
> index ddd127ee9678..79bc05fc6f73 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);
> }
[Severity: Medium]
Is the skb passed to raw_verify_header() already trimmed to the received
frame length at this point?
prep_skb() in arch/um/drivers/vector_kern.c sets the length from the
receive buffer capacity, before the recvmsg/recvmmsg:
skb_put(result, vp->max_packet);
result->data_len = len - vp->max_packet;
result->len += len - vp->max_packet;
and both receive loops call the verify callback before the trim:
vector_legacy_rx():
header_check = vp->verify_header(
vp->header_rxbuffer, skb, vp);
if (header_check < 0) { ... return 0; }
}
pskb_trim(skb, pkt_len - vp->rx_header_size);
vector_mmsg_rx() has the same ordering. The only guarantee before the
callback is pkt_len > vp->header_size, i.e. one payload byte.
So can the Ethernet and VLAN bytes that virtio_net_hdr_get_l3_offset()
reads at offsets 0..13 and beyond be bytes recvmsg never wrote, i.e. the
uninitialised remainder of the allocation, making network_offset and the
accept/reject decision depend on stale buffer contents whenever the
received frame is shorter than the parsed span?
Along the same lines, the length checks inside __virtio_net_hdr_to_skb()
see skb->len as capacity (up to vp->req_size, 65536 with GSO) rather than
the received length, for instance:
if (skb->len - p_off > gso_size * UDP_MAX_SEGMENTS)
return -EINVAL;
Since raw_verify_header() now returns the converter's error instead of
discarding it, does this turn the length mismatch into frames dropped with
rx_encaps_errors, where previously the return value was ignored? Would it
be better to move the pskb_trim() ahead of the verify_header() call?
> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> index c381b916c1b5..a4c005796acb 100644
> --- a/include/linux/virtio_net.h
> +++ b/include/linux/virtio_net.h
[ ... ]
> @@ -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;
[ ... ]
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index 50cae32ae269..04c80e23d7e2 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;
> }
> @@ -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;
[Severity: High]
Is skb_network_offset(skb) a strong enough origin here? Every other
converted caller derives the offset from the frame bytes via
virtio_net_hdr_get_l3_offset(), while these two use the network header
AF_PACKET established itself.
In packet_parse_headers() the VLAN adjustment is gated on the
socket-supplied protocol:
if (likely(skb->dev->type == ARPHRD_ETHER) &&
eth_type_vlan(skb->protocol) &&
vlan_get_protocol_and_depth(skb, skb->protocol, &depth) != 0)
skb_set_network_header(skb, depth);
For SOCK_RAW with an explicit sll_protocol, skb->protocol is the value the
sender passed, not the frame's EtherType (dev_parse_header_protocol() runs
only when skb->protocol is 0 or ETH_P_ALL).
So for a sender that binds with sll_protocol = htons(ETH_P_IP) but writes
an 802.1Q-tagged frame, network_offset stays 14 while the real IPv4 header
starts at 18. With csum_start = 34 and nh_min_len = 20, the new test
becomes:
transport_offset = 34;
34 < 20 -> false
14 > 34 - 20 -> 14 > 14 -> false
and the header is accepted, after skb_partial_csum_set() has already set
ip_summed = CHECKSUM_PARTIAL and transport_header = 34, which is 16 bytes
inside the IPv4 header spanning 18..37. Is that not exactly the case the
commit message says is now rejected?
Two related cases in the same path:
packet_parse_headers() silently ignores a failed
vlan_get_protocol_and_depth() (more nested tags than that parser accepts),
leaving network_offset at 14 for a deeper real L3 header.
In packet_snd(), the SOCK_RAW short-packet branch resets the network header
while skb->data still points at the link header:
} else if (reserve) {
skb_reserve(skb, -reserve);
if (len < reserve + sizeof(struct ipv6hdr) &&
dev->min_header_len != hard_header_len)
skb_reset_network_header(skb);
}
which yields network_offset == 0, and the new bound then reduces to the
pre-patch csum_start >= 20 check. Would using
virtio_net_hdr_get_l3_offset() at both AF_PACKET call sites, as the TAP and
virtio-net paths do, keep the origin consistent?
Note the follow-up commit in this area, "ip: reject partial checksums
covering network headers", only touches ip_do_fragment(), ip6_fragment(),
nf_br_ip_fragment() and br_ip6_fragment(), so it does not close this
AF_PACKET path later in the series.
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260921025341.44846-1-habte.yibelo%40gmail.com
^ permalink raw reply [flat|nested] 8+ messages in thread