netdev - Re: missing retval check of call_netdevice_notifiers in dev_change_net

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87tuz3jvvt.fsf@mellanox.com>
Date:   Mon, 22 Jun 2020 11:24:38 +0200
From:   Petr Machata <petrm@...lanox.com>
To:     "Jason A. Donenfeld" <Jason@...c4.com>
Cc:     "Eric W. Biederman" <ebiederm@...ssion.com>,
        Jiri Pirko <jiri@...lanox.com>,
        Johannes Berg <johannes@...solutions.net>,
        David Miller <davem@...emloft.net>,
        David Ahern <dsahern@...il.com>,
        Netdev <netdev@...r.kernel.org>
Subject: Re: missing retval check of call_netdevice_notifiers in dev_change_net_namespace


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