Re: [syzbot] [net?] KASAN: slab-use-after-free Read in unix_stream_read_actor (2)

[Kuniyuki Iwashima <kuniyu@amazon.com>]

From: Lizhi Xu <lizhi.xu@windriver.com>
Date: Thu, 5 Sep 2024 13:25:32 +0800
> The sock queue oob twice, the first manage_oob (in unix_stream_read_generic) peek next skb only,
> and the next skb is the oob skb, so if skb is oob skb we need use manage_oob dealwith it.

I think the correct fix should be like this.

The issue happens only when the head oob is consumed (!unix_skb_len(skb))
and the next skb is peeked.  Then, we can fallback to the else branch in
manage_oob().

#syz test

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index a1894019ebd5..9913a447b758 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -2654,51 +2654,49 @@ static int unix_stream_recv_urg(struct unix_stream_read_state *state)
 static struct sk_buff *manage_oob(struct sk_buff *skb, struct sock *sk,
 				  int flags, int copied)
 {
+	struct sk_buff *consumed_skb = NULL;
+	struct sk_buff *unread_skb = NULL;
 	struct unix_sock *u = unix_sk(sk);
 
-	if (!unix_skb_len(skb)) {
-		struct sk_buff *unlinked_skb = NULL;
-
-		spin_lock(&sk->sk_receive_queue.lock);
+	spin_lock(&sk->sk_receive_queue.lock);
 
+	if (!unix_skb_len(skb)) {
 		if (copied && (!u->oob_skb || skb == u->oob_skb)) {
 			skb = NULL;
 		} else if (flags & MSG_PEEK) {
 			skb = skb_peek_next(skb, &sk->sk_receive_queue);
 		} else {
-			unlinked_skb = skb;
+			consumed_skb = skb;
 			skb = skb_peek_next(skb, &sk->sk_receive_queue);
-			__skb_unlink(unlinked_skb, &sk->sk_receive_queue);
+			__skb_unlink(consumed_skb, &sk->sk_receive_queue);
 		}
 
-		spin_unlock(&sk->sk_receive_queue.lock);
-
-		consume_skb(unlinked_skb);
-	} else {
-		struct sk_buff *unlinked_skb = NULL;
+		if (!u->oob_skb || skb != u->oob_skb)
+			goto unlock;
+	}
 
-		spin_lock(&sk->sk_receive_queue.lock);
+	if (skb == u->oob_skb) {
+		if (copied) {
+			skb = NULL;
+		} else if (!(flags & MSG_PEEK)) {
+			WRITE_ONCE(u->oob_skb, NULL);
 
-		if (skb == u->oob_skb) {
-			if (copied) {
-				skb = NULL;
-			} else if (!(flags & MSG_PEEK)) {
-				WRITE_ONCE(u->oob_skb, NULL);
-
-				if (!sock_flag(sk, SOCK_URGINLINE)) {
-					__skb_unlink(skb, &sk->sk_receive_queue);
-					unlinked_skb = skb;
-					skb = skb_peek(&sk->sk_receive_queue);
-				}
-			} else if (!sock_flag(sk, SOCK_URGINLINE)) {
-				skb = skb_peek_next(skb, &sk->sk_receive_queue);
+			if (!sock_flag(sk, SOCK_URGINLINE)) {
+				__skb_unlink(skb, &sk->sk_receive_queue);
+				unread_skb = skb;
+				skb = skb_peek(&sk->sk_receive_queue);
 			}
+		} else if (!sock_flag(sk, SOCK_URGINLINE)) {
+			skb = skb_peek_next(skb, &sk->sk_receive_queue);
 		}
+	}
 
-		spin_unlock(&sk->sk_receive_queue.lock);
+unlock:
+	spin_unlock(&sk->sk_receive_queue.lock);
+
+	consume_skb(consumed_skb);
+	kfree_skb(unread_skb);
 
-		kfree_skb(unlinked_skb);
-	}
 	return skb;
 }
 #endif