missing retval check of call_netdevice_notifiers in dev_change_net_namespace

missing retval check of call_netdevice_notifiers in dev_change_net_namespace

[Jason A. Donenfeld]

Hi,

In register_netdevice, there's a call to
call_netdevice_notifiers(NETDEV_REGISTER), whose return value is
checked to determine whether or not to roll back the device
registration. The reason checking that return value is important is
because this represents an opportunity for the various notifiers to
veto the registration, or for another error to happen. It's good that
the error value is checked when that function is called from
register_netdevice. But from other functions, the error value is not
always checked.

The notification is split up into two stages:

        ret = raw_notifier_call_chain(&net->netdev_chain, val, info);
        if (ret & NOTIFY_STOP_MASK)
                return ret;
        return raw_notifier_call_chain(&netdev_chain, val, info);

One is per-net and the other is global. So there's ample space for
something in either chain to abort the whole process.

The wireguard module uses the notifier to keep track of netns changes
in order to do some reference count bookkeeping. If this bookkeeping
goes wrong, there's UaF potential. However, I noticed that the call to
call_netdevice_notifiers(NETDEV_REGISTER) at the end of
dev_change_net_namespace doesn't check its return value and doesn't
implement any sort of rollback like register_netdevice does:

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.

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.

Another idea would be to have a different notifier for netns changes
(instead of overloading NETDEV_REGISTER as is the case now), whose
entire chain always gets called, where the return value is always
ignored. This seems like it'd be a bit better, and at least explicit
about its purpose. But that'd be a quasi-new thing.

Finally, it could be the solution is that modules that use the netdev
notifier never really rely too heavily upon getting notifications, and
if they need something reliable, then they rearchitect their code to
not need that. I could imagine this attitude holding sway here
somehow, but it's also not very appealing from a code writing and
refactoring perspective.

I figured before I go to town coding one of these up, maybe I should
bring up the issue here, in case anybody has more developed thoughts
than me about it.

Thanks,
Jason

Re: missing retval check of call_netdevice_notifiers in dev_change_net_namespace

[David Ahern]

+Ido

On 6/21/20 1:58 AM, Jason A. Donenfeld wrote:
> 
> Finally, it could be the solution is that modules that use the netdev
> notifier never really rely too heavily upon getting notifications, and
> if they need something reliable, then they rearchitect their code to
> not need that. I could imagine this attitude holding sway here
> somehow, but it's also not very appealing from a code writing and
> refactoring perspective.
> 

The notifiers should be reliable and callers should handle the errors
when a notifier vetoes the change. Anything else leaves networking in an
unknown / inconsistent state.

Re: missing retval check of call_netdevice_notifiers in dev_change_net_namespace

[Petr Machata]

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?

Re: missing retval check of call_netdevice_notifiers in dev_change_net_namespace

[Eric W. Biederman]

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

> Hi,

Adding Herbert Xu who added support for failing notifications in
fcc5a03ac425 ("[NET]: Allow netdev REGISTER/CHANGENAME events to fail").

He might have some insight but 2007 was a long time ago.

All I remember is the ability of NETDEV_CHANGENAME to fail was abused by
sysfs to add a userspace ABI regression, and sysfs had to be fixed
not to do that.


> In register_netdevice, there's a call to
> call_netdevice_notifiers(NETDEV_REGISTER), whose return value is
> checked to determine whether or not to roll back the device
> registration. The reason checking that return value is important is
> because this represents an opportunity for the various notifiers to
> veto the registration, or for another error to happen. It's good that
> the error value is checked when that function is called from
> register_netdevice. But from other functions, the error value is not
> always checked.
>
> The notification is split up into two stages:
>
>         ret = raw_notifier_call_chain(&net->netdev_chain, val, info);
>         if (ret & NOTIFY_STOP_MASK)
>                 return ret;
>         return raw_notifier_call_chain(&netdev_chain, val, info);
>
> One is per-net and the other is global. So there's ample space for
> something in either chain to abort the whole process.
>
> The wireguard module uses the notifier to keep track of netns changes
> in order to do some reference count bookkeeping. If this bookkeeping
> goes wrong, there's UaF potential. However, I noticed that the call to
> call_netdevice_notifiers(NETDEV_REGISTER) at the end of
> dev_change_net_namespace doesn't check its return value and doesn't
> implement any sort of rollback like register_netdevice does:
>
> 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.
>
> 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.

In general we are talking a subsystem not a device that will veto
the change.  Implementations of devices should not need notifiers,
as they have enough methods.

It requires a level above a network device to care about your network
namespace.  Now wireguard happens to be both. So you care but I don't
believe an ordinary device will.

Nitpicks aside, if we are going to make sense of this we need to find
actualy notifiers that veto the change and look at them.

> 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.

The entire code consists of walking a linked list and calling the
callback function at each node on the list.   There is nothing
bad that can happen except list corruption and callback functions
that do silly things.  AKA implementation bugs.

There are no expected failures in walking a linked list.  So it should
be perfectly valid to rely on receiving your notification.

If callback function allocates memory or does something that is not 100%
reliable in a notifier that is a different issue and it is that
subystems problem.

> 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.

Semantically it can't happen.  Once we reach dev_close we have caused
userspace visible effects that simply can not be undone.  So the code
has to validate that the change can happen before we it reaches
dev_close.

Which unfortunately makes the code incompatible with the concept of
black boxes on the notification chain adding failure cases.

It wouldn't hurt to add a warning:

	err = call_netdevice_notifiers(NETDEV_REGISTER, dev)
        WARN_ON_ONCE(err).

Just to validate the assumption that nothing ever happens.

> Finally, it could be the solution is that modules that use the netdev
> notifier never really rely too heavily upon getting notifications, and
> if they need something reliable, then they rearchitect their code to
> not need that. I could imagine this attitude holding sway here
> somehow, but it's also not very appealing from a code writing and
> refactoring perspective.

Perhaps we can architect the failure case out of NETDEV_REGISTER so
people don't have to worry about it.

> I figured before I go to town coding one of these up, maybe I should
> bring up the issue here, in case anybody has more developed thoughts
> than me about it.

I hope this gives you some insight.  I think this may be a case where
the code allows more than anyone has actually taken advantage of.

Eric

Re: missing retval check of call_netdevice_notifiers in dev_change_net_namespace

[Herbert Xu]

On Mon, Jun 22, 2020 at 12:43:53PM -0500, Eric W. Biederman wrote:
> 
> Adding Herbert Xu who added support for failing notifications in
> fcc5a03ac425 ("[NET]: Allow netdev REGISTER/CHANGENAME events to fail").
> 
> He might have some insight but 2007 was a long time ago.

https://lists.openwall.net/netdev/2007/07/26/61

AFAICS this is still needed.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt