Re: [PATCH net v2] netns, rtnetlink: fix struct net reference leak

[Nicolas Dichtel <nicolas.dichtel@6wind.com>]

Le 22/12/2017 à 21:36, Craig Gallek a écrit :
> From: Craig Gallek <kraig@google.com>
> 
> netns ids were added in commit 0c7aecd4bde4 and defined as signed
> integers in both the kernel datastructures and the netlink interface.
> However, the semantics of the implementation assume that the ids
> are always greater than or equal to zero, except for an internal
> sentinal value NETNSA_NSID_NOT_ASSIGNED.
> 
> Several subsequent patches carried this pattern forward.  This patch
> updates all of the netlink input paths of this value to enforce the
> 'greater than or equal to zero' constraint.
> 
> This issue was discovered by syskaller.  It would set a negative
> value for a netns id and then repeatedly call the RTM_GETLINK.
> The put_net call in that path would not trigger for negative netns ids,
> caused a reference count leak, and eventually overflowed.  There are
> probably additional error paths that do not handle this situation
> correctly, but this was the only one I was able to trigger a real
> issue through.
> 
> Fixes: 0c7aecd4bde4 ("netns: add rtnl cmd to add and get peer netns ids")
> Fixes: 317f4810e45e ("rtnl: allow to create device with IFLA_LINK_NETNSID set")
> Fixes: 79e1ad148c84 ("rtnetlink: use netnsid to query interface")
> CC: Jiri Benc <jbenc@redhat.com>
> CC: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> CC: Jason A. Donenfeld <Jason@zx2c4.com>
> Signed-off-by: Craig Gallek <kraig@google.com>
> ---
>  net/core/net_namespace.c |  2 ++
>  net/core/rtnetlink.c     | 17 +++++++++++++----
>  2 files changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
> index 60a71be75aea..4b7ea33f5705 100644
> --- a/net/core/net_namespace.c
> +++ b/net/core/net_namespace.c
> @@ -627,6 +627,8 @@ static int rtnl_net_newid(struct sk_buff *skb, struct nlmsghdr *nlh,
>  		return -EINVAL;
>  	}
>  	nsid = nla_get_s32(tb[NETNSA_NSID]);
> +	if (nsid < 0)
> +		return -EINVAL;
No, this breaks the current behavior.
Look at alloc_netid(). If reqid is < 0, the kernel allocates an nsid with no
constraint. If reqid is >= 0, it tries to alloc the specified nsid.

>  
>  	if (tb[NETNSA_PID]) {
>  		peer = get_net_ns_by_pid(nla_get_u32(tb[NETNSA_PID]));
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index dabba2a91fc8..a928b8f081b8 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -1733,10 +1733,12 @@ static int rtnl_dump_ifinfo(struct sk_buff *skb, struct netlink_callback *cb)
>  			ifla_policy, NULL) >= 0) {
>  		if (tb[IFLA_IF_NETNSID]) {
>  			netnsid = nla_get_s32(tb[IFLA_IF_NETNSID]);
> -			tgt_net = get_target_net(skb, netnsid);
> -			if (IS_ERR(tgt_net)) {
> -				tgt_net = net;
> -				netnsid = -1;
> +			if (netnsid >= 0) {
> +				tgt_net = get_target_net(skb, netnsid);
I would prefer to put this test in get_target_net.

> +				if (IS_ERR(tgt_net)) {
> +					tgt_net = net;
> +					netnsid = -1;
Maybe using NETNSA_NSID_NOT_ASSIGNED is better? Same for the initialization of
this variable.

> +				}
>  			}
>  		}
>  
> @@ -2792,6 +2794,11 @@ static int rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
>  		if (tb[IFLA_LINK_NETNSID]) {
>  			int id = nla_get_s32(tb[IFLA_LINK_NETNSID]);
>  
> +			if (id < 0) {
> +				err =  -EINVAL;
> +				goto out;
> +			}
> +
This is not needed. get_net_ns_by_id() returns NULL if id is < 0.

>  			link_net = get_net_ns_by_id(dest_net, id);
>  			if (!link_net) {
>  				err =  -EINVAL;
> @@ -2883,6 +2890,8 @@ static int rtnl_getlink(struct sk_buff *skb, struct nlmsghdr *nlh,
>  
>  	if (tb[IFLA_IF_NETNSID]) {
>  		netnsid = nla_get_s32(tb[IFLA_IF_NETNSID]);
> +		if (netnsid < 0)
> +			return -EINVAL;
>  		tgt_net = get_target_net(skb, netnsid);
>  		if (IS_ERR(tgt_net))
>  			return PTR_ERR(tgt_net);
>