From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756992AbbJIOie (ORCPT ); Fri, 9 Oct 2015 10:38:34 -0400 Received: from out5-smtp.messagingengine.com ([66.111.4.29]:52776 "EHLO out5-smtp.messagingengine.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754331AbbJIOib (ORCPT ); Fri, 9 Oct 2015 10:38:31 -0400 X-Sasl-enc: CmVVf89LQPvVYTkTVbA0CTbQdTu/im+utzzOQmrSEyfg 1444401510 From: Hannes Frederic Sowa To: Jason Baron , davem@davemloft.net Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org, minipli@googlemail.com, normalperson@yhbt.net, eric.dumazet@gmail.com, rweikusat@mobileactivedefense.com, viro@zeniv.linux.org.uk, davidel@xmailserver.org, dave@stgolabs.net, olivier@mauras.ch, pageexec@freemail.hu, torvalds@linux-foundation.org, peterz@infradead.org, joe@perches.com Subject: Re: [PATCH v4 1/3] net: unix: fix use-after-free in unix_dgram_poll() In-Reply-To: <8f8dfc6fdb8015091b58509044b15489df261461.1444363559.git.jbaron@akamai.com> References: <8f8dfc6fdb8015091b58509044b15489df261461.1444363559.git.jbaron@akamai.com> User-Agent: Notmuch/0.19 (http://notmuchmail.org) Emacs/24.5.1 (x86_64-redhat-linux-gnu) Date: Fri, 09 Oct 2015 16:38:29 +0200 Message-ID: <87io6gayze.fsf@stressinduktion.org> MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, Jason Baron writes: > The unix_dgram_poll() routine calls sock_poll_wait() not only for the wait > queue associated with the socket s that we are poll'ing against, but also calls > sock_poll_wait() for a remote peer socket p, if it is connected. Thus, > if we call poll()/select()/epoll() for the socket s, there are then > a couple of code paths in which the remote peer socket p and its associated > peer_wait queue can be freed before poll()/select()/epoll() have a chance > to remove themselves from the remote peer socket. > > The way that remote peer socket can be freed are: > > 1. If s calls connect() to a connect to a new socket other than p, it will > drop its reference on p, and thus a close() on p will free it. > > 2. If we call close on p(), then a subsequent sendmsg() from s, will drop > the final reference to p, allowing it to be freed. > > Address this issue, by reverting unix_dgram_poll() to only register with > the wait queue associated with s and register a callback with the remote peer > socket on connect() that will wake up the wait queue associated with s. If > scenarios 1 or 2 occur above we then simply remove the callback from the > remote peer. This then presents the expected semantics to poll()/select()/ > epoll(). > > I've implemented this for sock-type, SOCK_RAW, SOCK_DGRAM, and SOCK_SEQPACKET > but not for SOCK_STREAM, since SOCK_STREAM does not use unix_dgram_poll(). > > Introduced in commit ec0d215f9420 ("af_unix: fix 'poll for write'/connected > DGRAM sockets"). > > Tested-by: Mathias Krause > Signed-off-by: Jason Baron While I think this approach works, I haven't seen where the current code leaks a reference. Assignment to unix_peer(sk) in general take spin_lock and increment refcount. Are there bugs at the two places you referred to? Is an easier fix just to use atomic_inc_not_zero(&sk->sk_refcnt) in unix_peer_get() which could also help other places? Thanks, Hannes