Re: [PATCH net-next v2] erspan: drop packets with an unknown ERSPAN version

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 1 · 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).

- [Medium] The new version check can be bypassed: an ETH_P_ERSPAN2 (type
  III) frame with a 4-byte GRE header never reaches it.

--

erspan: drop packets with an unknown ERSPAN version

This patch makes erspan_rcv() and ip6erspan_rcv() reject packets whose
ERSPAN version field is not 1 (type II) or 2 (type III). Before this,
such packets either had no ERSPAN header pulled or were given the type
III length.

> diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
> index 5485586fa0e62..ea41be3c4229b 100644
> --- a/net/ipv4/ip_gre.c
> +++ b/net/ipv4/ip_gre.c
> @@ -303,8 +303,10 @@ static int erspan_rcv(struct sk_buff *skb, struct tnl_ptk_info *tpi,
>  	if (tunnel) {
>  		if (is_erspan_type1(gre_hdr_len))
>  			len = gre_hdr_len;
> -		else
> +		else if (ver == ERSPAN_VERSION || ver == ERSPAN_VERSION2)
>  			len = gre_hdr_len + erspan_hdr_len(ver);
> +		else
> +			return PACKET_REJECT;

[Severity: Medium]
Can an ETH_P_ERSPAN2 frame with a 4-byte GRE header get around this new
check?

is_erspan_type1() looks only at gre_hdr_len and never checks
tpi->proto:

net/ipv4/ip_gre.c:is_erspan_type1() {
	/* Both ERSPAN type I (version 0) and type II (version 1) use
	 * protocol 0x88BE, but the type I has only 4-byte GRE header,
	 * while type II has 8-byte.
	 */
	return gre_hdr_len == 4;
}

gre_parse_header() treats ETH_P_ERSPAN2 (0x22EB, defined in uAPI as
type III) as always carrying an ERSPAN base header, whatever hdr_len
is:

net/ipv4/gre_demux.c:gre_parse_header() {
    ...
	if ((greh->protocol == htons(ETH_P_ERSPAN) && hdr_len != 4) ||
	    greh->protocol == htons(ETH_P_ERSPAN2)) {
		...
		tpi->key = cpu_to_be32(get_session_id(ershdr));
	}
    ...
}

gre_rcv() then calls erspan_rcv(skb, &tpi, 4). Since
is_erspan_type1(4) is true, ver is forced to 0 and the lookup uses
NO_KEY with key 0. That can match a keyless erspan_ver 0 tunnel or the
collect_md fallback in ip_tunnel_lookup().

>From there len = gre_hdr_len and the new version check is skipped. An
ERSPAN header with any version (0 or 3..15) is passed up as the start
of the inner Ethernet frame, which is the outcome the commit message
says is fixed.

In collect_md mode, tun_id = key32_to_tunnel_id(tpi->key) is also built
from the session ID in those 4 bytes, and the same bytes are handed up
as the inner MAC header.

__iptunnel_pull_header() still pulls ETH_HLEN, so nothing is read out
of bounds.

Would it make sense to also require tpi->proto == htons(ETH_P_ERSPAN)
when classifying a packet as type I, so this path is covered too?

>  
>  		if (unlikely(!pskb_may_pull(skb, len)))
>  			return PACKET_REJECT;

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261107130030.35220-1-yanhaishuang%40cmss.chinamobile.com