netdev - Re: use-after-free in sock_wake

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]

Date:	Wed, 25 Nov 2015 10:39:04 -0800
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	Rainer Weikusat <rweikusat@...ileactivedefense.com>
Cc:	Eric Dumazet <edumazet@...gle.com>,
	Dmitry Vyukov <dvyukov@...gle.com>,
	Benjamin LaHaise <bcrl@...ck.org>,
	"David S. Miller" <davem@...emloft.net>,
	Hannes Frederic Sowa <hannes@...essinduktion.org>,
	Al Viro <viro@...iv.linux.org.uk>,
	David Howells <dhowells@...hat.com>,
	Ying Xue <ying.xue@...driver.com>,
	"Eric W. Biederman" <ebiederm@...ssion.com>,
	netdev <netdev@...r.kernel.org>,
	LKML <linux-kernel@...r.kernel.org>,
	syzkaller <syzkaller@...glegroups.com>,
	Kostya Serebryany <kcc@...gle.com>,
	Alexander Potapenko <glider@...gle.com>,
	Sasha Levin <sasha.levin@...cle.com>
Subject: Re: use-after-free in sock_wake_async

On Wed, 2015-11-25 at 18:24 +0000, Rainer Weikusat wrote:
> Eric Dumazet <eric.dumazet@...il.com> 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 netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html