| Message ID | 20171222203626.113363-1-kraigatgoog@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net,v2] netns, rtnetlink: fix struct net reference leak | expand |
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); >
On Sat, Dec 23, 2017 at 5:12 PM, Nicolas Dichtel <nicolas.dichtel@6wind.com> wrote: > Le 22/12/2017 à 21:36, Craig Gallek a écrit : >> From: Craig Gallek <kraig@google.com> >> 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. Ah, thanks. alloc_netid does appear to do the right thing. In fact, this seems to be another clue to the problem. The current behavior is to allocate from [0,max) when the input value is negative and the problem seems to trigger when 0 is allocated. Changing this range to [1, max) fixes the problem, so there must be code elsewhere that does not handle the case where the id is zero properly... >> >> 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. Indeed, and by extension get_target_net does not need this check either (as it calls get_net_ns_by_id). I'm having trouble debugging this remotely, so I'll give it a whirl when I get back to the office next week. Thanks again for the pointers, Craig
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; 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); + if (IS_ERR(tgt_net)) { + tgt_net = net; + netnsid = -1; + } } } @@ -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; + } + 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);