Skip to content
  • Eric W. Biederman's avatar
    net: Fix double free and memory corruption in get_net_ns_by_id() · dd9a2648
    Eric W. Biederman authored
    
    [ Upstream commit 21b59443
    
     ]
    
    (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: default avatarKirill Tkhai <ktkhai@virtuozzo.com>
    Fixes: 0c7aecd4
    
     "netns: add rtnl cmd to add and get peer netns ids"
    Reviewed-by: default avatarAndrey Ryabinin <aryabinin@virtuozzo.com>
    Reviewed-by: default avatar"Eric W. Biederman" <ebiederm@xmission.com>
    Signed-off-by: default avatarEric W. Biederman <ebiederm@xmission.com>
    Reviewed-by: default avatarEric Dumazet <edumazet@google.com>
    Acked-by: default avatarNicolas Dichtel <nicolas.dichtel@6wind.com>
    Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
    Signed-off-by: default avatarGreg Kroah-Hartman <gregkh@linuxfoundation.org>
    dd9a2648