Re: [PATCH net v3 2/5] ip_gre: validate netlink attributes before changing the tunnel

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 1 (1) · Low: 3 (1)

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] ipgre_changelink() and erspan_changelink() still apply the new
  encapsulation to the live tunnel before ip_tunnel_changelink(), and…
- [Low] The commit message misstates how the COLLECT_METADATA changelink
  bug works.
- [Low] The patch adds a new check to ipgre_newlink_encap_setup() that
  the commit message does not mention: `if (t->collect_md &&…
- [Low] The patch moves ipgre_commit_parms() after
  ip_tunnel_changelink().

Pre-existing issues:
- [Medium] When erspan_changelink() succeeds, ipgre_commit_parms() writes
  the new t->erspan_ver, but tunnel->tun_hlen and tunnel->hlen are not…
- [Low] ipgre_commit_parms() writes t->erspan_ver, t->index, t->hwid,
  t->dir and t->ignore_df as separate plain stores under RTNL only.

--

ip_gre: validate netlink attributes before changing the tunnel

This commit stages the GRE and ERSPAN netlink attributes in a new local
struct ip_gre_parm. It commits them to the tunnel only after
ip_tunnel_changelink() succeeds. It also rejects enabling
IFLA_GRE_COLLECT_METADATA on an existing tunnel during changelink, and
moves encap setup after parameter validation in the changelink handlers.

> 1. ipgre_netlink_parms() sets t->collect_md = true and changes dev->type
>    from ARPHRD_IPGRE to ARPHRD_NONE before validating IFLA_GRE_IGNORE_DF
>    or running ip_tunnel_changelink(). Moreover, unlike ipip_changelink()
>    and ip6_tnl_changelink(), it accepts IFLA_GRE_COLLECT_METADATA during
>    changelink even though ip_tunnel_changelink() never registers the
>    tunnel as itn->collect_md_tun; a subsequent ip_tunnel_del() then
>    clears itn->collect_md_tun and blackholes the netns metadata tunnel.

[Severity: Low]
Is the "never registers" part accurate for the success path?

Before this patch, ipgre_netlink_parms() set t->collect_md = true before
ip_tunnel_changelink() ran. On success, ip_tunnel_changelink() calls
ip_tunnel_update(), which calls ip_tunnel_del() and then
ip_tunnel_add():

net/ipv4/ip_tunnel.c:ip_tunnel_add() {
    ...
	if (t->collect_md)
		rcu_assign_pointer(itn->collect_md_tun, t);
    ...
}

net/ipv4/ip_tunnel.c:ip_tunnel_del() {
	if (t->collect_md)
		rcu_assign_pointer(itn->collect_md_tun, NULL);
    ...
}

So a successful changelink evicts any existing metadata tunnel in the
netns and registers this tunnel in its place. Deleting either device
afterwards clears the slot.

The tunnel only ends up with collect_md set but unregistered on the
failure paths. These are the IFLA_GRE_IGNORE_DF -EINVAL, an encap setup
error, or ip_tunnel_changelink() failing before it reaches
ip_tunnel_update().

The fix itself looks correct. Since this is Cc: stable, could the
description also cover the success-path takeover of itn->collect_md_tun?

> diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
> index 82309efd417e0..df4d2f1f1d60c 100644
> --- a/net/ipv4/ip_gre.c
> +++ b/net/ipv4/ip_gre.c
> @@ -1183,15 +1183,41 @@ static int erspan_validate(struct nlattr *tb[], struct nlattr *data[],
>  	return 0;
>  }
>  
> +struct ip_gre_parm {
> +	__u32	fwmark;
> +	u32	index;
> +	u16	hwid;
> +	u8	erspan_ver;
> +	u8	dir;
> +	bool	ignore_df;
> +};
> +
> +static void ipgre_commit_parms(struct ip_tunnel *t,
> +			       const struct ip_gre_parm *gparms)
> +{
> +	t->ignore_df = gparms->ignore_df;
> +	t->erspan_ver = gparms->erspan_ver;
> +	t->index = gparms->index;
> +	t->hwid = gparms->hwid;
> +	t->dir = gparms->dir;
> +}

[Severity: Low]
This isn't a bug introduced by this patch, but these are plain stores
made under RTNL only. The i_flags/o_flags copies follow as separate
steps. erspan_xmit() and __gre_xmit() read the same fields without
synchronization.

Can a transmit that races with changelink build a packet that mixes a
new erspan_ver with the old index or hwid, or with the old tun_hlen?

The old code had the same unsynchronized writes in
erspan_netlink_parms(), and this patch doesn't widen the window. At
worst, headers could be malformed for a short time during
reconfiguration, and KCSAN could report the plain concurrent accesses.

[ ... ]

> @@ -1401,7 +1431,12 @@ ipgre_newlink_encap_setup(struct net_device *dev, struct nlattr *data[])
>  
>  	if (ipgre_netlink_encap_parms(data, &ipencap)) {
>  		struct ip_tunnel *t = netdev_priv(dev);
> -		int err = ip_tunnel_encap_setup(t, &ipencap);
> +		int err;
> +
> +		if (t->collect_md && ipencap.type != TUNNEL_ENCAP_NONE)
> +			return -EINVAL;

[Severity: Low]
The commit message doesn't mention this check. Is it meant as a
separate behavior change?

In ipgre_newlink() and erspan_newlink(), ipgre_newlink_encap_setup()
runs before ipgre_netlink_parms() sets t->collect_md, so the check can't
fire when the device is created. For creation, ipgre_tunnel_validate()
already rejects COLLECT_METADATA combined with a non-NONE ENCAP_TYPE in
the same request.

The check only takes effect in ipgre_changelink() and
erspan_changelink(). There, ipgre_netlink_parms() now runs first and
t->collect_md is live device state.

Consider a changelink on an existing external GRE/GRETAP/ERSPAN device
that sets IFLA_GRE_ENCAP_TYPE to FOU or GUE and omits
IFLA_GRE_COLLECT_METADATA. That request used to be accepted and now
fails with -EINVAL.

The restriction may well be right, since ip_md_tunnel_xmit() uses
tun_info->encap and not t->encap. Could the commit message describe
this userspace-visible change and explain why it is needed, given the
stable tag?

> +
> +		err = ip_tunnel_encap_setup(t, &ipencap);
>  
>  		if (err < 0)
>  			return err;

[ ... ]

> @@ -1458,24 +1495,25 @@ static int ipgre_changelink(struct net_device *dev, struct nlattr *tb[],
>  {
>  	struct ip_tunnel *t = netdev_priv(dev);
>  	struct ip_tunnel_parm_kern p;
> -	__u32 fwmark = t->fwmark;
> +	struct ip_gre_parm gparms;
>  	int err;
>  
>  	if (!rtnl_dev_link_net_capable(dev, t->net))
>  		return -EPERM;
>  
> -	err = ipgre_newlink_encap_setup(dev, data);
> -	if (err)
> +	err = ipgre_netlink_parms(dev, data, tb, &p, &gparms, false);
> +	if (err < 0)
>  		return err;
>  
> -	err = ipgre_netlink_parms(dev, data, tb, &p, &fwmark);
> -	if (err < 0)
> +	err = ipgre_newlink_encap_setup(dev, data);
> +	if (err)
>  		return err;
>  
> -	err = ip_tunnel_changelink(dev, tb, &p, fwmark);
> +	err = ip_tunnel_changelink(dev, tb, &p, gparms.fwmark);
>  	if (err < 0)
>  		return err;

[Severity: Medium]
ipgre_newlink_encap_setup() still applies the new encapsulation to the
live tunnel before ip_tunnel_changelink(), and that call can still fail.
Does this error return leave the device with the new encap and the old
parameters?

ipgre_newlink_encap_setup()->ip_tunnel_encap_setup() writes straight to
the tunnel:

net/ipv4/ip_tunnel.c:ip_tunnel_encap_setup() {
    ...
	t->encap.type = ipencap->type;
	t->encap.sport = ipencap->sport;
	t->encap.dport = ipencap->dport;
	t->encap.flags = ipencap->flags;

	t->encap_hlen = hlen;
	t->hlen = t->encap_hlen + t->tun_hlen;
    ...
}

After that, ip_tunnel_changelink() can still fail, for example here:

net/ipv4/ip_tunnel.c:ip_tunnel_changelink() {
    ...
	if (dev == itn->fb_tunnel_dev)
		return -EINVAL;

	t = ip_tunnel_find(itn, p, dev->type);

	if (t) {
		if (t->dev != dev)
			return -EEXIST;
    ...
}

It can also return -EINVAL on an IFF_POINTOPOINT/IFF_BROADCAST
mismatch.

Take "ip link set gre0 type gre encap fou encap-dport N" on the fallback
device, or pointing gre2 at gre1's remote while also adding "encap fou".
Both return an error, but the device now sends FOU/GUE-encapsulated
traffic with its old parms, fwmark, flags and gparms. This patch also
doesn't refresh the MTU or needed_headroom on this error path.

The commit message says the attributes are committed "only after
ip_tunnel_changelink() has succeeded", but the encap is still committed
before that point. erspan_changelink() uses the same ordering.

Later patches in the series ("ip_gre: compute tunnel lengths absolutely
instead of by delta" and "ip_gre: recompute erspan header lengths after
a change") only refresh the lengths on the error path. The partially
applied encap is therefore still there at the end of the series.

Could the encap be staged too, and applied only after
ip_tunnel_changelink() succeeds?

>  
> +	ipgre_commit_parms(t, &gparms);
>  	ip_tunnel_flags_copy(t->parms.i_flags, p.i_flags);
>  	ip_tunnel_flags_copy(t->parms.o_flags, p.o_flags);

[Severity: Low]
ip_tunnel_changelink()->ip_tunnel_update() ends with
netdev_state_change(dev). That runs the NETDEV_CHANGE notifiers and
sends an RTM_NEWLINK built by ipgre_fill_info() or erspan_fill_info().

With ipgre_commit_parms() now after that call, does the notification
report the old t->ignore_df? For ERSPAN, does it also report the old
erspan_ver, index, dir and hwid?

Before this patch, those fields were written ahead of
ip_tunnel_changelink(), so the first notification had the new values.
rtnl_changelink() sets DO_SETLINK_NOTIFY, so do_setlink() sends a
second, correct notification afterwards. Listeners still see one
transient RTM_NEWLINK with stale values. The ordering is unchanged at
the end of the series.

> @@ -1490,24 +1528,25 @@ static int erspan_changelink(struct net_device *dev, struct nlattr *tb[],

[ ... ]

> -	err = ip_tunnel_changelink(dev, tb, &p, fwmark);
> +	err = ip_tunnel_changelink(dev, tb, &p, gparms.fwmark);
>  	if (err < 0)
>  		return err;
>  
> +	ipgre_commit_parms(t, &gparms);
>  	ip_tunnel_flags_copy(t->parms.i_flags, p.i_flags);
>  	ip_tunnel_flags_copy(t->parms.o_flags, p.o_flags);

[Severity: Medium]
This is a pre-existing issue, but when erspan_changelink() succeeds with
a different IFLA_GRE_ERSPAN_VER, ipgre_commit_parms() updates
t->erspan_ver and does not recompute tunnel->tun_hlen or tunnel->hlen.
In this patch those are derived from erspan_ver only in
erspan_tunnel_init():

net/ipv4/ip_gre.c:erspan_tunnel_init() {
    ...
	if (tunnel->erspan_ver == 0)
		tunnel->tun_hlen = 4; /* 4-byte GRE hdr. */
	else
		tunnel->tun_hlen = 8; /* 8-byte GRE hdr. */
    ...
}

__gre_xmit() builds the GRE header from tunnel->tun_hlen. Can a v0 to
v1/v2 change then produce malformed headers, with the sequence number
overwriting the flags/protocol, plus the wrong MTU and headroom?

The old code also changed erspan_ver without recomputing these lengths.
A later patch in the series, "ip_gre: recompute erspan header lengths
after a change", fixes this by calling erspan_link_update() from
erspan_changelink().

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