Re: [PATCH net] net: limit priority VLAN untag depth

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Kamil Sawicki wrote:
> The priority VLAN receive path can repeatedly remove VID 0 tags and retry
> packet processing. Unlike the read-only VLAN protocol parser, this path has
> no limit on the number of nested tags it consumes.
> 
> Each skb_vlan_untag() advances skb->data and resets header offsets. The
> offsets are stored in 16-bit fields. With enough nested tags, the network
> or transport offset can wrap. A later protocol handler can then derive a
> large pull length from the wrapped offsets and access memory outside the
> skb allocation.
> 
> Apply the existing VLAN_MAX_DEPTH limit to the exceptional VID 0 retry.
> Count the tag already stored in skb VLAN metadata as the first level, then
> stop before untagging a ninth level. Keep the accounting inside this
> uncommon branch so the regular VLAN receive path is unaffected.
> 
> Fixes: 36b2f61a42c2 ("net: handle 802.1P vlan 0 packets properly")

Thanks for the patch. Agreed on the general approach.

One small suggestion for the drop_reason is worth a respin imho.
But aside from that

Reviewed-by: Willem de Bruijn <willemb@google.com>

Just curious: is more than one 802.1P vlan 0 packet preceding a real
vlan tag even something that would occur in practice?

> Cc: stable@vger.kernel.org
> Assisted-by: openai:gpt-5.6-sol
> Signed-off-by: Kamil Sawicki <user148783812897@gmail.com>
> ---
> Tested on net/main f4377bf85887 (7.3-rc7) with KASAN. The patched kernel
> blocked the nested vhost/TAP trigger after eight untag calls on five fresh
> boots, with no tcp_v4_rcv call. The eight/nine-tag boundary, nonlinear
> GSO, 32 MiB BIG TCP and fragmented-head tests passed.
> 
> An unpatched protocol-253 control executed 31 calls to
> skb_copy_and_csum_bits() with offset 65516 and length 548. A direct shadow
> probe recorded a 20-byte wrapped linear source outside the skb allocation;
> all four Generic-KASAN shadow bytes were zero. The patched control completed
> the same workload with zero matching sink calls and zero shadow-probe hits.
> 
> Allyesconfig and allmodconfig W=1 builds completed with CONFIG_WERROR=
> because unrelated existing warnings stopped the default full builds. Both
> produced vmlinux and bzImage, and net/core/dev.c emitted no warnings.
> 
>  net/core/dev.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/net/core/dev.c b/net/core/dev.c
> index e76762e29360ea..b40efea27dc292 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -6198,6 +6198,8 @@ static int __netif_receive_skb_core(struct sk_buff **pskb, bool pfmemalloc,
>  	}
>  
>  	if (unlikely(skb_vlan_tag_present(skb)) && !netdev_uses_dsa(skb->dev)) {
> +		unsigned int vlan_depth = 1;
> +

nit: since this does not count the actual vlan tags removed with jumps
to another_round, slightly confusing naming. Maybe 8021p_depth
or so.

>  check_vlan_id:
>  		if (skb_vlan_tag_get_id(skb)) {
>  			/* Vlan id is non 0 and vlan_do_receive() above couldn't
> @@ -6209,6 +6211,10 @@ static int __netif_receive_skb_core(struct sk_buff **pskb, bool pfmemalloc,
>  			 * 802.1Q or 802.1AD and vlan_do_receive() above could
>  			 * not find vlan dev for vlan id 0.
>  			 */
> +			if (unlikely(vlan_depth >= VLAN_MAX_DEPTH))
> +				goto drop;

consider

        if (unlikely(vlan_depth >= VLAN_MAX_DEPTH)) {
            drop_reason = SKB_DROP_REASON_RECURSION_LIMIT;
            goto drop;
        }

> +
> +			vlan_depth++;
>  			__vlan_hwaccel_clear_tag(skb);
>  			skb = skb_vlan_untag(skb);
>  			if (unlikely(!skb))
> 
> base-commit: f4377bf85887f57b59abb5479f7660a81ffe4449
> -- 
> 2.53.0
>