Re: [PATCH net 1/2] ip6: vti: Use ip6_tnl.net in vti6_changelink().

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
ip6: vti: Use ip6_tnl.net in vti6_changelink().

After commit 5e72ce3e3980 ("net: ipv6: Use link netns in newlink() of
rtnl_link_ops"), vti6_newlink() correctly resolves the per-netns vti6
hash via link_net. vti6_changelink() and vti6_update() were not
converted in that series and still read dev_net(dev) /
dev_net(t->dev), which diverge from the device's creation netns
after IFLA_NET_NS_FD migration. The result is a stale per-netns hash
entry; cleanup_net() of the original netns then walks freed memory.

> diff --git a/net/ipv6/ip6_vti.c b/net/ipv6/ip6_vti.c
> index ad5290be4dd6..dcb257411d6e 100644
> --- a/net/ipv6/ip6_vti.c
> +++ b/net/ipv6/ip6_vti.c
> @@ -722,10 +722,11 @@ vti6_tnl_change(struct ip6_tnl *t, const struct __ip6_tnl_parm *p,
>  static int vti6_update(struct ip6_tnl *t, struct __ip6_tnl_parm *p,
>  		       bool keep_mtu)
>  {
> -	struct net *net = dev_net(t->dev);
> -	struct vti6_net *ip6n = net_generic(net, vti6_net_id);
> +	struct net *net = t->net;
> +	struct vti6_net *ip6n;
>  	int err;

Does this fix introduce a regression by leaving a loophole in
vti6_siocdevprivate()?

While vti6_changelink() and vti6_update() now correctly use t->net,
vti6_siocdevprivate() still uses dev_net(dev) to locate the tunnel during
a tunnel modification ioctl:

net/ipv6/ip6_vti.c:vti6_siocdevprivate() {
	...
	struct net *net = dev_net(dev);
	...
	case SIOCCHGTUNNEL:
		...
		t = vti6_locate(net, &p1, cmd == SIOCADDTUNNEL);
		if (dev != ip6n->fb_tnl_dev && cmd == SIOCCHGTUNNEL) {
			if (t) {
				if (t->dev != dev) {
					err = -EEXIST;
					break;
				}
			} else
				t = netdev_priv(dev);

			err = vti6_update(t, &p1, false);
		}
	...
}

Because the collision check occurs in the new namespace (dev_net(dev)), but
vti6_update() now modifies the original namespace's hash table (t->net),
could an attacker in the new namespace configure their tunnel to perfectly
match the parameters of an existing victim tunnel in the original namespace?

Since the check in the new namespace finds no collision, it seems it bypasses
the error check. Then vti6_update() prepends the attacker's tunnel
into the original namespace's hash table, which might allow intercepting or
hijacking traffic destined for the victim tunnel.

Should vti6_siocdevprivate() also be updated to use t->net for collision
checks to prevent cross-namespace traffic hijacking?
-- 
pw-bot: cr