Re: missing retval check of call_netdevice_notifiers in dev_change_net_namespace

[Petr Machata <petrm@mellanox.com>]

Jason A. Donenfeld <Jason@zx2c4.com> writes:

> int dev_change_net_namespace(struct net_device *dev, struct net *net,
> const char *pat)
> {
>        struct net *net_old = dev_net(dev);
>        int err, new_nsid, new_ifindex;
>
>        ASSERT_RTNL();
> [...]
>         /* Add the device back in the hashes */
>         list_netdevice(dev);
>
>         /* Notify protocols, that a new device appeared. */
>         call_netdevice_notifiers(NETDEV_REGISTER, dev);
> [...]
> }
>
> Notice that call_netdevice_notifiers isn't checking it's return value there.

I was wondering if the logic is the chance to veto namespace change is
simply setting a NETIF_F_NETNS_LOCAL flag. But that doesn't sound right.
It looks like there are use cases for vetoing the netns motion, e.g.
moving of an offloaded gre/tap underlay. So it sounds like this should
be checked for vetoes.

> It seems like if any device vetoes the notification chain, it's bad
> news bears for modules that depend on getting a netns change
> notification.
>
> I've been trying to audit the various registered notifiers to see if
> any of them pose a risk for wireguard. There are also unexpected
> errors that can happen, such as OOM conditions for kmalloc(GFP_KERNEL)
> or vmalloc and suchlike, which might be influenceable by attackers. In
> other words, relying on those notifications always being delivered
> seems kind of brittle. Not _super_ brittle, but brittle enough that
> it's at the moment making me a bit nervous. (See: UaF potential.)
>
> I've been trying to come up with a good solution to this.
>
> I'm not sure how reasonable it'd be to implement rollback inside of
> dev_change_net_namespace, but I guess that could technically be
> possible. The way that'd work would be that when vetoed, the function
> would complete, but then would start over again (a "goto top" sort of
> pattern), with oldnet and newnet reversed. But that could of course
> fail too and we could get ourselves in some sort of infinite loop. Not
> good.

I think it would be fair to just ignore the veto during the backward
motion. Perhaps with a WARN_ON, because it is indicative of a bug in
whoever vetoed it: how was the creation not vetoed when the motion back
is?