Re: use-after-free in sock_wake_async

[Eric Dumazet]

On Wed, 2015-11-25 at 18:24 +0000, Rainer Weikusat wrote:
> Eric Dumazet <eric.dumazet@xxxxxxxxx> writes:
> > On Wed, 2015-11-25 at 17:30 +0000, Rainer Weikusat wrote:
> >
> >> In case this is wrong, it obviously implies that sk_sleep(sk) must not
> >> be used anywhere as it accesses the same struck sock, hence, when that
> >> can "suddenly" disappear despite locks are used in the way indicated
> >> above, there is now safe way to invoke that, either, as it just does a
> >> rcu_dereference_raw based on the assumption that the caller knows that
> >> the i-node (and the corresponding wait queue) still exist.
> >> 
> >
> > Oh well.
> >
> > sk_sleep() is not used if the return is NULL
> 
> static long unix_stream_data_wait(struct sock *sk, long timeo,
> 				  struct sk_buff *last, unsigned int last_len)
> {
> 	struct sk_buff *tail;
> 	DEFINE_WAIT(wait);
> 
> 	unix_state_lock(sk);
> 
> 	for (;;) {
> 		prepare_to_wait(sk_sleep(sk), &wait, TASK_INTERRUPTIBLE);
> 
> 		tail = skb_peek_tail(&sk->sk_receive_queue);
> 		if (tail != last ||
> 		    (tail && tail->len != last_len) ||
> 		    sk->sk_err ||
> 		    (sk->sk_shutdown & RCV_SHUTDOWN) ||
> 		    signal_pending(current) ||
> 		    !timeo)
> 			break;
> 
> 		set_bit(SOCK_ASYNC_WAITDATA, &sk->sk_socket->flags);
> 		unix_state_unlock(sk);
> 		timeo = freezable_schedule_timeout(timeo);
> 		unix_state_lock(sk);
> 
> 		if (sock_flag(sk, SOCK_DEAD))
> 			break;
> 
> 		clear_bit(SOCK_ASYNC_WAITDATA, &sk->sk_socket->flags);
> 	}
> 
> 	finish_wait(sk_sleep(sk), &wait);
> 	unix_state_unlock(sk);
> 	return timeo;
> }
> 
> Neither prepare_to_wait nor finish_wait check if the pointer is
> null. For the finish_wait case, it shouldn't be null because if
> SOCK_DEAD is not found to be set after the unix_state_lock was acquired,
> unix_release_sock didn't execute the corresponding code yet, hence,
> inode etc will remain available until after the corresponding unlock.


> 
> But this isn't true anymore if the inode can go away despite
> sock_release couldn't complete yet.


You are looking at the wrong side.

Of course, the thread 'owning' a socket has a reference on it, so it
knows sk->sk_socket and sk->sk_ww is not NULL.

The problem is that at the time a wakeup is done, it can be done by a
process or softirq having no ref on the 'struct socket', as
sk->sk_socket can become NULL at anytime.

This is why we have sk_wq , and RCU protection, so that we do not have
to use expensive atomic operations in this fast path.




--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/