* [PATCH net v2] netns, rtnetlink: fix struct net reference leak
@ 2017-12-22 20:36 Craig Gallek
2017-12-23 22:12 ` Nicolas Dichtel
0 siblings, 1 reply; 3+ messages in thread
From: Craig Gallek @ 2017-12-22 20:36 UTC (permalink / raw)
To: David Miller, Jiri Benc; +Cc: netdev, Nicolas Dichtel, Jason A . Donenfeld
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;
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);
--
2.15.1.620.gb9897f4670-goog
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH net v2] netns, rtnetlink: fix struct net reference leak
2017-12-22 20:36 [PATCH net v2] netns, rtnetlink: fix struct net reference leak Craig Gallek
@ 2017-12-23 22:12 ` Nicolas Dichtel
2017-12-29 15:56 ` Craig Gallek
0 siblings, 1 reply; 3+ messages in thread
From: Nicolas Dichtel @ 2017-12-23 22:12 UTC (permalink / raw)
To: Craig Gallek, David Miller, Jiri Benc; +Cc: netdev, Jason A . Donenfeld
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);
>
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH net v2] netns, rtnetlink: fix struct net reference leak
2017-12-23 22:12 ` Nicolas Dichtel
@ 2017-12-29 15:56 ` Craig Gallek
0 siblings, 0 replies; 3+ messages in thread
From: Craig Gallek @ 2017-12-29 15:56 UTC (permalink / raw)
To: Nicolas Dichtel; +Cc: David Miller, Jiri Benc, netdev, Jason A . Donenfeld
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-12-29 15:56 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-12-22 20:36 [PATCH net v2] netns, rtnetlink: fix struct net reference leak Craig Gallek
2017-12-23 22:12 ` Nicolas Dichtel
2017-12-29 15:56 ` Craig Gallek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox