netdev - Re: [PATCH net] rtnetlink: Return correct error on changing device netns

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Thu, 26 Aug 2021 11:15:22 -0500
From:   ebiederm@...ssion.com (Eric W. Biederman)
To:     Andrey Ignatov <rdna@...com>
Cc:     <netdev@...r.kernel.org>, <davem@...emloft.net>, <kuba@...nel.org>,
        <kernel-team@...com>
Subject: Re: [PATCH net] rtnetlink: Return correct error on changing device netns

Andrey Ignatov <rdna@...com> writes:

> Currently when device is moved between network namespaces using
> RTM_NEWLINK message type and one of netns attributes (FLA_NET_NS_PID,
> IFLA_NET_NS_FD, IFLA_TARGET_NETNSID) but w/o specifying IFLA_IFNAME, and
> target namespace already has device with same name, userspace will get
> EINVAL what is confusing and makes debugging harder.
>
> Fix it so that userspace gets more appropriate EEXIST instead what makes
> debugging much easier.
>
> Before:
>
>   # ./ifname.sh
>   + ip netns add ns0
>   + ip netns exec ns0 ip link add l0 type dummy
>   + ip netns exec ns0 ip link show l0
>   8: l0: <BROADCAST,NOARP> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000
>       link/ether 66:90:b5:d5:78:69 brd ff:ff:ff:ff:ff:ff
>   + ip link add l0 type dummy
>   + ip link show l0
>   10: l0: <BROADCAST,NOARP> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000
>       link/ether 6e:c6:1f:15:20:8d brd ff:ff:ff:ff:ff:ff
>   + ip link set l0 netns ns0
>   RTNETLINK answers: Invalid argument
>
> After:
>
>   # ./ifname.sh
>   + ip netns add ns0
>   + ip netns exec ns0 ip link add l0 type dummy
>   + ip netns exec ns0 ip link show l0
>   8: l0: <BROADCAST,NOARP> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000
>       link/ether 1e:4a:72:e3:e3:8f brd ff:ff:ff:ff:ff:ff
>   + ip link add l0 type dummy
>   + ip link show l0
>   10: l0: <BROADCAST,NOARP> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000
>       link/ether f2:fc:fe:2b:7d:a6 brd ff:ff:ff:ff:ff:ff
>   + ip link set l0 netns ns0
>   RTNETLINK answers: File exists
>
> The problem is that do_setlink() passes its `char *ifname` argument,
> that it gets from a caller, to __dev_change_net_namespace() as is (as
> `const char *pat`), but semantics of ifname and pat can be different.
>
> For example, __rtnl_newlink() does this:
>
> net/core/rtnetlink.c
>     3270	char ifname[IFNAMSIZ];
>      ...
>     3286	if (tb[IFLA_IFNAME])
>     3287		nla_strscpy(ifname, tb[IFLA_IFNAME], IFNAMSIZ);
>     3288	else
>     3289		ifname[0] = '\0';
>      ...
>     3364	if (dev) {
>      ...
>     3394		return do_setlink(skb, dev, ifm, extack, tb, ifname, status);
>     3395	}
>
> , i.e. do_setlink() gets ifname pointer that is always valid no matter
> if user specified IFLA_IFNAME or not and then do_setlink() passes this
> ifname pointer as is to __dev_change_net_namespace() as pat argument.
>
> But the pat (pattern) in __dev_change_net_namespace() is used as:
>
> net/core/dev.c
>    11198	err = -EEXIST;
>    11199	if (__dev_get_by_name(net, dev->name)) {
>    11200		/* We get here if we can't use the current device name */
>    11201		if (!pat)
>    11202			goto out;
>    11203		err = dev_get_valid_name(net, dev, pat);
>    11204		if (err < 0)
>    11205			goto out;
>    11206	}
>
> As the result the `goto out` path on line 11202 is neven taken and
> instead of returning EEXIST defined on line 11198,
> __dev_change_net_namespace() returns an error from dev_get_valid_name()
> and this, in turn, will be EINVAL for ifname[0] = '\0' set earlier.
>
> Fixes: d8a5ec672768 ("[NET]: netlink support for moving devices between network namespaces.")
> Signed-off-by: Andrey Ignatov <rdna@...com>

The analysis and the fix looks good to me.

The code calling do_setlink is inconsistent.  One caller of do_setlink
passes a NULL to indicate not name has been specified.  Other callers
pass a string of zero bytes to indicate no name has been specified.

I wonder if we might want to fix the callers to uniformly pass NULL,
instead of a string of length zero.

There is a slight chance this will trigger a regression somewhere
because we are changing the error code but this change looks easy enough
to revert in the unlikely event this breaks existing userspace.

Reviewed-by: "Eric W. Biederman" <ebiederm@...ssion.com>

> ---
>  net/core/rtnetlink.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index f6af3e74fc44..662eb1c37f47 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -2608,6 +2608,7 @@ static int do_setlink(const struct sk_buff *skb,
>  		return err;
>  
>  	if (tb[IFLA_NET_NS_PID] || tb[IFLA_NET_NS_FD] || tb[IFLA_TARGET_NETNSID]) {
> +		const char *pat = ifname && ifname[0] ? ifname : NULL;
>  		struct net *net;
>  		int new_ifindex;
>  
> @@ -2623,7 +2624,7 @@ static int do_setlink(const struct sk_buff *skb,
>  		else
>  			new_ifindex = 0;
>  
> -		err = __dev_change_net_namespace(dev, net, ifname, new_ifindex);
> +		err = __dev_change_net_namespace(dev, net, pat, new_ifindex);
>  		put_net(net);
>  		if (err)
>  			goto errout;