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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Incomplete fix: `skb->protocol` is not updated after `network_header` adjustment in `tap_get_user()`.
--

--- Patch [2]: [PATCH net 2/2] net: tap: fix skb->protocol not updated after VLAN network header adjustment ---
commit 6866d2a8bbb104d45013a4b8fcd7c1800086ae28
Author: Wei Fang <[email protected]>

net: tap: fix skb->protocol not updated after VLAN network header adjustment

This commit fixes an issue in tap_get_user_xdp() where skb->protocol was not
updated after advancing the network header past the VLAN tag. It sets the
protocol to the inner EtherType to keep skb->protocol and network_header
consistent for subsequent processing.

> diff --git a/drivers/net/tap.c b/drivers/net/tap.c
> index fae115915c8ef..afcc4919bd042 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;
> +		}
> +	}

[Severity: High]
This isn't a bug introduced by this patch, but does tap_get_user() also need
this same fix for non-XDP transmission paths?

Looking at tap_get_user() in drivers/net/tap.c, it contains the identical
logic that was patched here:

drivers/net/tap.c:tap_get_user() {
    ...
    /* 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);
    ...
}

Without updating skb->protocol here as well, will TC egress and eBPF programs
still read skb->protocol as ETH_P_8021Q while network_header points to the
inner IP header?

If a user or guest VM writes a VLAN-tagged packet to a TAP file descriptor
using standard (non-XDP) I/O, could this allow them to craft an IP header
that causes traffic classification and security policies to misinterpret the
packet?

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2