Re: [PATCH] unix: avoid use-after-free in ep_remove_wait_queue
From: Rainer Weikusat <hidden>
Date: 2015-11-17 18:38:49
Also in:
linux-fsdevel, lkml
Jason Baron [off-list ref] writes:
On 11/15/2015 01:32 PM, Rainer Weikusat wrote:quoted
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".