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

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

[wei.fang]

From: Wei Fang <wei.fang@nxp.com>

After calling skb_set_network_header() to skip past the VLAN tag,
skb->protocol still points to the outer VLAN EtherType. If
skb_probe_transport_header() is then called without updating
skb->protocol to the inner protocol, the flow dissector will misparse
the inner header as a VLAN header and fail to set transport_header,
leaving it at its uninitialized sentinel value (~0U).

This series fixes two instances of this bug in packet_parse_headers()
and tap_get_user_xdp() by assigning the return value of
vlan_get_protocol_and_depth() to skb->protocol after
skb_set_network_header().

Wei Fang (2):
  net: packet: fix skb->protocol not updated after VLAN network header
    adjustment
  net: tap: fix skb->protocol not updated after VLAN network header
    adjustment

 drivers/net/tap.c      | 12 +++++++++---
 net/packet/af_packet.c | 12 +++++++++---
 2 files changed, 18 insertions(+), 6 deletions(-)

-- 
2.34.1

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

[wei.fang]

From: Wei Fang <wei.fang@nxp.com>

In packet_parse_headers(), when processing a VLAN-tagged frame on a
SOCK_RAW AF_PACKET socket, 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 resolved by
vlan_get_protocol_and_depth(), leaving it pointing to the outer VLAN
EtherType (e.g. ETH_P_8021Q).

This mismatch causes skb_probe_transport_header() to invoke the flow
dissector with proto=ETH_P_8021Q but nhoff already pointing past the
VLAN tag to the inner header. The 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 = 0xFFFF).

Any subsequent code that calls skb_transport_header() or udp_hdr() on
such an skb will dereference a pointer 65535 bytes past skb->head,
potentially corrupting arbitrary kernel memory.

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 when skb_probe_transport_header() is called, allowing the
flow dissector to correctly identify the transport layer header.

Fixes: dfed913e8b55 ("net/af_packet: add VLAN support for AF_PACKET SOCK_RAW GSO")
Assisted-by: WChat:claude-opus-4-8
Signed-off-by: Wei Fang <wei.fang@nxp.com>
---
 net/packet/af_packet.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 0e1355be89f6..a62d445047c0 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -1945,9 +1945,15 @@ static void packet_parse_headers(struct sk_buff *skb, struct socket *sock)
 
 	/* Move network header to the right position for VLAN tagged packets */
 	if (likely(skb->dev->type == ARPHRD_ETHER) &&
-	    eth_type_vlan(skb->protocol) &&
-	    vlan_get_protocol_and_depth(skb, skb->protocol, &depth) != 0)
-		skb_set_network_header(skb, depth);
+	    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;
+		}
+	}
 
 	skb_probe_transport_header(skb);
 }
-- 
2.34.1

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

[wei.fang]

From: Wei Fang <wei.fang@nxp.com>

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@nxp.com>
---
 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;
+		}
+	}
 
 	rcu_read_lock();
 	tap = rcu_dereference(q->tap);
-- 
2.34.1

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@nxp.com>
> 
> 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@nxp.com>
> ---
>  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
> 

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@nxp.com>
> > ---
> >  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.

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

[Wei Fang]

> > > --- 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.

For AF_PACKET, it has been confirmed that skb->protocol does not need to be
restored to its initial value; otherwise, the egress tc flower for protocol ip will
not match the packet. A known issue is that the packets cannot match the egress
TC flower rules for protocol 802.1Q, but this issue exists before this series. This
is likely a limitation of using AF_PACKET to send packets.

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

[Willem de Bruijn]

Wei Fang wrote:
> > > > --- 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.
> 
> For AF_PACKET, it has been confirmed that skb->protocol does not need to be
> restored to its initial value; otherwise, the egress tc flower for protocol ip will
> not match the packet. A known issue is that the packets cannot match the egress
> TC flower rules for protocol 802.1Q, but this issue exists before this series. This
> is likely a limitation of using AF_PACKET to send packets.

Actually this update of network header to start of the IP header may
have been a mistake. If userspace inserts a VLAN packet, that is what
should enter the stack.

But for this specific issue: would it make sense to just move
skb_probe_transport_header before that adjustment?

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

[Wei Fang]

> > > > > --- 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.
> >
> > For AF_PACKET, it has been confirmed that skb->protocol does not need to be
> > restored to its initial value; otherwise, the egress tc flower for protocol ip will
> > not match the packet. A known issue is that the packets cannot match the
> egress
> > TC flower rules for protocol 802.1Q, but this issue exists before this series. This
> > is likely a limitation of using AF_PACKET to send packets.
> 
> Actually this update of network header to start of the IP header may
> have been a mistake. If userspace inserts a VLAN packet, that is what
> should enter the stack.
> 
> But for this specific issue: would it make sense to just move
> skb_probe_transport_header before that adjustment?

I think it is feasible. The transport_header will be set correctly, and other
behaviors will remain consistent with before, without introducing an
new issues.