Thread (1 message) 1 message, 1 author, 2015-11-17

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".
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help