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

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] [day] [month] [year] [list]

Date:   Fri, 29 Dec 2017 10:56:00 -0500
From:   Craig Gallek <kraigatgoog@...il.com>
To:     Nicolas Dichtel <nicolas.dichtel@...nd.com>
Cc:     David Miller <davem@...emloft.net>, Jiri Benc <jbenc@...hat.com>,
        netdev <netdev@...r.kernel.org>,
        "Jason A . Donenfeld" <Jason@...c4.com>
Subject: Re: [PATCH net v2] netns, rtnetlink: fix struct net reference leak

On Sat, Dec 23, 2017 at 5:12 PM, Nicolas Dichtel
<nicolas.dichtel@...nd.com> wrote:
> Le 22/12/2017 à 21:36, Craig Gallek a écrit :
>> From: Craig Gallek <kraig@...gle.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