From: "Michael S. Tsirkin" <mst@redhat.com>
To: Eric Dumazet <edumazet@google.com>
Cc: "David S . Miller" <davem@davemloft.net>,
Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
Simon Horman <horms@kernel.org>,
netdev@vger.kernel.org, edumazet@kernel.org,
Weiming Shi <bestswngs@gmail.com>,
Willem de Bruijn <willemb@google.com>,
Jason Wang <jasowang@redhat.com>
Subject: Re: [PATCH net] net: always dissect GSO packets in __virtio_net_hdr_to_skb()
Date: Sun, 27 Sep 2026 21:31:14 -0400 [thread overview]
Message-ID: <20260927212942-mutt-send-email-mst@kernel.org> (raw)
In-Reply-To: <20260927195536.2489079-1-edumazet@google.com>
On Sun, Sep 27, 2026 at 07:55:36PM +0000, Eric Dumazet wrote:
> Commit 9e8db5913264 ("net: avoid false positives in untrusted gso
> validation") added a '&& skb->network_header' check before flow-dissecting
> GSO packets without VIRTIO_NET_HDR_F_NEEDS_CSUM in
> __virtio_net_hdr_to_skb(), because some callers (such as tun_get_user(),
> tun_xdp_one(), virtnet_receive_done(), and raw_verify_header()) called
> virtio_net_hdr_*_to_skb() before initializing skb->network_header and
> skb->dev.
>
> However, skb->network_header is an offset from skb->head, not a boolean
> flag. When skb_headroom(skb) is 0 on a device without L2 headers (for
> instance packet_snd() or tpacket_snd() on a tunnel/pure-L3 device where
> LL_RESERVED_SPACE_EX(dev, 0) == 0),
Hmm. I have:
LL_RESERVED_SPACE_EX(dev, 0)
((((hlen) + READ_ONCE((dev)->needed_headroom)) \
& ~(HH_DATA_MOD - 1)) + HH_DATA_MOD)
#define HH_DATA_MOD 16
So how can LL_RESERVED_SPACE_EX(dev, 0) == 0 ?
> skb_reset_network_header(skb)
> legitimately sets skb->network_header to 0.
>
> Whenever the 'if (gso_type && skb->network_header)' branch was skipped,
> the fallback 'else if (gso_type)' only pulled nh_min_len + thlen (40 bytes
> for TCPv4) without dissecting the packet, without validating ip_proto or
> n_proto, and without setting skb->transport_header. If an IPv4 packet
> carries IP options (ihl > 5, up to 60 bytes) or an IPv6 packet carries
> extension headers, pulling only nh_min_len + thlen leaves the TCP header
> outside skb->head, causing tcp_hdrlen(skb) in skb_gso_transport_seglen()
> to read out-of-bounds:
>
> BUG: KASAN: slab-out-of-bounds in skb_gso_transport_seglen+0x173/0x200 net/core/skbuff.c:5503
> Read of size 1 at addr ffff888103c7ec4d by task repro/5718
> Call Trace:
> <TASK>
> skb_gso_transport_seglen+0x173/0x200 net/core/skbuff.c:5503
> skb_gso_network_seglen include/linux/skbuff.h:4718 [inline]
> skb_gso_validate_network_len+0x92/0x1a0 net/core/skbuff.c:5566
> ip_finish_output_gso net/ipv4/ip_output.c:285 [inline]
> __ip_finish_output+0x28b/0x4a0 net/ipv4/ip_output.c:316
>
> In addition, when skb->protocol is pre-set by a caller before
> __virtio_net_hdr_to_skb(), 'if (!skb->protocol)' is skipped and
> virtio_net_hdr_match_proto() was not checked.
>
> Fix this by:
> 1. Initializing skb->dev and skb->network_header (plus skb->protocol for
> IFF_TUN) before virtio_net_hdr_*_to_skb() in tun_get_user(),
> tun_xdp_one(), virtnet_receive_done(), and raw_verify_header().
> 2. Removing '&& skb->network_header' and the unvalidated
> 'else if (gso_type)' fallback in __virtio_net_hdr_to_skb() so all GSO
> packets without VIRTIO_NET_HDR_F_NEEDS_CSUM are flow-dissected, have
> their transport header pulled into linear data, and have
> skb->transport_header set.
> 3. Validating virtio_net_hdr_match_proto(keys.basic.n_proto, hdr_gso_type)
> after skb_flow_dissect_flow_keys_basic().
>
> Fixes: 9e8db5913264 ("net: avoid false positives in untrusted gso validation")
> Fixes: d5be7f632bad ("net: validate untrusted gso packets without csum offload")
> Fixes: 924a9bc362a5 ("net: check if protocol extracted by virtio_net_hdr_set_proto is correct")
> Reported-by: Weiming Shi <bestswngs@gmail.com>
> Closes: https://lore.kernel.org/netdev/20260927163117.746432-2-bestswngs@gmail.com/
> Assisted-by: LLM
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Willem de Bruijn <willemb@google.com>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> ---
> arch/um/drivers/vector_transports.c | 1 +
> drivers/net/tun.c | 23 ++++++----
> drivers/net/virtio_net.c | 2 +
> include/linux/virtio_net.h | 66 ++++++++++++++---------------
> 4 files changed, 48 insertions(+), 44 deletions(-)
>
> diff --git a/arch/um/drivers/vector_transports.c b/arch/um/drivers/vector_transports.c
> index ddd127ee96785daa4c485b2a06f078686efd2046..e1fb2a76fdff59ef3032d091be06a06fa58046cc 100644
> --- a/arch/um/drivers/vector_transports.c
> +++ b/arch/um/drivers/vector_transports.c
> @@ -209,6 +209,7 @@ static int raw_verify_header(
> if ((vheader->flags & VIRTIO_NET_HDR_F_DATA_VALID) > 0)
> return 1;
>
> + skb_set_network_header(skb, ETH_HLEN);
> virtio_net_hdr_to_skb(skb, vheader, virtio_legacy_is_little_endian());
> return 0;
> }
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 5a302709a68aa3308b0c850b4a2957df3260b352..242899f7fd0711c5c59ff85accbf5bd1be9c6f39 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -1897,12 +1897,7 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
> }
> }
>
> - if (tun_vnet_hdr_tnl_to_skb(tun->flags, features, skb, &hdr)) {
> - atomic_long_inc(&tun->rx_frame_errors);
> - err = -EINVAL;
> - goto free_skb;
> - }
> -
> + skb->dev = tun->dev;
> switch (tun->flags & TUN_TYPE_MASK) {
> case IFF_TUN:
> if (tun->flags & IFF_NO_PI) {
> @@ -1927,9 +1922,8 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
> }
> }
>
> - skb_reset_mac_header(skb);
> + skb_reset_network_header(skb);
> skb->protocol = pi.proto;
> - skb->dev = tun->dev;
> break;
> case IFF_TAP:
> if (!pskb_may_pull(skb, ETH_HLEN)) {
> @@ -1937,10 +1931,19 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
> drop_reason = SKB_DROP_REASON_HDR_TRUNC;
> goto drop;
> }
> - skb->protocol = eth_type_trans(skb, tun->dev);
> + skb_set_network_header(skb, ETH_HLEN);
> break;
> }
>
> + if (tun_vnet_hdr_tnl_to_skb(tun->flags, features, skb, &hdr)) {
> + atomic_long_inc(&tun->rx_frame_errors);
> + err = -EINVAL;
> + goto free_skb;
> + }
> +
> + if ((tun->flags & TUN_TYPE_MASK) == IFF_TAP)
> + skb->protocol = eth_type_trans(skb, tun->dev);
> +
> /* copy skb_ubuf_info for callback when skb has no error */
> if (zerocopy) {
> skb_zcopy_init(skb, msg_control);
> @@ -2600,6 +2603,8 @@ static int tun_xdp_one(struct tun_struct *tun,
>
> features = tun_vnet_hdr_guest_features(READ_ONCE(tun->vnet_hdr_sz));
> tnl_hdr = (struct virtio_net_hdr_v1_hash_tunnel *)gso;
> + skb->dev = tun->dev;
> + skb_set_network_header(skb, ETH_HLEN);
> if (tun_vnet_hdr_tnl_to_skb(tun->flags, features, skb, tnl_hdr)) {
> atomic_long_inc(&tun->rx_frame_errors);
> kfree_skb(skb);
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index bf82ef9874abb4094931496fb6064a12ee75b789..daab43ac92ce4276b0f8f88684991e3993022ce6 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -2515,6 +2515,8 @@ static void virtnet_receive_done(struct virtnet_info *vi, struct receive_queue *
> goto frame_err;
> }
>
> + skb->dev = dev;
> + skb_set_network_header(skb, ETH_HLEN);
> if (virtio_net_hdr_tnl_to_skb(skb, &hdr->tnl_hdr, vi->rx_tnl,
> vi->rx_tnl_csum,
> virtio_is_little_endian(vi->vdev))) {
> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> index c381b916c1b54afacba5473888af589a24fe087c..8902d5a5c418c46af0a6e7b26ae89948c20b0b54 100644
> --- a/include/linux/virtio_net.h
> +++ b/include/linux/virtio_net.h
> @@ -111,48 +111,44 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb,
> p_off = nh_min_len + thlen;
> if (!pskb_may_pull(skb, p_off))
> return -EINVAL;
> - } else {
> + } else if (gso_type) {
> /* gso packets without NEEDS_CSUM do not set transport_offset.
> * probe and drop if does not match one of the above types.
> */
> - if (gso_type && skb->network_header) {
> - struct flow_keys_basic keys;
> -
> - if (!skb->protocol) {
> - __be16 protocol = dev_parse_header_protocol(skb);
> -
> - if (!protocol)
> - virtio_net_hdr_set_proto(skb, hdr);
> - else if (!virtio_net_hdr_match_proto(protocol,
> - hdr_gso_type))
> - return -EINVAL;
> - else
> - skb->protocol = protocol;
> - }
> -retry:
> - if (!skb_flow_dissect_flow_keys_basic(NULL, skb, &keys,
> - NULL, 0, 0, 0,
> - 0)) {
> - /* UFO does not specify ipv4 or 6: try both */
> - if (gso_type & SKB_GSO_UDP &&
> - skb->protocol == htons(ETH_P_IP)) {
> - skb->protocol = htons(ETH_P_IPV6);
> - goto retry;
> - }
> - return -EINVAL;
> - }
> + struct flow_keys_basic keys;
>
> - p_off = keys.control.thoff + thlen;
> - if (!pskb_may_pull(skb, p_off) ||
> - keys.basic.ip_proto != ip_proto)
> - return -EINVAL;
> + if (!skb->protocol) {
> + __be16 protocol = dev_parse_header_protocol(skb);
>
> - skb_set_transport_header(skb, keys.control.thoff);
> - } else if (gso_type) {
> - p_off = nh_min_len + thlen;
> - if (!pskb_may_pull(skb, p_off))
> + if (!protocol)
> + virtio_net_hdr_set_proto(skb, hdr);
> + else if (!virtio_net_hdr_match_proto(protocol,
> + hdr_gso_type))
> return -EINVAL;
> + else
> + skb->protocol = protocol;
> }
> +retry:
> + if (!skb_flow_dissect_flow_keys_basic(NULL, skb, &keys,
> + NULL, 0, 0, 0,
> + 0)) {
> + /* UFO does not specify ipv4 or 6: try both */
> + if (gso_type & SKB_GSO_UDP &&
> + skb->protocol == htons(ETH_P_IP)) {
> + skb->protocol = htons(ETH_P_IPV6);
> + goto retry;
> + }
> + return -EINVAL;
> + }
> +
> + p_off = keys.control.thoff + thlen;
> + if (!pskb_may_pull(skb, p_off) ||
> + keys.basic.ip_proto != ip_proto ||
> + !virtio_net_hdr_match_proto(keys.basic.n_proto,
> + hdr_gso_type))
> + return -EINVAL;
> +
> + skb_set_transport_header(skb, keys.control.thoff);
> }
>
> if (hdr_gso_type != VIRTIO_NET_HDR_GSO_NONE) {
> --
> 2.56.0.rc1.315.gc6ed9934b7-goog
next prev parent reply other threads:[~2026-09-28 1:31 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-09-27 19:55 [PATCH net] net: always dissect GSO packets in __virtio_net_hdr_to_skb() Eric Dumazet
2026-09-27 20:33 ` Michael S. Tsirkin
2026-09-27 22:11 ` Eric Dumazet
2026-09-28 0:54 ` Willem de Bruijn
2026-09-28 1:31 ` Michael S. Tsirkin [this message]
2026-09-28 6:22 ` Eric Dumazet
2026-09-28 6:31 ` Eric Dumazet
2026-09-28 8:10 ` Michael S. Tsirkin
2026-09-28 10:35 ` Eric Dumazet
2026-09-29 3:55 ` netdev-bot+sashiko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20260927212942-mutt-send-email-mst@kernel.org \
--to=mst@redhat.com \
--cc=bestswngs@gmail.com \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=edumazet@kernel.org \
--cc=horms@kernel.org \
--cc=jasowang@redhat.com \
--cc=kuba@kernel.org \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=willemb@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.