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

[Willem de Bruijn]

wei.fang@ wrote:
> From: Wei Fang <wei.fang@xxxxxxx>
> 
> 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?

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

>  
>  	rcu_read_lock();
>  	tap = rcu_dereference(q->tap);
> -- 
> 2.34.1
>