RE: [PATCH net 2/2] net: tap: fix skb->protocol not updated after VLAN network header adjustment

[Wei Fang]

> > In tap_get_user_xdp(), when processing a VLAN-tagged frame,
> > skb_set_network_header() is called with the depth returned by
> > vlan_get_protocol_and_depth() to advance network_header past the VLAN
> > tag to the inner protocol header. However, skb->protocol was not updated
> > to reflect the inner EtherType, leaving it pointing to the outer VLAN
> > EtherType (e.g. ETH_P_8021Q).
> >
> > This mismatch has two consequences. First, skb_probe_transport_header()
> > is called after the VLAN adjustment with proto=ETH_P_8021Q but nhoff
> > already pointing past the VLAN tag to the inner header. The flow
> > dissector interprets the inner header bytes as a VLAN header, fails to
> > find a recognizable encapsulated protocol, and returns false.
> > Consequently, transport_header is never set and remains at its
> > uninitialized sentinel value (~0U), causing any subsequent
> > skb_transport_header() or udp_hdr() call to dereference a pointer
> > 65535 bytes past skb->head, potentially corrupting arbitrary kernel
> > memory. Second, TC egress and eBPF programs that inspect skb->protocol
> > directly (e.g. bpf_skb_net_base_len(), bpf_skb_net_grow(),
> > __bpf_redirect_neigh()) will see ETH_P_8021Q instead of the inner
> > protocol and behave incorrectly.
> >
> > Save the return value of vlan_get_protocol_and_depth(), which already
> > resolves the inner EtherType, and assign it to skb->protocol after
> > skb_set_network_header(). This keeps skb->protocol and network_header
> > consistent for all subsequent processing.
> >
> > Fixes: 8c76e77f9069 ("tap: call skb_probe_transport_header after setting
> skb->dev")
> > Assisted-by: WChat:claude-opus-4-8
> > Signed-off-by: Wei Fang <wei.fang@xxxxxxx>
> > ---
> >  drivers/net/tap.c | 12 +++++++++---
> >  1 file changed, 9 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/tap.c b/drivers/net/tap.c
> > index fae115915c8e..afcc4919bd04 100644
> > --- a/drivers/net/tap.c
> > +++ b/drivers/net/tap.c
> > @@ -1081,9 +1081,15 @@ static int tap_get_user_xdp(struct tap_queue *q,
> struct xdp_buff *xdp)
> >  	}
> >
> >  	/* Move network header to the right position for VLAN tagged packets */
> > -	if (eth_type_vlan(skb->protocol) &&
> > -	    vlan_get_protocol_and_depth(skb, skb->protocol, &depth) != 0)
> > -		skb_set_network_header(skb, depth);
> > +	if (eth_type_vlan(skb->protocol)) {
> > +		__be16 proto = vlan_get_protocol_and_depth(skb, skb->protocol,
> > +							   &depth);
> > +
> > +		if (proto != 0) {
> > +			skb_set_network_header(skb, depth);
> > +			skb->protocol = proto;
> > +		}
> > +	}
> 
> Does the same apply to the same call in tap_get_user?

The situation of tap_get_user() is different, skb_probe_transport_header()
is called before the VLAN adjustment block. So I think transport_header
should be correct. The only concern is whether skb->protocol needs to be
updated after calling skb_set_network_header().

I'm not sure as I am not fairly familiar with the tap driver. I added this patch
because Sashiko reported that the tap driver has the same issue as af_packet.

> 
> And in general to other callers of vlan_get_protocol_and_depth,
> including through wrapper skb_network_protocol?

I don't think this issue exists elsewhere. The issue arises because
skb_probe_transport_header() is called after skb_set_network_header(), and
at this point, skb->protocol and network_header are not synchronized ( In
__skb_flow_dissect(), nhoff = skb_network_offset(skb) but proto is
ETH_P_8021Q or ETH_P_8021AD)), causing skb_probe_transport_header() to
fail to set transport_header correctly.

Perhaps the correct approach would be to restore the original `skb->protocol`
value after `skb_probe_transport_header()`, maintaining consistency with the
previous behavior; otherwise, it might introduce new issues.