Re: [PATCH net v2 5/5] gre: fix out-of-bounds read of erspan metadata in collect_md mode

[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 (2) · 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] Commit message / in-code comment mismatch: the patch is titled
  "gre: fix out-of-bounds read of erspan metadata in collect_md mode"…

Pre-existing issues:
- [Medium] erspan_rcv() (net/ipv4/ip_gre.c:334-361) hands collect_md
  consumers an ERSPAN option with md->version == 0 for ERSPAN type I…
- [Medium] is_erspan_type1() (net/ipv4/ip_gre.c:258-265) classifies a
  packet as ERSPAN type I purely from gre_hdr_len == 4, without…

--

gre: fix out-of-bounds read of erspan metadata in collect_md mode

This adds a check rejecting an ERSPAN base header whose version field is
neither 1 nor 2 in erspan_rcv() and ip6erspan_rcv(), and skips the md2
metadata copy in erspan_rcv() for ERSPAN type I, which carries no ERSPAN
header.  The collect_md option area is left as the zeroed area handed out
by ip_tun_rx_dst(), so md->version = 0 alone describes type I.

No Link: tags were present in the commit header.

First, a question about the changelog itself:

> gre: fix out-of-bounds read of erspan metadata in collect_md mode

and

>     Both read 8 bytes at 12 bytes from the start of the GRE header, but only
>     ask pskb_may_pull() for erspan_hdr_len(ver) bytes beyond it, which type I
>     support made 0 for version 0.

[Severity: Medium]
Is the "out-of-bounds read" description accurate here?  Both erspan_rcv()
and ip6erspan_rcv() only reach the memcpy() after
__iptunnel_pull_header(skb, len, htons(ETH_P_TEB), false, false) returned
success, and that helper linearizes an extra ETH_HLEN for ETH_P_TEB with
raw_proto == false:

net/ipv4/ip_tunnel_core.c:__iptunnel_pull_header() {
	...
	if (!raw_proto && inner_proto == htons(ETH_P_TEB)) {
		struct ethhdr *eh;

		if (unlikely(!pskb_may_pull(skb, ETH_HLEN)))
			return -ENOMEM;
	...
}

erspan_rcv() does "goto drop" and ip6erspan_rcv() does
"return PACKET_REJECT" when that fails, so at the copy point there are at
least 14 linear bytes from skb->data.  Walking the three cases in the
changelog:

- IPv4 type I: gre_hdr_len == 4, len == 4, pkt_md == skb->data + 4, an
  ERSPAN_V2_MDSIZE copy ends at skb->data + 12.
- IPv4 8 byte GRE with ershdr->ver == 0: len == 8, pkt_md == skb->data + 4,
  copy ends at skb->data + 12.
- IPv6 with ver == 0: len == 0, pkt_md == skb->data + 4, copy ends at
  skb->data + 12.

All three stay inside the linearized region, so nothing outside the skb is
touched and KASAN would not fire.  What does happen is that the first 8
bytes of the inner Ethernet frame get copied into md->u.md2 and an
unvalidated 4 bit wire value is stored in md->version, which is a
data-correctness problem rather than a memory-safety one.

Could the subject, the changelog and the stable justification describe that
instead, or is there a path that defeats the ETH_HLEN guarantee?

> diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
> index 696884f53cdcc..92f3a52d20d38 100644
> --- a/net/ipv4/ip_gre.c
> +++ b/net/ipv4/ip_gre.c

[ ... ]

> @@ -294,6 +293,9 @@ static int erspan_rcv(struct sk_buff *skb, struct tnl_ptk_info *tpi,
>  
>  		ershdr = (struct erspan_base_hdr *)(skb->data + gre_hdr_len);
>  		ver = ershdr->ver;
> +		if (unlikely(ver != 1 && ver != 2))
> +			return PACKET_REJECT;
> +

[Severity: Medium]
This isn't a bug introduced by this patch, but can this new check be
bypassed for a packet class that does carry a base header?

is_erspan_type1() decides purely on the GRE header length and never looks
at the GRE protocol:

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() is more careful and treats the two protocols
differently:

net/ipv4/gre_demux.c:gre_parse_header() {
	if ((greh->protocol == htons(ETH_P_ERSPAN) && hdr_len != 4) ||
	    greh->protocol == htons(ETH_P_ERSPAN2)) {
		struct erspan_base_hdr *ershdr;

		if (!pskb_may_pull(skb, nhs + hdr_len + sizeof(*ershdr)))
	...
}

So a frame with greh->flags == 0 and greh->protocol == htons(ETH_P_ERSPAN2)
yields hdr_len == 4, and gre_rcv() still routes it to erspan_rcv():

net/ipv4/ip_gre.c:gre_rcv() {
	if (unlikely(tpi.proto == htons(ETH_P_ERSPAN) ||
		     tpi.proto == htons(ETH_P_ERSPAN2))) {
		if (erspan_rcv(skb, &tpi, hdr_len) == PACKET_RCVD)
			return 0;
	...
}

In erspan_rcv() is_erspan_type1(4) then returns true, ver is forced to 0 and
the new "if (unlikely(ver != 1 && ver != 2))" above is never reached for
that packet.  len also stays at 4, so the 4 byte base header plus the 8 byte
md2 are handed to ip_tunnel_rcv() as the start of the inner Ethernet frame.

Would making the type I predicate also require
tpi->proto == htons(ETH_P_ERSPAN) close that, given ETH_P_ERSPAN2 can never
describe type I?

> @@ -334,19 +337,26 @@ static int erspan_rcv(struct sk_buff *skb, struct tnl_ptk_info *tpi,
>  			info = &tun_dst->u.tun_info;
>  			info->options_len = sizeof(*md);
>  
> -			/* skb can be uncloned in __iptunnel_pull_header, so
> -			 * old pkt_md is no longer valid and we need to reset
> -			 * it
> -			 */
> -			gh = skb_network_header(skb) +
> -			     skb_network_header_len(skb);
> -			pkt_md = (struct erspan_metadata *)(gh + gre_hdr_len +
> -							    sizeof(*ershdr));
>  			md = ip_tunnel_info_opts(&tun_dst->u.tun_info);
>  			md->version = ver;

[Severity: Medium]
This is a pre-existing issue, but is the version 0 encoding usable by any
consumer of this option?  For type I this publishes md->version == 0 with
info->options_len = sizeof(*md) and IP_TUNNEL_ERSPAN_OPT_BIT set, while
every peer of that option only accepts 1 or 2:

net/ipv4/ip_gre.c:erspan_fb_xmit() {
	if (version == 1) {
		...
	} else if (version == 2) {
		...
	} else {
		goto err_free_skb;
	}
}

net/ipv6/ip6_gre.c:ip6erspan_tunnel_xmit() {
		if (md->version == 1) {
			...
		} else if (md->version == 2) {
			...
		} else {
			goto tx_err;
		}
}

net/ipv4/ip_tunnel_core.c:ip_tun_parse_opts_erspan() {
	ver = nla_get_u8(tb[LWTUNNEL_IP_OPT_ERSPAN_VER]);
	if (ver == 1) {
	...
	} else if (ver == 2) {
	...
	} else {
		return -EINVAL;
	}
}

So a BPF or OVS mirror that re-emits a received ERSPAN type I frame with the
received tun_info is dropped in erspan_fb_xmit() (kfree_skb plus
tx_dropped), even though the non collect_md path does implement type I:

net/ipv4/ip_gre.c:erspan_xmit() {
	if (tunnel->erspan_ver == 0) {
		proto = htons(ETH_P_ERSPAN);
		__clear_bit(IP_TUNNEL_SEQ_BIT, flags);
	} else if (tunnel->erspan_ver == 1) {
	...
}

The patch does not create that asymmetry, but the changelog wording
"md->version = 0 alone describes it" makes version 0 a permanent, blessed
encoding.  Should erspan_fb_xmit(), ip6erspan_tunnel_xmit() and the netlink
parser learn about type I, or should IP_TUNNEL_ERSPAN_OPT_BIT not be
advertised for type I at all?

> +
> +			/* Type I has no ERSPAN header, thus no metadata to
> +			 * extract: reading it would go past the @len bytes
> +			 * pulled above. ip_tun_rx_dst() zeroed @md for us.
> +			 */

[Severity: Medium]
Same question as on the changelog above: "reading it would go past the @len
bytes pulled above" leaves out that __iptunnel_pull_header() already
required ETH_HLEN more linear bytes, so the read landed on the inner
Ethernet frame rather than past the linear area.  Could the comment say that
type I has no metadata to copy and the bytes there belong to the inner
frame?

> +			if (!is_erspan_type1(gre_hdr_len)) {
> +				/* skb can be uncloned in __iptunnel_pull_header, so
> +				 * old pkt_md is no longer valid and we need to reset
> +				 * it
> +				 */
> +				gh = skb_network_header(skb) +
> +				     skb_network_header_len(skb);
> +				pkt_md = (struct erspan_metadata *)(gh + gre_hdr_len +
> +								    sizeof(*ershdr));
> +				md2 = &md->u.md2;
> +				memcpy(md2, pkt_md, ver == 1 ? ERSPAN_V1_MDSIZE :
> +							       ERSPAN_V2_MDSIZE);
> +			}

[Severity: Medium]
This isn't a bug introduced by this patch, but tied to the is_erspan_type1()
question above: for an ETH_P_ERSPAN2 frame with a 4 byte GRE header this
guard now skips the copy for a packet that does carry an ERSPAN v2 metadata
block, so collect_md consumers see version 0 with an all zero md2.  Before
this change the copy at gh + gre_hdr_len + sizeof(*ershdr) happened to land
on the real md2 for that packet.

>  
>  			__set_bit(IP_TUNNEL_ERSPAN_OPT_BIT,
>  				  info->key.tun_flags);
> diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
> index 8ebda0b6a78b2..a59fb82c74dad 100644
> --- a/net/ipv6/ip6_gre.c
> +++ b/net/ipv6/ip6_gre.c
> @@ -503,6 +503,8 @@ static int ip6erspan_rcv(struct sk_buff *skb,
>  	ipv6h = ipv6_hdr(skb);
>  	ershdr = (struct erspan_base_hdr *)skb->data;
>  	ver = ershdr->ver;
> +	if (unlikely(ver != 1 && ver != 2))
> +		return PACKET_REJECT;

The version rejection itself looks right for this path, since ip6erspan_rcv()
always has a base header at skb->data.  The only question here is the one
raised on the changelog about the ver == 0 case being described as an out of
bounds read.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260916100155.1398403-1-edumazet%40google.com