[PATCH net] net: parse the eighth nested VLAN tag

[PATCH net] net: parse the eighth nested VLAN tag

[Avi Weiss]

__vlan_get_protocol_offset() starts with VLAN_MAX_DEPTH (8) but
pre-decrements parse_depth before checking it. This rejects the eighth
VLAN header and returns protocol 0 instead of the encapsulated EtherType.

Check the counter before decrementing it, allowing eight nested tags
while continuing to reject a ninth. A compiled source-extracted test of
the helper and its public inline wrapper reproduces the incorrect result
for eight tags and verifies the corrected result. Seven-tag, nine-tag,
and truncated-header controls are unchanged.

Fixes: 469aceddfa3e ("vlan: consolidate VLAN parsing code and limit max parsing depth")
Signed-off-by: Avi Weiss <thnkslprpt@gmail.com>
---
 net/core/skbuff.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 48f676411724..b10ce1d46674 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -7703,7 +7703,7 @@ struct vlan_type_depth __vlan_get_protocol_offset(const struct sk_buff *skb,
 
 		vh = skb_header_pointer(skb, mac_offset + vlan_depth,
 					sizeof(vhdr), &vhdr);
-		if (unlikely(!vh || !--parse_depth))
+		if (unlikely(!vh || !parse_depth--))
 			return (struct vlan_type_depth) { 0 };
 
 		type = vh->h_vlan_encapsulated_proto;

base-commit: cb284914e749388eef402938bdb3115fe1f6d337
-- 
2.43.0

Re: [PATCH net] net: parse the eighth nested VLAN tag

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH net] net: parse the eighth nested VLAN tag

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The patch fixes the counter only when mac_len is 0 or covers the
  first VLAN header.

--

net: parse the eighth nested VLAN tag

This commit changes the parse_depth check in __vlan_get_protocol_offset()
from a pre-decrement to a post-decrement. The goal is for a packet with
eight nested VLAN tags to return the encapsulated EtherType instead of 0,
while a ninth tag is still rejected.

> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 48f6764117243..b10ce1d46674b 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -7703,7 +7703,7 @@ struct vlan_type_depth __vlan_get_protocol_offset(const struct sk_buff *skb,
>  
>  		vh = skb_header_pointer(skb, mac_offset + vlan_depth,
>  					sizeof(vhdr), &vhdr);
> -		if (unlikely(!vh || !--parse_depth))
> +		if (unlikely(!vh || !parse_depth--))
>  			return (struct vlan_type_depth) { 0 };

[Severity: High]
Does this allow eight nested tags for every value of skb->mac_len?

parse_depth goes down once per loop pass, not once per VLAN header that is
actually parsed. What the first pass reads depends on where the loop
starts:

net/core/skbuff.c:__vlan_get_protocol_offset() {
    ...
	if (vlan_depth) {
		if (WARN_ON_ONCE(vlan_depth < VLAN_HLEN))
			return (struct vlan_type_depth) { 0 };
		vlan_depth -= VLAN_HLEN;
	} else {
		vlan_depth = ETH_HLEN;
	}
    ...
}

With mac_len == 0 or mac_len == ETH_HLEN + VLAN_HLEN, the first pass
parses the first tag, and eight tags now work.

With mac_len == ETH_HLEN and the first tag still in-band, vlan_depth
starts at 10. The first pass reads the last two bytes of the source MAC as
the TCI and the outer TPID as h_vlan_encapsulated_proto. That only re-reads
the type that was passed in.

Eight in-band tags then take nine passes. The ninth pass sees
parse_depth == 0 and returns { 0 }, which is the result the commit message
says is fixed.

The egress path on a device without VLAN TX offload seems to produce this
state:

validate_xmit_vlan()
  __vlan_hwaccel_push_inside()
    vlan_insert_tag_set_proto()
      __vlan_insert_inner_tag()

__vlan_insert_inner_tag() pushes the hwaccel tag inline and moves
skb->mac_header, but it does not update skb->mac_len:

	skb_push(skb, VLAN_HLEN);
	...
	if (skb_mac_header_was_set(skb))
		skb->mac_header -= VLAN_HLEN;

A forwarded skb that arrived with mac_len == ETH_HLEN keeps that value. A
later vlan_get_protocol() call, such as one in a driver's xmit checksum
path, would then still get 0 for a frame with eight tags.

On the plain RX path (skb_vlan_untag() followed by skb_reset_mac_len()),
the extra pass happens to line up with the stripped hwaccel tag, so eight
tags still work there.

Could parse_depth be decremented only for passes that parse a real VLAN
header? Alternatively, the start offset could be chosen so that every pass
parses one.

If the current behaviour is kept, should the commit message and the
VLAN_MAX_DEPTH comment mention that the result depends on mac_len? The two
texts in question are "allowing eight nested tags while continuing to
reject a ninth" and "Max. number of nested VLAN tags parsed".

>  
>  		type = vh->h_vlan_encapsulated_proto;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261031135507.3970551-2-thnkslprpt%40gmail.com