Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753469AbbKLTMX (ORCPT ); Thu, 12 Nov 2015 14:12:23 -0500 Received: from tiger.mobileactivedefense.com ([217.174.251.109]:38859 "EHLO tiger.mobileactivedefense.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752304AbbKLTMV (ORCPT ); Thu, 12 Nov 2015 14:12:21 -0500 From: Rainer Weikusat To: Jason Baron Cc: Rainer Weikusat , Dmitry Vyukov , syzkaller , Michal Kubecek , Al Viro , "linux-fsdevel\@vger.kernel.org" , LKML , David Miller , Hannes Frederic Sowa , David Howells , Paul Moore , salyzyn@android.com, sds@tycho.nsa.gov, ying.xue@windriver.com, netdev , Kostya Serebryany , Alexander Potapenko , Andrey Konovalov , Sasha Levin , Julien Tinnes , Kees Cook , Mathias Krause Subject: Re: [PATCH] unix: avoid use-after-free in ep_remove_wait_queue In-Reply-To: <56437C69.6090009@akamai.com> (Jason Baron's message of "Wed, 11 Nov 2015 12:35:37 -0500") References: <20151012120249.GB16370@unicorn.suse.cz> <1444652071.27760.156.camel@edumazet-glaptop2.roam.corp.google.com> <563CC002.5050307@akamai.com> <87ziyrcg67.fsf@doppelsaurus.mobileactivedefense.com> <87fv0fnslr.fsf_-_@doppelsaurus.mobileactivedefense.com> <877flp34fl.fsf@doppelsaurus.mobileactivedefense.com> <56437C69.6090009@akamai.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.4 (gnu/linux) Date: Thu, 12 Nov 2015 19:11:41 +0000 Message-ID: <87bnazt4lu.fsf@doppelsaurus.mobileactivedefense.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.4.3 (tiger.mobileactivedefense.com [217.174.251.109]); Thu, 12 Nov 2015 19:11:53 +0000 (GMT) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2574 Lines: 67 Jason Baron writes: >> + >> +/* Needs sk unix state lock. After recv_ready indicated not ready, >> + * establish peer_wait connection if still needed. >> + */ >> +static int unix_dgram_peer_wake_me(struct sock *sk, struct sock *other) >> +{ >> + int connected; >> + >> + connected = unix_dgram_peer_wake_connect(sk, other); >> + >> + if (unix_recvq_full(other)) >> + return 1; >> + >> + if (connected) >> + unix_dgram_peer_wake_disconnect(sk, other); >> + >> + return 0; >> +} >> + > > So the comment above this function says 'needs unix state lock', however > the usage in unix_dgram_sendmsg() has the 'other' lock, while the usage > in unix_dgram_poll() has the 'sk' lock. So this looks racy. That's one thing which is broken with this patch. Judging from a 'quick' look at the _dgram_sendmsg code, the unix_state_lock(other) will need to be turned into a unix_state_double_lock(sk, other) and the remaining code changed accordingly (since all of the checks must be done without unlocking other). There's also something else seriously wrong with the present patch: Some code in unix_dgram_connect presently (with this change) looks like this: /* * If it was connected, reconnect. */ if (unix_peer(sk)) { struct sock *old_peer = unix_peer(sk); unix_peer(sk) = other; if (unix_dgram_peer_wake_disconnect(sk, other)) wake_up_interruptible_poll(sk_sleep(sk), POLLOUT | POLLWRNORM | POLLWRBAND); unix_state_double_unlock(sk, other); if (other != old_peer) unix_dgram_disconnected(sk, old_peer); sock_put(old_peer); and trying to disconnect from a peer the socket is just being connected to is - of course - "flowering tomfoolery" (literal translation of the German "bluehender Bloedsinn") --- it needs to disconnect from old_peer instead. I'll address the suggestion and send an updated patch "later today" (may become "early tomorrow"). I have some code addressing both issues but that's part of a release of 'our' kernel fork, ie, 3.2.54-based I'll need to do 'soon'. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/