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:
> > Perhaps you're running a different repro from the one I used. Which is
> > the C repro from the run at commit ca4ee40bf13d.

Thanks for verifying.

> I reran with the exact ReproC from that run:
> https://syzkaller.appspot.com/text?tag=ReproC&x=10e3fe5a580000
> (commit ca4ee40bf13d), rebuilt locally, and reran.
> 
> > I see that the virtio_net_hdr has hdr_len 106 and csum_start 88.
> > Those are fine. Same for your repro?
> 
> Yes, same in my run as well:
> vnet_hlen=106, vnet_csum_start=88 (first 20 dumps are consistent).
> 
> > The question is how skb->network_header can be greater than
> > skb->transport_header right after virtio_net_hdr_to_skb.
> 
> From my instrumentation on the same skb in packet_snd():
> - packet_parse_headers() sets netoff from device L2 layout:
>   hard_hlen=172 on ip6gretap0, so netoff=172.

I don't see this, but hard coding it gets the same issue.

More importantly, I took a closer look at a fix.

Unfortunately skb_network_offset cannot be trusted for link layers
with variable length headers. With SOCK_RAW it is the worst case
hard_header_length. PF_PACKET is network layer agnostic, and with
SOCK_RAW on variable length link layer packets, nothing communicates
this.

So, a straightforward check like this may have false positives where
a valid packet is shorter than this worst case estimation of network
offset.

	@@ -103,7 +103,7 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb,

			if (!skb_partial_csum_set(skb, start, off))
				return -EINVAL;
	-               if (skb_transport_offset(skb) < nh_min_len)
	+               if (skb_transport_offset(skb) < skb_network_offset(skb) + nh_min_len)

As a result, I don't see any test we can do at this point that will
not have false positives.

Only for GSO packets is there downstream code that requires the network header
and that assumes this header starts at skb->network_header. __skb_gso_segment
calls skb_reset_mac_len(skb) and parses the headers in skb_mac_gso_segment and
skb_network_protocol robustly using skb_header_pointer and pskb_may_pull.

We can at this entry point anticipate reaching that code and add an
extra branch if gso_type. But might as well just make robust the one access in
the GSO path that is not yet.

So in short your original approach is probably preferable.

Please do add a Link: to this thread.

And one more thing: skb_header_pointer already takes the fast path of
just returning the offset if within linear. No need to special case
the DODGY vs non-DODGY path.


~                                                                                                                                               
~                                                                                                                                               
~                                                                                                                                               
~                                                                                                                                               
~