[PATCH] af_unix: fix holding spinlock in oob handling

From: Rao Shoaib <redacted>

syzkaller found that OOB code was holding spinlock
while calling a function in which it could sleep.

Reported-by: syzbot+8760ca6c1ee783ac4abd@syzkaller.appspotmail.com

Fixes: 314001f0bf92 ("af_unix: Add OOB support")

Signed-off-by: Rao Shoaib <redacted>
---
 net/unix/af_unix.c | 36 ++++++++++++++++++++++++------------
 1 file changed, 24 insertions(+), 12 deletions(-)

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 00d8b08cdbe1..a626e52c629a 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c

@@ -2362,19 +2362,37 @@ static int unix_stream_recv_urg(struct unix_stream_read_state *state)
 	struct sock *sk = sock->sk;
 	struct unix_sock *u = unix_sk(sk);
 	int chunk = 1;
+	struct sk_buff *oob_skb;
 
-	if (sock_flag(sk, SOCK_URGINLINE) || !u->oob_skb)
+	mutex_lock(&u->iolock);
+	unix_state_lock(sk);
+
+	if (sock_flag(sk, SOCK_URGINLINE) || !u->oob_skb) {
+		unix_state_unlock(sk);
+		mutex_unlock(&u->iolock);
 		return -EINVAL;
+	}
 
-	chunk = state->recv_actor(u->oob_skb, 0, chunk, state);
-	if (chunk < 0)
-		return -EFAULT;
+	oob_skb = u->oob_skb;
 
 	if (!(state->flags & MSG_PEEK)) {
-		UNIXCB(u->oob_skb).consumed += 1;
-		kfree_skb(u->oob_skb);
 		u->oob_skb = NULL;
 	}
+
+	unix_state_unlock(sk);
+
+	chunk = state->recv_actor(oob_skb, 0, chunk, state);
+
+	if (!(state->flags & MSG_PEEK)) {
+		UNIXCB(oob_skb).consumed += 1;
+		kfree_skb(oob_skb);
+	}
+
+	mutex_unlock(&u->iolock);
+
+	if (chunk < 0)
+		return -EFAULT;
+
 	state->msg->msg_flags |= MSG_OOB;
 	return 1;
 }

@@ -2434,13 +2452,7 @@ static int unix_stream_read_generic(struct unix_stream_read_state *state,
 	if (unlikely(flags & MSG_OOB)) {
 		err = -EOPNOTSUPP;
 #if IS_ENABLED(CONFIG_AF_UNIX_OOB)
-		mutex_lock(&u->iolock);
-		unix_state_lock(sk);
-
 		err = unix_stream_recv_urg(state);
-
-		unix_state_unlock(sk);
-		mutex_unlock(&u->iolock);
 #endif
 		goto out;
 	}

-- 
2.27.0

On Thu, Aug 12, 2021 at 12:07 AM Rao Shoaib [off-list ref] wrote:

From: Rao Shoaib <redacted>

syzkaller found that OOB code was holding spinlock
while calling a function in which it could sleep.

Reported-by: syzbot+8760ca6c1ee783ac4abd@syzkaller.appspotmail.com

Fixes: 314001f0bf92 ("af_unix: Add OOB support")

Signed-off-by: Rao Shoaib <redacted>
---

Please do not add these empty lines.

Fixes: ...
Reported-by: ...
Signed-off-by: ...

Also you might take a look at queue_oob()

1)  Setting skb->len tp 1 should not be needed, skb_put() already does that
2) After unix_state_lock(other); we probably need to check status of
the other socket.
3) Some skb_free() calls should have been consume_skb()

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index ec02e70a549b42f6c102253508c48426a13f7bc4..0c27e2976f9d234ca3bb131731375bc51a056846

100644

--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c

@@ -1908,7 +1908,6 @@ static int queue_oob(struct socket *sock, struct

msghdr *msg, struct sock *other
                return err;

        skb_put(skb, 1);
-       skb->len = 1;
        err = skb_copy_datagram_from_iter(skb, 0, &msg->msg_iter, 1);

        if (err) {

@@ -1917,11 +1916,17 @@ static int queue_oob(struct socket *sock,

struct msghdr *msg, struct sock *other
        }

        unix_state_lock(other);
+       if (sock_flag(other, SOCK_DEAD) ||
+           (other->sk_shutdown & RCV_SHUTDOWN)) {
+               unix_state_unlock(other);
+               kfree_skb(skb);
+               return -EPIPE;
+       }
        maybe_add_creds(skb, sock, other);
        skb_get(skb);

        if (ousk->oob_skb)
-               kfree_skb(ousk->oob_skb);
+               consume_skb(ousk->oob_skb);

        ousk->oob_skb = skb;

Hi Eric,

Thanks for your review I will take care of the comments.

Shoaib

On 8/12/21 12:53 AM, Eric Dumazet wrote:

On 8/12/21 12:53 AM, Eric Dumazet wrote:

         if (ousk->oob_skb)
-               kfree_skb(ousk->oob_skb);
+               consume_skb(ousk->oob_skb);

Should I be using consume_skb(), as the skb is not being consumed, the 
ref count is decremented and if zero skb will be freed.

Shoaib

On Thu, Aug 12, 2021 at 7:37 PM Shoaib Rao [off-list ref] wrote:

On 8/12/21 12:53 AM, Eric Dumazet wrote:

quoted

         if (ousk->oob_skb)
-               kfree_skb(ousk->oob_skb);
+               consume_skb(ousk->oob_skb);

Should I be using consume_skb(), as the skb is not being consumed, the
ref count is decremented and if zero skb will be freed.

consume_skb() and kfree_skb() have the same ref count handling.

The difference is that kfree_skb() is used by convention when a packet
is dropped

Admins can look closely at packet drops with drop_monitor, or :

perf record -a -g -e skb:kfree_skb sleep 10
perf report

In your case, the oob_skb is not really dropped. It is replaced by
another one, it is part of the normal operation.

On 8/12/21 1:33 PM, Eric Dumazet wrote:

On Thu, Aug 12, 2021 at 7:37 PM Shoaib Rao [off-list ref] wrote:

quoted

On 8/12/21 12:53 AM, Eric Dumazet wrote:

quoted

          if (ousk->oob_skb)
-               kfree_skb(ousk->oob_skb);
+               consume_skb(ousk->oob_skb);

Should I be using consume_skb(), as the skb is not being consumed, the
ref count is decremented and if zero skb will be freed.

consume_skb() and kfree_skb() have the same ref count handling.

The difference is that kfree_skb() is used by convention when a packet
is dropped

Admins can look closely at packet drops with drop_monitor, or :

perf record -a -g -e skb:kfree_skb sleep 10
perf report

In your case, the oob_skb is not really dropped. It is replaced by
another one, it is part of the normal operation.

Thanks a lot for the explanation. This was very helpful. In my case the 
skb may be dropped (oob was not read but the read has passed beyond oob, 
or could become part of normal data). Anyways, I will change it to use 
consume_skb().

Regards,

Shoaib

On Wed, 11 Aug 2021 15:06:52 -0700 Rao Shoaib wrote:

From: Rao Shoaib <redacted>

syzkaller found that OOB code was holding spinlock
while calling a function in which it could sleep.

Reported-by: syzbot+8760ca6c1ee783ac4abd@syzkaller.appspotmail.com

Fixes: 314001f0bf92 ("af_unix: Add OOB support")

Signed-off-by: Rao Shoaib <redacted>

IIUC issues pointed out by Eric are separate so I removed the spacing
between the tags and applied, thanks!

OH, I just sent you a combined patch. I will resend it with just the 
nits pointed out by Eric.

Shoaib

On 8/13/21 10:32 AM, Jakub Kicinski wrote:

On Wed, 11 Aug 2021 15:06:52 -0700 Rao Shoaib wrote:

quoted

From: Rao Shoaib <redacted>

syzkaller found that OOB code was holding spinlock
while calling a function in which it could sleep.

Reported-by: syzbot+8760ca6c1ee783ac4abd@syzkaller.appspotmail.com

Fixes: 314001f0bf92 ("af_unix: Add OOB support")

Signed-off-by: Rao Shoaib <redacted>

IIUC issues pointed out by Eric are separate so I removed the spacing
between the tags and applied, thanks!