From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nicolas Dichtel Subject: Re: [PATCH net v2] netns, rtnetlink: fix struct net reference leak Date: Sat, 23 Dec 2017 23:12:25 +0100 Message-ID: <74a824ae-738d-2a42-8bfb-5f9ff08ce939@6wind.com> References: <20171222203626.113363-1-kraigatgoog@gmail.com> Reply-To: nicolas.dichtel@6wind.com Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Cc: netdev@vger.kernel.org, "Jason A . Donenfeld" To: Craig Gallek , David Miller , Jiri Benc Return-path: Received: from mail-wm0-f68.google.com ([74.125.82.68]:39812 "EHLO mail-wm0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752216AbdLWWMa (ORCPT ); Sat, 23 Dec 2017 17:12:30 -0500 Received: by mail-wm0-f68.google.com with SMTP id i11so27188225wmf.4 for ; Sat, 23 Dec 2017 14:12:29 -0800 (PST) In-Reply-To: <20171222203626.113363-1-kraigatgoog@gmail.com> Content-Language: fr Sender: netdev-owner@vger.kernel.org List-ID: Le 22/12/2017 à 21:36, Craig Gallek a écrit : > From: Craig Gallek > > 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 > CC: Nicolas Dichtel > CC: Jason A. Donenfeld > Signed-off-by: Craig Gallek > --- > 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); >