netdev - Re: [PATCH] unix: avoid use-after-free in ep_remove_wait

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87fv04wjwm.fsf@doppelsaurus.mobileactivedefense.com>
Date:	Tue, 17 Nov 2015 18:38:49 +0000
From:	Rainer Weikusat <rweikusat@...ileactivedefense.com>
To:	Jason Baron <jbaron@...mai.com>
Cc:	Rainer Weikusat <rweikusat@...ileactivedefense.com>,
	Dmitry Vyukov <dvyukov@...gle.com>,
	syzkaller <syzkaller@...glegroups.com>,
	Michal Kubecek <mkubecek@...e.cz>,
	Al Viro <viro@...iv.linux.org.uk>,
	"linux-fsdevel\@vger.kernel.org" <linux-fsdevel@...r.kernel.org>,
	LKML <linux-kernel@...r.kernel.org>,
	David Miller <davem@...emloft.net>,
	Hannes Frederic Sowa <hannes@...essinduktion.org>,
	David Howells <dhowells@...hat.com>,
	Paul Moore <paul@...l-moore.com>, salyzyn@...roid.com,
	sds@...ho.nsa.gov, ying.xue@...driver.com,
	netdev <netdev@...r.kernel.org>,
	Kostya Serebryany <kcc@...gle.com>,
	Alexander Potapenko <glider@...gle.com>,
	Andrey Konovalov <andreyknvl@...gle.com>,
	Sasha Levin <sasha.levin@...cle.com>,
	Julien Tinnes <jln@...gle.com>,
	Kees Cook <keescook@...gle.com>,
	Mathias Krause <minipli@...glemail.com>
Subject: Re: [PATCH] unix: avoid use-after-free in ep_remove_wait_queue

Jason Baron <jbaron@...mai.com> writes:
> On 11/15/2015 01:32 PM, Rainer Weikusat wrote:
>
>> 
>> That was my original idea. The problem with this is that the code
>> starting after the _lock and running until the main code path unlock has
>> to be executed in one go with the other lock held as the results of the
>> tests above this one may become invalid as soon as the other lock is
>> released. This means instead of continuing execution with the send code
>> proper after the block in case other became receive-ready between the
>> first and the second test (possible because _dgram_recvmsg does not
>> take the unix state lock), the whole procedure starting with acquiring
>> the other lock would need to be restarted. Given sufficiently unfavorable
>> circumstances, this could even turn into an endless loop which couldn't
>> be interrupted. (unless code for this was added). 
>> 
>
> hmmm - I think we can avoid it by doing the wakeup from the write path
> in the rare case that the queue has emptied - and avoid the double lock. IE:
>
>                 unix_state_unlock(other);
>                 unix_state_lock(sk);
>                 err = -EAGAIN;
>                 if (unix_peer(sk) == other) {
>                        unix_dgram_peer_wake_connect(sk, other);
>                        if (skb_queue_len(&other->sk_receive_queue) == 0)
>                                need_wakeup = true;
>                 }
>                 unix_state_unlock(sk);
>                 if (need_wakeup)
>                         wake_up_interruptible_poll(sk_sleep(sk), POLLOUT
> | POLLWRNORM | POLLWRBAND);
>                 goto out_free;

This should probably rather be

if (unix_dgram_peer_wake_connect(sk, other) &&
    skb_queue_len(&other->sk_receive_queue) == 0)
	need_wakeup = 1;

as there's no need to do the wake up if someone else already connected
and then, the double lock could be avoided at the expense of returning a
gratuitous EAGAIN to the caller and throwing all of the work
_dgram_sendmsg did so far, eg, allocate a skb, copy the data into the
kernel, do all the other checks, away.

This would enable another thread to do one of the following things in
parallell with the 'locked' part of _dgram_sendmsg

	1) connect sk to a socket != other
        2) use sk to send to a socket != other
        3) do a shutdown on sk
        4) determine write-readyness of sk via poll callback

IMHO, the only thing which could possibly matter is 2) and my suggestion
for this would rather be "use a send socket per sending thread if this
matters to you" than "cause something to fail which could as well
have succeeded".
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html