Re: [PATCH net] net: always dissect GSO packets in __virtio_net_hdr_to_skb()

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

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), 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>

Reviewed-by: Willem de Bruijn <willemb@google.com>

Thanks Eric.