* Re: [PATCH net v6 1/2] net: validate virtio checksum start after network header
2026-09-22 3:03 ` [PATCH net v6 1/2] net: validate virtio checksum start after network header Paulos Yibelo
@ 2026-09-22 5:14 ` Michael S. Tsirkin
[not found] ` <CAHv8Y_4OKdVkigToBmXXhhUxu+EL2iWVADZJ=LS4CGqBATgU5g@mail.gmail.com>
2026-09-22 5:56 ` Johannes Berg
2026-09-22 22:29 ` Willem de Bruijn
2026-09-25 9:05 ` netdev-bot+sashiko
2 siblings, 2 replies; 14+ messages in thread
From: Michael S. Tsirkin @ 2026-09-22 5:14 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 Mon, Sep 21, 2026 at 11:03:09PM -0400, 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 <habte.yibelo@gmail.com>
> Link: https://lore.kernel.org/netdev/20260920004733.6473-2-habte.yibelo@gmail.com/
> Cc: stable@vger.kernel.org
> Assisted-by: LLM
> Signed-off-by: Paulos Yibelo <habte.yibelo@gmail.com>
Wow i just asked some questions, and it's getting more and more complex.
I am not sure people have time to review at this pace.
> ---
> 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(-)
>
> diff --git a/arch/um/drivers/vector_transports.c b/arch/um/drivers/vector_transports.c
> index ddd127ee9..5a9d8f49a 100644
> --- a/arch/um/drivers/vector_transports.c
> +++ b/arch/um/drivers/vector_transports.c
> @@ -197,6 +197,8 @@ 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;
> + __be16 network_protocol;
> + int network_offset;
>
> if ((vheader->gso_type != VIRTIO_NET_HDR_GSO_NONE) &&
> (vp->req_size != 65536)) {
> @@ -209,8 +211,14 @@ 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_eth_get_l3_offset(skb, vheader,
> + &network_protocol);
> + if (network_offset < 0)
> + return network_offset;
> +
> + return virtio_net_hdr_to_skb(skb, vheader,
> + virtio_legacy_is_little_endian(),
> + network_offset, network_protocol);
> }
>
> static bool get_uint_param(
> @@ -491,4 +499,3 @@ int build_transport_data(struct vector_private *vp)
> return build_bess_transport_data(vp);
> return 0;
> }
> -
> diff --git a/drivers/net/tun_vnet.h b/drivers/net/tun_vnet.h
> index f4c652b1f..e004213fa 100644
> --- a/drivers/net/tun_vnet.h
> +++ b/drivers/net/tun_vnet.h
> @@ -177,10 +177,49 @@ 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,
> + __be16 *network_protocol)
> +{
> + if ((flags & TUN_TYPE_MASK) != IFF_TAP) {
> + u8 version, first_byte;
> + const u8 *first;
> +
> + *network_protocol = 0;
> + if (!(hdr->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM))
> + return 0;
> +
> + first = skb_header_pointer(skb, 0, sizeof(first_byte),
> + &first_byte);
> + if (!first)
> + return -EINVAL;
> +
> + version = *first >> 4;
> + if (version == 4)
> + *network_protocol = htons(ETH_P_IP);
> + else if (version == 6)
> + *network_protocol = htons(ETH_P_IPV6);
> + return 0;
> + }
> +
> + return virtio_net_hdr_eth_get_l3_offset(skb, hdr,
> + network_protocol);
> +}
> +
> 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));
> + __be16 network_protocol;
> + int network_offset = tun_vnet_hdr_get_l3_offset(flags, skb, hdr,
> + &network_protocol);
> +
> + if (network_offset < 0)
> + return network_offset;
> +
> + return virtio_net_hdr_to_skb(skb, hdr,
> + tun_vnet_is_little_endian(flags),
> + network_offset, network_protocol);
> }
>
> /*
> @@ -199,10 +238,19 @@ 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;
> + __be16 network_protocol;
> + int network_offset = tun_vnet_hdr_get_l3_offset(flags, skb, vnet_hdr,
> + &network_protocol);
> +
> + 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, network_protocol);
> }
>
> 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..8e93dad28 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -2502,6 +2502,8 @@ static void virtnet_receive_done(struct virtnet_info *vi, struct receive_queue *
> {
> struct virtio_net_common_hdr *hdr;
> struct net_device *dev = vi->dev;
> + __be16 network_protocol;
> + int network_offset;
>
> hdr = skb_vnet_common_hdr(skb);
> if (dev->features & NETIF_F_RXHASH && vi->has_rss_hash_report)
> @@ -2515,9 +2517,13 @@ 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_eth_get_l3_offset(skb, &hdr->hdr,
> + &network_protocol);
> + 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, network_protocol)) {
> 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..b89d821a0 100644
> --- a/include/linux/virtio_net.h
> +++ b/include/linux/virtio_net.h
> @@ -48,16 +48,61 @@ static inline int virtio_net_hdr_set_proto(struct sk_buff *skb,
> return 0;
> }
>
> +/*
> + * Return the L3 offset and protocol of an Ethernet frame starting at skb->data.
> + * The offset is unused without NEEDS_CSUM, so avoid parsing and return zero.
> + */
for networking code multiline comments:
/* always
* look like this
*/
/*
* never
* like this
*/
> +static inline int
> +virtio_net_hdr_eth_get_l3_offset(const struct sk_buff *skb,
> + const struct virtio_net_hdr *hdr,
> + __be16 *network_protocol)
> +{
> + unsigned int parse_depth = VLAN_MAX_DEPTH;
> + const struct ethhdr *eth;
> + struct ethhdr ethbuf;
> + __be16 protocol;
> + int depth = ETH_HLEN;
> +
> + *network_protocol = 0;
> + 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;
> + }
> +
> + *network_protocol = protocol;
> + 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,
> + __be16 network_protocol)
> {
> - 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;
> unsigned int ip_proto;
>
> + if (network_protocol == htons(ETH_P_IPV6))
> + nh_min_len = sizeof(struct ipv6hdr);
I'd prefer this inside the initializer:
unsigned int nh_min_len = (network_protocol == htons(ETH_P_IPV6))?
sizeof(struct ipv6hdr) : sizeof(struct iphdr);
> +
> if (hdr_gso_type != VIRTIO_NET_HDR_GSO_NONE) {
> switch (hdr_gso_type & ~VIRTIO_NET_HDR_GSO_ECN) {
> case VIRTIO_NET_HDR_GSO_TCPV4:
> @@ -98,16 +143,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 +255,12 @@ 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,
> + __be16 network_protocol)
From API POV, what is network_protocol here, exactly? I suspect it is
really af packet with IP protocol specific? So maybe it's really min_hdr
and then everyone except AF_PACKET can just pass 0? or do I
misunderstand?
> {
> - 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, network_protocol);
> }
>
> /* This function must be called after virtio_net_hdr_from_skb(). */
> @@ -287,7 +339,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 +349,20 @@ 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,
> + __be16 network_protocol)
> {
> 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, network_protocol);
>
> /* Tunnel not supported/negotiated, but the hdr asks for it. */
> if (!tnl_hdr_negotiated)
> @@ -332,19 +386,24 @@ 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);
> + if (network_protocol == htons(ETH_P_IPV6))
> + outer_l3min = sizeof(struct ipv6hdr);
So I vaguely understand that "network_protocol" is something af_packet
specific? because we already have inner_isv6 and outer_isv6
so this is neither? or do I misunderstand?
> 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, network_protocol);
> if (ret)
> return ret;
>
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index 50cae32ae..0b37d5474 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -2550,6 +2550,26 @@ static void tpacket_destruct_skb(struct sk_buff *skb)
> sock_wfree(skb);
> }
>
> +static int packet_vnet_hdr_to_skb(struct sk_buff *skb,
> + const struct virtio_net_hdr *vnet_hdr)
> +{
> + __be16 network_protocol = 0;
> + int network_offset;
> +
> + if (skb->dev->type == ARPHRD_ETHER) {
> + network_offset = virtio_net_hdr_eth_get_l3_offset(skb, vnet_hdr,
> + &network_protocol);
> + } else {
> + network_offset = skb_network_offset(skb);
> + network_protocol = skb->protocol;
> + }
> + if (network_offset < 0)
> + return network_offset;
> +
> + return virtio_net_hdr_to_skb(skb, vnet_hdr, vio_le(),
> + network_offset, network_protocol);
> +}
> +
> static int __packet_snd_vnet_parse(struct virtio_net_hdr *vnet_hdr, size_t len)
> {
> if ((vnet_hdr->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) &&
> @@ -2901,7 +2921,7 @@ 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 (packet_vnet_hdr_to_skb(skb, &vnet_hdr)) {
> tp_len = -EINVAL;
> goto tpacket_error;
> }
> @@ -3103,7 +3123,7 @@ 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 = packet_vnet_hdr_to_skb(skb, &vnet_hdr);
> if (err)
> goto out_free;
> len += vnet_hdr_sz;
>
> base-commit: 1e24c4f2ee44be0eee94092b5d13cbdb4bdf0d60
> --
> 2.46.0
^ permalink raw reply [flat|nested] 14+ messages in thread[parent not found: <CAHv8Y_4OKdVkigToBmXXhhUxu+EL2iWVADZJ=LS4CGqBATgU5g@mail.gmail.com>]
* Re: [PATCH net v6 1/2] net: validate virtio checksum start after network header
[not found] ` <CAHv8Y_4OKdVkigToBmXXhhUxu+EL2iWVADZJ=LS4CGqBATgU5g@mail.gmail.com>
@ 2026-09-22 5:55 ` Johannes Berg
0 siblings, 0 replies; 14+ messages in thread
From: Johannes Berg @ 2026-09-22 5:55 UTC (permalink / raw)
To: Paulos Yibelo, Michael S. Tsirkin
Cc: netdev, richard, anton.ivanov, 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 Tue, 2026-09-22 at 01:44 -0400, Paulos Yibelo wrote:
> If this sounds good to you I can get started on the modifications.
>
No it doesn't, go away and don't just keep sending LLM output without
ever even bothering to look at it.
johannes
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net v6 1/2] net: validate virtio checksum start after network header
2026-09-22 5:14 ` Michael S. Tsirkin
[not found] ` <CAHv8Y_4OKdVkigToBmXXhhUxu+EL2iWVADZJ=LS4CGqBATgU5g@mail.gmail.com>
@ 2026-09-22 5:56 ` Johannes Berg
2026-09-22 8:46 ` Michael S. Tsirkin
1 sibling, 1 reply; 14+ messages in thread
From: Johannes Berg @ 2026-09-22 5:56 UTC (permalink / raw)
To: Michael S. Tsirkin, Paulos Yibelo
Cc: netdev, richard, anton.ivanov, 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 Tue, 2026-09-22 at 01:14 -0400, Michael S. Tsirkin wrote:
>
> >
> > +/*
> > + * Return the L3 offset and protocol of an Ethernet frame starting at skb->data.
> > + * The offset is unused without NEEDS_CSUM, so avoid parsing and return zero.
> > + */
>
> for networking code multiline comments:
>
> /* always
> * look like this
> */
>
> /*
> * never
> * like this
> */
This, for the record, is't really true any more - we dropped the special
rule a few years ago.
johannes
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH net v6 1/2] net: validate virtio checksum start after network header
2026-09-22 5:56 ` Johannes Berg
@ 2026-09-22 8:46 ` Michael S. Tsirkin
0 siblings, 0 replies; 14+ messages in thread
From: Michael S. Tsirkin @ 2026-09-22 8:46 UTC (permalink / raw)
To: Johannes Berg
Cc: Paulos Yibelo, netdev, richard, anton.ivanov,
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 Tue, Sep 22, 2026 at 07:56:23AM +0200, Johannes Berg wrote:
> On Tue, 2026-09-22 at 01:14 -0400, Michael S. Tsirkin wrote:
> >
> > >
> > > +/*
> > > + * Return the L3 offset and protocol of an Ethernet frame starting at skb->data.
> > > + * The offset is unused without NEEDS_CSUM, so avoid parsing and return zero.
> > > + */
> >
> > for networking code multiline comments:
> >
> > /* always
> > * look like this
> > */
> >
> > /*
> > * never
> > * like this
> > */
>
> This, for the record, is't really true any more - we dropped the special
> rule a few years ago.
>
> johannes
Oh good to know. thanks!
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net v6 1/2] net: validate virtio checksum start after network header
2026-09-22 3:03 ` [PATCH net v6 1/2] net: validate virtio checksum start after network header Paulos Yibelo
2026-09-22 5:14 ` Michael S. Tsirkin
@ 2026-09-22 22:29 ` Willem de Bruijn
2026-09-23 1:27 ` Willem de Bruijn
2026-09-25 9:05 ` netdev-bot+sashiko
2 siblings, 1 reply; 14+ messages in thread
From: Willem de Bruijn @ 2026-09-22 22:29 UTC (permalink / raw)
To: Paulos Yibelo, netdev
Cc: 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
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 <habte.yibelo@gmail.com>
> Link: https://lore.kernel.org/netdev/20260920004733.6473-2-habte.yibelo@gmail.com/
> Cc: stable@vger.kernel.org
> Assisted-by: LLM
> Signed-off-by: Paulos Yibelo <habte.yibelo@gmail.com>
> ---
> 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.
--- a/drivers/net/tun_vnet.h
+++ b/drivers/net/tun_vnet.h
@@ -152,6 +152,9 @@ 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_is_little_endian(flags));
}
--- a/include/linux/virtio_net.h
+++ b/include/linux/virtio_net.h
@@ -71,9 +71,27 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb,
if (!pskb_may_pull(skb, needed))
return -EINVAL;
+ if (!skb_network_header_was_set(skb) ||
+ (skb->dev && skb->dev->type == ARPHRD_ETHER)) {
+ int nhoff = ETH_HLEN;
+
+ if (unlikely(start < ETH_HLEN + nh_min_len))
+ return -EINVAL;
+ __vlan_get_protocol(skb, eth_hdr(skb)->h_proto, &nhoff);
+ nh_min_len += nhoff;
+ } else {
+ nh_min_len += skb_network_offset(skb);
+ }
+ if (unlikely(start < nh_min_len))
+ return -EINVAL;
+
+ const struct iphdr *iph = (void *)(skb->data + nh_min_len - sizeof(*iph));
+ if (iph->version == 4)
+ nh_min_len += max_t(u32, iph->ihl * 4, sizeof(*iph)) - sizeof(*iph);
+ else if (iph->version == 6)
/* ..here I don't trust the initial bug output, but the branch is clear.. */
+ if (unlikely(start < nh_min_len))
+ return -EINVAL;
+
if (!skb_partial_csum_set(skb, start, off))
return -EINVAL;
- if (skb_transport_offset(skb) < nh_min_len)
- return -EINVAL;
}
--- a/net/ipv4/ip_output.c
+++ b/net/ipv4/ip_output.c
@@ -772,7 +772,9 @@ int ip_do_fragment(struct net *net, struct sock *sk, struct sk_buff *skb,
if (skb->ip_summed == CHECKSUM_PARTIAL &&
(!(dev->features & NETIF_F_CSUM_MASK) ||
- skb_checksum_help(skb)))
+ skb_checksum_start_offset(skb) < (int)(iph->ihl * 4) ||
+ skb_checksum_help(skb)))
goto fail;
And the summary of the problem:
### 1. What the Problem Is
When userspace (AF_PACKET, TUN/TAP, vhost-net) or a device (virtio_net, UML) injects a CHECKSUM_PARTIAL packet using struct virtio_net_hdr,
virtio_net.h:47-119 validates hdr->csum_start:
// include/linux/virtio_net.h:107 (added by commit 49d14b54a527)
if (skb_transport_offset(skb) < nh_min_len)
return -EINVAL;
This validation has three bugs:
1. Missing L2 + VLAN offset: skb_transport_offset(skb) (hdr->csum_start) is an offset from skb->data, which points to the L2 header (14
bytes for Ethernet + 4 * n bytes for 802.1Q/802.1ad VLAN tags) on Ethernet callers (virtio_net, IFF_TAP, PACKET_SOCK_DGRAM/SOCK_RAW).
Comparing csum_start < 20 allows csum_start to land inside the L2 VLAN tags (14..21) or inside the L3 header (22..33).
2. Wrong nh_min_len for IPv6 non-GSO: nh_min_len defaults to sizeof(struct iphdr) (20) and is only raised to sizeof(struct ipv6hdr) (40)
when gso_type is VIRTIO_NET_HDR_GSO_TCPV6. A non-GSO or GSO_UDP_L4 IPv6 packet is only checked against 20 bytes.
3. Ignores IPv4 options (iph->ihl > 5): Even without an L2 header (IFF_TUN), an IPv4 header with options can be up to 60 bytes (ihl = 15),
allowing csum_start = 20 to point 40 bytes inside the IPv4 options area.
This causes two kernel bugs downstream:
• Bug A (WARN_ONCE / panic_on_warn in skb_checksum_help): With 2 VLAN tags (22 bytes L2), csum_start = 20 passes 20 >= 20. Once
eth_type_trans() + skb_vlan_untag() pull 22 bytes, skb_checksum_start_offset(skb) (csum_start - skb_headroom(skb)) becomes -2. In
dev.c:3645, offset >= skb_headlen(skb) promotes signed -2 to 0xfffffffeU, firing WARN_ONCE(1, ...) and crashing panic_on_warn=1 hosts.
• Bug B (TOCTOU L3 Header Corruption -> OOB Read in ip_do_fragment): With csum_start = 20, csum_offset = 0 on TAP/AF_PACKET, csum_start
lands at byte 6 of struct iphdr (frag_off) or byte 0 (version/ihl on double-VLAN frames). In ip_output.c:774 and nf_conntrack_bridge.c:42,
skb_checksum_help(skb) runs before hlen = iph->ihl * 4 is read. The 16-bit checksum write corrupts iph->ihl (e.g. from 5 [20B] to 15 [60B])
after ip_rcv_core() already validated it, causing ip_do_fragment() to read 60 bytes out-of-bounds from skb->data.
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH net v6 1/2] net: validate virtio checksum start after network header
2026-09-22 22:29 ` Willem de Bruijn
@ 2026-09-23 1:27 ` Willem de Bruijn
2026-09-23 10:21 ` Michael S. Tsirkin
0 siblings, 1 reply; 14+ messages in thread
From: Willem de Bruijn @ 2026-09-23 1:27 UTC (permalink / raw)
To: Willem de Bruijn, Paulos Yibelo, netdev
Cc: 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
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 <habte.yibelo@gmail.com>
> > Link: https://lore.kernel.org/netdev/20260920004733.6473-2-habte.yibelo@gmail.com/
> > Cc: stable@vger.kernel.org
> > Assisted-by: LLM
> > Signed-off-by: Paulos Yibelo <habte.yibelo@gmail.com>
> > ---
> > 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;
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH net v6 1/2] net: validate virtio checksum start after network header
2026-09-23 1:27 ` Willem de Bruijn
@ 2026-09-23 10:21 ` Michael S. Tsirkin
2026-09-23 10:46 ` Eric Dumazet
0 siblings, 1 reply; 14+ messages in thread
From: Michael S. Tsirkin @ 2026-09-23 10:21 UTC (permalink / raw)
To: Willem de Bruijn
Cc: Paulos Yibelo, netdev, richard, anton.ivanov, johannes,
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 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 <habte.yibelo@gmail.com>
> > > Link: https://lore.kernel.org/netdev/20260920004733.6473-2-habte.yibelo@gmail.com/
> > > Cc: stable@vger.kernel.org
> > > Assisted-by: LLM
> > > Signed-off-by: Paulos Yibelo <habte.yibelo@gmail.com>
> > > ---
> > > 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 seems likely that not all drivers validate headers defensively,
and incoming packets can easily become outgoing ones.
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?
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
- future protocol extensions that now will require surgery in
virtio, instead of just being passed through to the host
What do others think?
--
MST
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH net v6 1/2] net: validate virtio checksum start after network header
2026-09-23 10:21 ` Michael S. Tsirkin
@ 2026-09-23 10:46 ` Eric Dumazet
2026-09-24 2:02 ` Michael S. Tsirkin
0 siblings, 1 reply; 14+ messages in thread
From: Eric Dumazet @ 2026-09-23 10:46 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Willem de Bruijn, Paulos Yibelo, netdev, richard, anton.ivanov,
johannes, jasowangio, eperezma, xuanzhuo, andrew+netdev, pablo,
fw, phil, razor, idosch, dsahern, davem, kuba, pabeni, horms,
linux-um, virtualization, netfilter-devel, coreteam, bridge,
linux-kernel
On Wed, Sep 23, 2026 at 12:21 PM Michael S. Tsirkin <mst@redhat.com> 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 <habte.yibelo@gmail.com>
> > > > Link: https://lore.kernel.org/netdev/20260920004733.6473-2-habte.yibelo@gmail.com/
> > > > Cc: stable@vger.kernel.org
> > > > Assisted-by: LLM
> > > > Signed-off-by: Paulos Yibelo <habte.yibelo@gmail.com>
> > > > ---
> > > > 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
>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH net v6 1/2] net: validate virtio checksum start after network header
2026-09-23 10:46 ` Eric Dumazet
@ 2026-09-24 2:02 ` Michael S. Tsirkin
2026-09-24 17:18 ` Jakub Kicinski
0 siblings, 1 reply; 14+ messages in thread
From: Michael S. Tsirkin @ 2026-09-24 2:02 UTC (permalink / raw)
To: Eric Dumazet
Cc: Willem de Bruijn, Paulos Yibelo, netdev, richard, anton.ivanov,
johannes, jasowangio, eperezma, xuanzhuo, andrew+netdev, pablo,
fw, phil, razor, idosch, dsahern, davem, kuba, pabeni, horms,
linux-um, virtualization, netfilter-devel, coreteam, bridge,
linux-kernel
On Wed, Sep 23, 2026 at 12:46:42PM +0200, Eric Dumazet wrote:
> On Wed, Sep 23, 2026 at 12:21 PM Michael S. Tsirkin <mst@redhat.com> 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 <habte.yibelo@gmail.com>
> > > > > Link: https://lore.kernel.org/netdev/20260920004733.6473-2-habte.yibelo@gmail.com/
> > > > > Cc: stable@vger.kernel.org
> > > > > Assisted-by: LLM
> > > > > Signed-off-by: Paulos Yibelo <habte.yibelo@gmail.com>
> > > > > ---
> > > > > 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.
Thanks for the explanation Eric!
>
> > 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.
Well:
$ git grep 'skb->csum_start\ ='
drivers/net/ethernet/hisilicon/hns3/hns3_enet.c: skb->csum_start = (unsigned char *)th - skb->head;
drivers/net/ethernet/mellanox/mlx5/core/en_rx.c: skb->csum_start = (unsigned char *)uh - skb->head;
drivers/net/ethernet/mellanox/mlx5/core/en_rx.c: skb->csum_start = (unsigned char *)uh - skb->head;
drivers/net/ethernet/mellanox/mlx5/core/en_rx.c: skb->csum_start = (unsigned char *)tcp - skb->head;
drivers/net/ethernet/mellanox/mlx5/core/en_rx.c: skb->csum_start = (unsigned char *)tcp - skb->head;
drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c: skb->csum_start = skb_transport_header(skb) - skb->head;
What did I miss?
>
> > 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
> >
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH net v6 1/2] net: validate virtio checksum start after network header
2026-09-24 2:02 ` Michael S. Tsirkin
@ 2026-09-24 17:18 ` Jakub Kicinski
0 siblings, 0 replies; 14+ messages in thread
From: Jakub Kicinski @ 2026-09-24 17:18 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Eric Dumazet, Willem de Bruijn, Paulos Yibelo, netdev, richard,
anton.ivanov, johannes, jasowangio, eperezma, xuanzhuo,
andrew+netdev, pablo, fw, phil, razor, idosch, dsahern, davem,
pabeni, horms, linux-um, virtualization, netfilter-devel,
coreteam, bridge, linux-kernel
On Wed, 23 Sep 2026 22:02:57 -0400 Michael S. Tsirkin wrote:
> $ git grep 'skb->csum_start\ ='
> drivers/net/ethernet/hisilicon/hns3/hns3_enet.c: skb->csum_start = (unsigned char *)th - skb->head;
> drivers/net/ethernet/mellanox/mlx5/core/en_rx.c: skb->csum_start = (unsigned char *)uh - skb->head;
> drivers/net/ethernet/mellanox/mlx5/core/en_rx.c: skb->csum_start = (unsigned char *)uh - skb->head;
> drivers/net/ethernet/mellanox/mlx5/core/en_rx.c: skb->csum_start = (unsigned char *)tcp - skb->head;
> drivers/net/ethernet/mellanox/mlx5/core/en_rx.c: skb->csum_start = (unsigned char *)tcp - skb->head;
These are HW-GRO, ASIC coalesced TCP segments so it better know
the _correct_ offsets of things. And it's hardly remote-provided?
> drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c: skb->csum_start = skb_transport_header(skb) - skb->head;
selftest
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net v6 1/2] net: validate virtio checksum start after network header
2026-09-22 3:03 ` [PATCH net v6 1/2] net: validate virtio checksum start after network header Paulos Yibelo
2026-09-22 5:14 ` Michael S. Tsirkin
2026-09-22 22:29 ` Willem de Bruijn
@ 2026-09-25 9:05 ` netdev-bot+sashiko
2 siblings, 0 replies; 14+ messages in thread
From: netdev-bot+sashiko @ 2026-09-25 9:05 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 4 potential
issue(s) to consider.
Critical: 0 · High: 0 · Medium: 1 · Low: 3
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).
- [Medium] Undisclosed tightening of an existing uAPI in a stable-tagged
patch.
- [Low] Commit message does not describe the actual defect and states no
user-visible symptom for a Fixes:/Cc: stable patch.
- [Low] The new Ethernet/VLAN parser invoked from raw_verify_header()
(arch/um/drivers/vector_transports.c) treats the skb's current length…
- [Low] Reported for completeness, but the claimed bypass does not reach
the asserted malformed geometry.
--
net: validate virtio checksum start after network header
This passes a data-relative L3 offset and L3 protocol into
__virtio_net_hdr_to_skb() and virtio_net_hdr_tnl_to_skb(), adds an
Ethernet/VLAN parser (virtio_net_hdr_eth_get_l3_offset()) used by
AF_PACKET, tap/tun, virtio_net and the UML vector raw transport, and
tightens the CHECKSUM_PARTIAL minimum-header check accordingly.
Link: https://lore.kernel.org/netdev/20260920004733.6473-2-habte.yibelo@gmail.com/
A few questions below.
The changelog says:
> __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.
[Severity: Low]
Is this the actual defect? The pre-patch test in __virtio_net_hdr_to_skb()
is:
if (skb_transport_offset(skb) < nh_min_len)
return -EINVAL;
nh_min_len = skb_transport_offset(skb);
which never reads skb->network_header, so an unestablished network header
does not affect it.
What the old test misses looks different: the minimum is measured from
skb->data, which for the Ethernet-framed callers (AF_PACKET, tap,
virtio_net, UML raw/tap) is the MAC header, so a csum_start of 20..33 passes
while leaving fewer than 20 bytes of L3 header.
Also, for AF_PACKET the caller does establish the network header before
conversion:
net/packet/af_packet.c:packet_parse_headers() {
...
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);
}
Could the changelog describe the data-relative minimum instead of the
network header, and also state the user-visible symptom? With two Fixes:
tags and Cc: stable, and Reported-by: naming the author, there is no
reproducer or symptom given. The referenced commit 49d14b54a527 documented
its own symptom (the "offset (-6) >= skb_headlen() (14)" warning from
skb_checksum_help() via ip_do_fragment()); is the same warning, a
mis-placed checksum, or something else being fixed here?
> diff --git a/arch/um/drivers/vector_transports.c b/arch/um/drivers/vector_transports.c
> index ddd127ee96785..5a9d8f49a64ba 100644
> --- a/arch/um/drivers/vector_transports.c
> +++ b/arch/um/drivers/vector_transports.c
> @@ -197,6 +197,8 @@ 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;
> + __be16 network_protocol;
> + int network_offset;
>
> if ((vheader->gso_type != VIRTIO_NET_HDR_GSO_NONE) &&
> (vp->req_size != 65536)) {
> @@ -209,8 +211,14 @@ 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_eth_get_l3_offset(skb, vheader,
> + &network_protocol);
> + if (network_offset < 0)
> + return network_offset;
[Severity: Low]
Does this parse bytes that have not been received yet? Both UML receive
callers run verify_header() before the skb is trimmed to the recvmsg
result, and prep_skb() has already set the length to the full buffer
capacity:
arch/um/drivers/vector_kern.c:prep_skb() {
...
skb_put(result, vp->max_packet);
result->data_len = len - vp->max_packet;
result->len += len - vp->max_packet;
...
}
arch/um/drivers/vector_kern.c:vector_legacy_rx() {
...
header_check = vp->verify_header(vp->header_rxbuffer, skb, vp);
...
pskb_trim(skb, pkt_len - vp->rx_header_size);
...
}
vector_mmsg_rx() has the same ordering.
virtio_net_hdr_eth_get_l3_offset() bounds its reads with
skb_header_pointer(), i.e. against skb_headlen(), so for a NEEDS_CSUM frame
that was truncated inside its Ethernet or nested VLAN headers the parser
reads stale bytes of the freshly allocated head rather than reporting a
short header. The resulting network_offset/network_protocol, and now the
return value of raw_verify_header(), then come from uninitialized buffer
contents.
Would it make sense for the callers to establish the received length before
calling verify_header()?
> +
> + return virtio_net_hdr_to_skb(skb, vheader,
> + virtio_legacy_is_little_endian(),
> + network_offset, network_protocol);
> }
>
> static bool get_uint_param(
[ ... ]
> diff --git a/drivers/net/tun_vnet.h b/drivers/net/tun_vnet.h
> index f4c652b1fa44d..e004213fad22c 100644
> --- a/drivers/net/tun_vnet.h
> +++ b/drivers/net/tun_vnet.h
> @@ -177,10 +177,49 @@ 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,
> + __be16 *network_protocol)
> +{
> + if ((flags & TUN_TYPE_MASK) != IFF_TAP) {
> + u8 version, first_byte;
> + const u8 *first;
> +
> + *network_protocol = 0;
> + if (!(hdr->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM))
> + return 0;
> +
> + first = skb_header_pointer(skb, 0, sizeof(first_byte),
> + &first_byte);
> + if (!first)
> + return -EINVAL;
> +
> + version = *first >> 4;
> + if (version == 4)
> + *network_protocol = htons(ETH_P_IP);
> + else if (version == 6)
> + *network_protocol = htons(ETH_P_IPV6);
> + return 0;
> + }
[Severity: Low]
For IFF_TUN this derives network_protocol from the payload version nibble,
while skb->protocol is set later from the user-supplied tun_pi.proto:
drivers/net/tun.c:tun_get_user() {
...
case IFF_TUN:
if (tun->flags & IFF_NO_PI) {
...
ip_version = skb->data[0] >> 4;
...
}
skb_reset_mac_header(skb);
skb->protocol = pi.proto;
...
}
The nibble/pi.proto consistency check only runs under IFF_NO_PI, so a
writer can declare pi.proto = ETH_P_IPV6 with a first nibble of 4, keeping
nh_min_len at sizeof(struct iphdr) while the stack labels the skb IPv6.
That particular combination looks harmless in practice, since
ip6_rcv_core() drops it before any header parsing:
net/ipv6/ip6_input.c:ip6_rcv_core() {
...
if (hdr->version != 6) {
SKB_DR_SET(reason, UNHANDLED_PROTO);
goto err;
}
...
}
and a genuine IPv6 payload always yields nibble 6 and thus the intended
40-byte minimum. Still, is keying the length bound off the payload nibble
while the rest of the stack acts on pi.proto intentional here?
> +
> + return virtio_net_hdr_eth_get_l3_offset(skb, hdr,
> + network_protocol);
> +}
> +
[ ... ]
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index e34c52d059d39..8e93dad287911 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -2515,9 +2517,13 @@ 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_eth_get_l3_offset(skb, &hdr->hdr,
> + &network_protocol);
> + 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, network_protocol)) {
> 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 c381b916c1b54..b89d821a0dd2d 100644
> --- a/include/linux/virtio_net.h
> +++ b/include/linux/virtio_net.h
> @@ -48,16 +48,61 @@ static inline int virtio_net_hdr_set_proto(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, u8 hdr_gso_type)
> + bool little_endian, u8 hdr_gso_type,
> + int network_offset,
> + __be16 network_protocol)
> {
> - 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;
> unsigned int ip_proto;
>
> + if (network_protocol == htons(ETH_P_IPV6))
> + nh_min_len = sizeof(struct ipv6hdr);
> +
[ ... ]
> @@ -98,16 +143,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;
[Severity: Medium]
Does this change the accepted csum_start range for existing users, and
should the changelog say so?
The floor moves from 20 (relative to skb->data) to parsed_L3_offset + 20 or
40: 34 for a plain Ethernet frame, 54 for Ethernet plus IPv6, and more with
VLAN or QinQ.
Three concrete behaviour changes:
- AF_PACKET sendmsg() and tpacket_snd() now return -EINVAL for virtio
headers with csum_start in 20..33 that were accepted before.
- The new
if (network_protocol == htons(ETH_P_IPV6))
nh_min_len = sizeof(struct ipv6hdr);
applies the 40-byte minimum to any frame whose parsed ethertype is
ETH_P_IPV6, whereas previously only VIRTIO_NET_HDR_GSO_TCPV6 did that.
- On the receive side, virtnet_receive_done() now takes the frame_err path
(DEV_STATS_INC(rx_frame_errors) plus dev_kfree_skb) for host descriptors
that were accepted before.
There is also the non-IP case: when the parsed ethertype is neither
ETH_P_IPV4 nor ETH_P_IPV6, nh_min_len stays sizeof(struct iphdr), so a
non-IP Ethernet frame whose checksum field sits close to the L2 header is
rejected even though it has no 20-byte L3 header at all. Is that
intended?
Given the Cc: stable, would it be worth stating the new accept/reject
boundary in the changelog and adding a selftest that pins it?
> - 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;
[ ... ]
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index 50cae32ae2692..0b37d5474f868 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -3103,7 +3123,7 @@ 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 = packet_vnet_hdr_to_skb(skb, &vnet_hdr);
> if (err)
> goto out_free;
> len += vnet_hdr_sz;
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922030310.8684-1-habte.yibelo%40gmail.com
^ permalink raw reply [flat|nested] 14+ messages in thread