netdev - Re: [PATCH v1 net] af_unix: Update unix_sk(sk)->oob_skb under sk_receive

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: <dc8e67fac99c7a1d2cb36bff2217515116bf58cf.camel@redhat.com>
Date: Fri, 10 May 2024 09:53:25 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Kuniyuki Iwashima <kuniyu@...zon.com>
Cc: billy@...rlabs.sg, davem@...emloft.net, edumazet@...gle.com,
 kuba@...nel.org,  kuni1840@...il.com, netdev@...r.kernel.org
Subject: Re: [PATCH v1 net] af_unix: Update unix_sk(sk)->oob_skb under
 sk_receive_queue lock.

On Fri, 2024-05-10 at 14:03 +0900, Kuniyuki Iwashima wrote:
> From: Paolo Abeni <pabeni@...hat.com>
> Date: Thu, 09 May 2024 11:12:38 +0200
> > On Tue, 2024-05-07 at 10:00 -0700, Kuniyuki Iwashima wrote:
> > > Billy Jheng Bing-Jhong reported a race between __unix_gc() and
> > > queue_oob().
> > > 
> > > __unix_gc() tries to garbage-collect close()d inflight sockets,
> > > and then if the socket has MSG_OOB in unix_sk(sk)->oob_skb, GC
> > > will drop the reference and set NULL to it locklessly.
> > > 
> > > However, the peer socket still can send MSG_OOB message to the
> > > GC candidate and queue_oob() can update unix_sk(sk)->oob_skb
> > > concurrently, resulting in NULL pointer dereference. [0]
> > > 
> > > To avoid the race, let's update unix_sk(sk)->oob_skb under the
> > > sk_receive_queue's lock.
> > 
> > I'm sorry to delay this fix but...
> > 
> > AFAICS every time AF_UNIX touches the ooo_skb, it's under the receiver
> > unix_state_lock. The only exception is __unix_gc. What about just
> > acquiring such lock there?
> 
> In the new GC, there is unix_state_lock -> gc_lock ordering, and
> we need another fix then.
> 
> That's why I chose locking recvq for old GC too.
> https://lore.kernel.org/netdev/20240507172606.85532-1-kuniyu@amazon.com/
> 
> Also, Linus says:
> 
>     I really get the feeling that 'sb->oob_skb' should actually be forced
>     to always be in sync with the receive queue by always doing the
>     accesses under the receive_queue lock.
> 
> ( That's in the security@ thread I added you, but I just noticed
>   Linus replied to the previous mail.  I'll forward the mails to you. )
> 
> 
> > Otherwise there are other chunk touching the ooo_skb is touched where
> > this patch does not add the receive queue spin lock protection e.g. in
> > unix_stream_recv_urg(), making the code a bit inconsistent.
> 
> Yes, now the receive path is protected by unix_state_lock() and the
> send path is by unix_state_lock() and recvq lock.
> 
> Ideally, as Linus suggested, we should acquire recvq lock everywhere
> touching oob_skb and remove the additional refcount by skb_get(), but
> I thought it's too much as a fix and I would do that refactoring in
> the next cycle.
> 
> What do you think ?

I missed/forgot the unix_state_lock -> gc_lock ordering on net-next.

What about using the receive queue lock, and acquiring that everywhere
oob_skb is touched, without the additional refcount refactor?

Would be more consistent and reasonably small. It should work on the
new CG, too.

The refcount refactor could later come on net-next, and will be less
complex with the lock already in place.

Incremental patch on top of yours, completely untested:
---
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 9a6ad5974dff..a489f2aef29d 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -2614,8 +2614,10 @@ static int unix_stream_recv_urg(struct unix_stream_read_state *state)
 
 	mutex_lock(&u->iolock);
 	unix_state_lock(sk);
+	spin_lock(&sk->sk_receive_queue.lock);
 
 	if (sock_flag(sk, SOCK_URGINLINE) || !u->oob_skb) {
+		spin_unlock(&sk->sk_receive_queue.lock);
 		unix_state_unlock(sk);
 		mutex_unlock(&u->iolock);
 		return -EINVAL;
@@ -2627,6 +2629,7 @@ static int unix_stream_recv_urg(struct unix_stream_read_state *state)
 		WRITE_ONCE(u->oob_skb, NULL);
 	else
 		skb_get(oob_skb);
+	spin_unlock(&sk->sk_receive_queue.lock);
 	unix_state_unlock(sk);
 
 	chunk = state->recv_actor(oob_skb, 0, chunk, state);
@@ -2655,6 +2658,7 @@ static struct sk_buff *manage_oob(struct sk_buff *skb, struct sock *sk,
 		consume_skb(skb);
 		skb = NULL;
 	} else {
+		spin_lock(&sk->sk_receive_queue.lock);
 		if (skb == u->oob_skb) {
 			if (copied) {
 				skb = NULL;
@@ -2673,6 +2677,7 @@ static struct sk_buff *manage_oob(struct sk_buff *skb, struct sock *sk,
 				skb = skb_peek(&sk->sk_receive_queue);
 			}
 		}
+		spin_unlock(&sk->sk_receive_queue.lock);
 	}
 	return skb;
 }