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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox