lkml.org 
[lkml]   [2018]   [Jan]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Subject[PATCH 4.9 43/75] net: Fix double free and memory corruption in get_net_ns_by_id()
    Date
    4.9-stable review patch.  If anyone has any objections, please let me know.

    ------------------

    From: "Eric W. Biederman" <ebiederm@xmission.com>


    [ Upstream commit 21b5944350052d2583e82dd59b19a9ba94a007f0 ]

    (I can trivially verify that that idr_remove in cleanup_net happens
    after the network namespace count has dropped to zero --EWB)

    Function get_net_ns_by_id() does not check for net::count
    after it has found a peer in netns_ids idr.

    It may dereference a peer, after its count has already been
    finaly decremented. This leads to double free and memory
    corruption:

    put_net(peer) rtnl_lock()
    atomic_dec_and_test(&peer->count) [count=0] ...
    __put_net(peer) get_net_ns_by_id(net, id)
    spin_lock(&cleanup_list_lock)
    list_add(&net->cleanup_list, &cleanup_list)
    spin_unlock(&cleanup_list_lock)
    queue_work() peer = idr_find(&net->netns_ids, id)
    | get_net(peer) [count=1]
    | ...
    | (use after final put)
    v ...
    cleanup_net() ...
    spin_lock(&cleanup_list_lock) ...
    list_replace_init(&cleanup_list, ..) ...
    spin_unlock(&cleanup_list_lock) ...
    ... ...
    ... put_net(peer)
    ... atomic_dec_and_test(&peer->count) [count=0]
    ... spin_lock(&cleanup_list_lock)
    ... list_add(&net->cleanup_list, &cleanup_list)
    ... spin_unlock(&cleanup_list_lock)
    ... queue_work()
    ... rtnl_unlock()
    rtnl_lock() ...
    for_each_net(tmp) { ...
    id = __peernet2id(tmp, peer) ...
    spin_lock_irq(&tmp->nsid_lock) ...
    idr_remove(&tmp->netns_ids, id) ...
    ... ...
    net_drop_ns() ...
    net_free(peer) ...
    } ...
    |
    v
    cleanup_net()
    ...
    (Second free of peer)

    Also, put_net() on the right cpu may reorder with left's cpu
    list_replace_init(&cleanup_list, ..), and then cleanup_list
    will be corrupted.

    Since cleanup_net() is executed in worker thread, while
    put_net(peer) can happen everywhere, there should be
    enough time for concurrent get_net_ns_by_id() to pick
    the peer up, and the race does not seem to be unlikely.
    The patch fixes the problem in standard way.

    (Also, there is possible problem in peernet2id_alloc(), which requires
    check for net::count under nsid_lock and maybe_get_net(peer), but
    in current stable kernel it's used under rtnl_lock() and it has to be
    safe. Openswitch begun to use peernet2id_alloc(), and possibly it should
    be fixed too. While this is not in stable kernel yet, so I'll send
    a separate message to netdev@ later).

    Cc: Nicolas Dichtel <nicolas.dichtel@6wind.com>
    Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
    Fixes: 0c7aecd4bde4 "netns: add rtnl cmd to add and get peer netns ids"
    Reviewed-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
    Reviewed-by: "Eric W. Biederman" <ebiederm@xmission.com>
    Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
    Reviewed-by: Eric Dumazet <edumazet@google.com>
    Acked-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
    Signed-off-by: David S. Miller <davem@davemloft.net>
    Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
    ---
    net/core/net_namespace.c | 2 +-
    1 file changed, 1 insertion(+), 1 deletion(-)

    --- a/net/core/net_namespace.c
    +++ b/net/core/net_namespace.c
    @@ -263,7 +263,7 @@ struct net *get_net_ns_by_id(struct net
    spin_lock_irqsave(&net->nsid_lock, flags);
    peer = idr_find(&net->netns_ids, id);
    if (peer)
    - get_net(peer);
    + peer = maybe_get_net(peer);
    spin_unlock_irqrestore(&net->nsid_lock, flags);
    rcu_read_unlock();


    \
     
     \ /
      Last update: 2018-01-01 16:19    [W:3.131 / U:0.224 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site