Re: [PATCH net v5] net: use skb_header_pointer() only for DODGY TCPv4 GSO skbs

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

Guoyu Su wrote:
> Thanks Willem, this is a good point.
> 
> I reran with instrumentation at two exact points:
> 1) packet_snd(), immediately after virtio_net_hdr_to_skb() returns
>    (net/packet/af_packet.c)
> 2) gso_features_check(), in the SKB_GSO_DODGY branch before frag_off access
>    (net/core/dev.c)
> 
> For the same skb, I consistently see (first 20 dumps):
> - pkt_after_vnet:
>   skb=... len=56584 headlen=172 data_len=56412 netoff=172 transoff=88
> gso_type=0x3
>   skb_dump: headroom=4, mac=(4,172), trans=92
> - gso_dodgy:
>   skb=... nhoff=172 headlen=172 netoff=172 transoff=88
> 
> So in this run, coverage up to p_off on the transport-side does not imply
> safe direct access at nhoff on the network-side (nhoff/headlen are
> both 172 here).

Perhaps you're running a different repro from the one I used. Which is
the C repro from the run at commit ca4ee40bf13d.

I see that the virtio_net_hdr has hdr_len 106 and csum_start 88. Those
are fine. Same for your repro?

The question is how skb->network_header can be greater than
skb->transport_header right after virtio_net_hdr_to_skb. And whether
this can be a sanity test to drop clearly malformed packets.

E.g.,

	@@ -105,8 +108,12 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb,
				return -EINVAL;
			if (skb_transport_offset(skb) < nh_min_len)
				return -EINVAL;
	+               if (skb_transport_offset(skb) < skb_network_offset(skb) + nh_min_len)
	+                       return -EINVAL;


As far as I can see network_header is set entirely in packet_snd, not
updated in virtio_net_hdr_to_skb in this path.

It seems that hard_header_len for this device is 76. That is part of
the answer. It is an ip6gretap device, so this is the encap hlen.

> I agree that validating/dropping malformed packets as early as possible in
> virtio_net_hdr_to_skb() would be preferable if we can make that check precise.
> This patch addresses the observed safety gap at gso_features_check() for DODGY
> packets in the current path.
> 
> If helpful, I can share more skb_dump snippets / full serial log.

Friendly reminder to not top post

https://docs.kernel.org/process/submitting-patches.html#use-trimmed-interleaved-replies-in-email-discussions

> Willem de Bruijn <willemdebruijn.kernel@gmail.com> 于2026年3月22日周日 04:58写道：
> >
> > Scars wrote:
> > > I instrumented packet_snd(), __virtio_net_hdr_to_skb(), and
> > > gso_features_check() while running the C repro.
> > >
> > > In repeated runs, for the same skb, I consistently observed:
> > > - __virtio_net_hdr_to_skb() (NEEDS_CSUM path):
> > > skb_transport_offset=88, thlen=20, so p_off=108;
> > > pskb_may_pull(..., 108)
> >
> > All the above matches the skb_dump from my previous post.
> >
> > > succeeds (headlen=172).
> >
> > My output shows headlen 108. Here we start to diverge.
> >
> > > - gso_features_check() on the resulting DODGY TCPv4 skb uses
> > > nhoff=skb_network_offset(skb)=172.
> >
> > And I see headroom of 4, so mac at 4, skb->network_header at 80 and
> > skb->transport_header at 92. No 172.
> >
> > That part is key. My measurement is in packet_snd right after
> > virtio_net_hdr_to_skb. Where do you see this, and can you perhaps get
> > an skb_dump (NOT full_skb, as these are large, just the header
> > metadata).
> >
> > I don't mean to delay the fix. Just, in general, a preferable fix for
> > these weird user injected packets is to detect and drop as close to
> > kerne entry as possible, meaning in virtio_net_hdr_to_skb, rather than
> > have to make the main datapath robust against crazy packets -- which
> > comes with branches and other overhead on the legitimate hot path.
> >
> >
> > >
> > > So the pull checks in __virtio_net_hdr_to_skb() guarantee access up to
> > > p_off, but do not guarantee that the
> > > header at nhoff is safely linear for direct iph->frag_off dereference.
> > >
> > > In this run, nhoff==headlen on the observed packets (IPv4 header
> > > starts at the linear tail boundary). Using
> > > skb_header_pointer() in the DODGY branch avoids this gap.
> > >
> > > I did not hit a KMSAN report in this rerun (instrumented/patched
> > > kernel), but the offset mismatch above was
> > > reproducible.
> > >
> > > Willem de Bruijn <willemdebruijn.kernel@gmail.com> 于2026年3月21日周六 09:36写道：
> > > >
> > > > Willem de Bruijn wrote:
> > > > > Guoyu Su wrote:
> > > > > > Syzbot reported a KMSAN uninit-value warning in gso_features_check()
> > > > > > called from netif_skb_features() [1].
> > > > > >
> > > > > > The current direct skb->len check is not sufficient for SKB_GSO_DODGY
> > > > > > packets. In the AF_PACKET/PACKET_VNET_HDR path, packet_snd() can build
> > > > > > a DODGY GSO skb whose total length is large enough, while the IPv4
> > > > > > header is not fully available as initialized linear data for a direct
> > > > > > iph->frag_off access.
> > > > >
> > > > > The fix looks fine, but the AI review of an earlier revision brings up
> > > > > a good point: __virtio_net_hdr_to_skb calls pskb_may_pull in all paths
> > > > > to ensure the network header is fully in skb linear. What kind of packet
> > > > > is this that managed to escape those checks?
> > > >
> > > > The packets I got out of the C repro just after virtio_net_hdr_to_skb
> > > > look as below.
> > > >
> > > > [   76.539562] vnet_hdr: flags=0x75 gso_type=0x1 hlen=0x6a gso_sz=0x416d cstart=0x58
> > > > [   76.539755] skb len=56584 data_len=56476 headroom=4 headlen=108 tailroom=0
> > > > [   76.539755] end-tail=208 mac=(4,76) mac_len=0 net=(80,12) trans=92
> > > > [   76.539755] shinfo(txflags=0 nr_frags=3 gso(size=16749 type=3 segs=0))
> > > > [   76.539755] csum(0x10005c start=92 offset=16 ip_summed=3 complete_sw=0 valid=0 level=0)
> > > > [   76.539755] hash(0x0 sw=0 l4=0) proto=0x0800 pkttype=0 iif=0
> > > > [   76.539755] priority=0x0 mark=0x0 alloc_cpu=0 vlan_all=0x0
> > > > [   76.539755] encapsulation=0 inner(proto=0x0000, mac=0, net=0, trans=0)
> > > > [   76.540713] dev name=ip6gretap0 feat=0x0000000e401d4869
> > > > [   76.540843] sk family=17 type=3 proto=0
> > > >
> > > > Clearly fishy. They do have VIRTIO_NET_HDR_F_NEEDS_CSUM set, so we
> > > > know which branch they take.
> > > >
> > > >         skb_reset_mac_header(skb);
> > > >
> > > >         if (hdr->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) {
> > > >                 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));
> > > >
> > > > // start == 88
> > > > // needed == 88 + 18 == 106
> > > >
> > > >                 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)
> > > >                         return -EINVAL;
> > > >
> > > >                 nh_min_len = skb_transport_offset(skb);
> > > >
> > > > // nh_min_len == 88
> > > >
> > > >                 p_off = nh_min_len + thlen;
> > > >
> > > > // p_off == 108
> > > >
> > > >                 if (!pskb_may_pull(skb, p_off))
> > > >                         return -EINVAL;
> > > >
> > > > // headlen == 108
> > > >
> > > > At the end of this headlen == 108, so all of iphdr should be in
> > > > linear.
> > > >
> > > > Since the syz repro requires repeat it is possible that I simply did
> > > > not capture the right packet, but I don't see the C program vary the
> > > > packet contents.
> >
> >