Re: [PATCH net v3 3/8] tun: clear stale skb->protocol of NAPI frags skbs

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message understates what a stale skb->protocol does in
  the non-tunnel path.
- [Low] The first symptom in the commit message ("the outer protocol
  check of UDP tunnel GSO packets rejects a valid frame when the two…

--

tun: clear stale skb->protocol of NAPI frags skbs

This patch clears skb->protocol for IFF_TAP frames in tun_get_user()
before tun_vnet_hdr_tnl_to_skb() runs. With IFF_NAPI_FRAGS, an skb
recycled by napi_reuse_skb() keeps the protocol that napi_frags_skb() set
for the previous frame. Without this patch, the virtio header checks see
that stale value.

Closes: https://lore.kernel.org/netdev/179386753589.488856.11455181721004759454@kernel.org/

[ ... ]

> On the IFF_TAP path, tun_get_user() calls tun_vnet_hdr_tnl_to_skb()
> before eth_type_trans(), so the virtio header checks see the protocol
> of the previous frame: the outer protocol check of UDP tunnel GSO
> packets rejects a valid frame when the two ethertypes differ, and

[Severity: Low]
Does this symptom really come from 90e33d459407? The outer protocol check
that compares against skb->protocol seems to have been added earlier in
this series, by "net: virtio: validate outer protocol of UDP tunnel GSO
packets":

include/linux/virtio_net.h:virtio_net_l3_start() {
    ...
		h_proto = eth_hdr(skb)->h_proto;
		...
		if (skb->protocol && skb->protocol != h_proto)
			return false;
    ...
}

At the baseline, the tunnel path in virtio_net_hdr_tnl_to_skb() never
read skb->protocol. A stale value could not cause a tunnel rejection
there.

With the current ordering, the two intermediate commits ("net: virtio:
validate outer protocol of UDP tunnel GSO packets" and "net: virtio: do
not use dev->type to find the outer L3 header") would drop a valid UDP
tunnel GSO frame with -EINVAL. This happens on an IFF_TAP |
IFF_NAPI_FRAGS device when GRO holds packets and the outer ethertype
differs from the one in the previous merged frame, for example an IPv4
outer after an IPv6 frame:

tun_get_user()
  tun_vnet_hdr_tnl_to_skb()
    virtio_net_hdr_tnl_to_skb()
      virtio_net_tnl_offsets_ok()
        virtio_net_l3_start()   <- stale skb->protocol != h_proto

Could this patch move ahead of "net: virtio: validate outer protocol of
UDP tunnel GSO packets" so the series stays bisectable? If not, could
the commit message at least mention the dependency on that commit?

> GSO packets without VIRTIO_NET_HDR_F_NEEDS_CSUM are dissected with
> the wrong protocol.

[Severity: Low]
Is "dissected with the wrong protocol" too mild for what happens here?
In __virtio_net_hdr_to_skb(), a stale non-zero skb->protocol means
dev_parse_header_protocol() never reads the real h_proto of the frame:

include/linux/virtio_net.h:__virtio_net_hdr_to_skb() {
    ...
	} else if (gso_type) {
		...
		if (!skb->protocol) {
			skb->protocol = dev_parse_header_protocol(skb);
			if (!skb->protocol)
				virtio_net_hdr_set_proto(skb, hdr);
		}
retry:
		if (!skb_flow_dissect_flow_keys_basic(NULL, skb, &keys,
						      NULL, 0, 0, 0,
						      0)) {
		...
		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;
    ...
}

When data is NULL, the dissector starts from skb->protocol at
skb_network_offset(), which tun_get_user() sets to ETH_HLEN. So
virtio_net_hdr_match_proto() checks the protocol of the previous frame,
not the ethertype of this one.

A writer could then send a VIRTIO_NET_HDR_GSO_TCPV4 frame whose h_proto
is something else, such as MPLS or NSH, with what looks like an
IPv4/TCP payload at offset 14. That frame would pass validation and get
SKB_GSO_TCPV4 | SKB_GSO_DODGY. eth_type_trans() would then set
skb->protocol to the h_proto that was never checked.

The code change closes this. Could the commit message describe it as a
bypass of the check that the ethertype matches the gso_type, rather than
just a misparse?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261105115715.1466924-1-edumazet%40kernel.org