From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nicolas Dichtel Subject: Re: [PATCH] net: Fix double free and memory corruption in get_net_ns_by_id() Date: Wed, 20 Dec 2017 16:00:19 +0100 Message-ID: <22d5677f-4ad3-98e6-0fed-2648c96edfb0@6wind.com> References: <87vah3aj1b.fsf@xmission.com> <20171218.152320.827340066666299976.davem@davemloft.net> <87d13aaaqr.fsf@xmission.com> Reply-To: nicolas.dichtel@6wind.com Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Cc: David Miller , ktkhai@virtuozzo.com, security@kernel.org, secalert@redhat.com, eric.dumazet@gmail.com, stephen@networkplumber.org To: "Eric W. Biederman" , netdev@vger.kernel.org Return-path: Received: from mail-wm0-f66.google.com ([74.125.82.66]:46701 "EHLO mail-wm0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753868AbdLTPAZ (ORCPT ); Wed, 20 Dec 2017 10:00:25 -0500 Received: by mail-wm0-f66.google.com with SMTP id r78so10476611wme.5 for ; Wed, 20 Dec 2017 07:00:24 -0800 (PST) In-Reply-To: <87d13aaaqr.fsf@xmission.com> Content-Language: fr Sender: netdev-owner@vger.kernel.org List-ID: Le 19/12/2017 à 18:27, Eric W. Biederman a écrit : > > (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 > Signed-off-by: Kirill Tkhai > Fixes: 0c7aecd4bde4 "netns: add rtnl cmd to add and get peer netns ids" > Reviewed-by: Andrey Ryabinin > Reviewed-by: "Eric W. Biederman" > Signed-off-by: Eric W. Biederman Good catch, thank you. Acked-by: Nicolas Dichtel