Jason A. Donenfeld <ja...@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?