[PATCH] af_unix: fix holding spinlock in oob handling

[PATCH] af_unix: fix holding spinlock in oob handling

[Rao Shoaib]

From: Rao Shoaib <rao.shoaib@oracle.com>

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 <rao.shoaib@oracle.com>
---
 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

Re: [PATCH] af_unix: fix holding spinlock in oob handling

[Eric Dumazet]

On Thu, Aug 12, 2021 at 12:07 AM Rao Shoaib <Rao.Shoaib@oracle.com> wrote:
>
> From: Rao Shoaib <rao.shoaib@oracle.com>
>
> 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 <rao.shoaib@oracle.com>
> ---

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;

Re: [PATCH] af_unix: fix holding spinlock in oob handling

[Shoaib Rao]

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 Thu, Aug 12, 2021 at 12:07 AM Rao Shoaib <Rao.Shoaib@oracle.com> wrote:
>> From: Rao Shoaib <rao.shoaib@oracle.com>
>>
>> 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 <rao.shoaib@oracle.com>
>> ---
> 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;

Re: [PATCH] af_unix: fix holding spinlock in oob handling

[Shoaib Rao]

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

Re: [PATCH] af_unix: fix holding spinlock in oob handling

[Eric Dumazet]

On Thu, Aug 12, 2021 at 7:37 PM Shoaib Rao <rao.shoaib@oracle.com> 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.
>

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.

Re: [PATCH] af_unix: fix holding spinlock in oob handling

[Shoaib Rao]

On 8/12/21 1:33 PM, Eric Dumazet wrote:
> On Thu, Aug 12, 2021 at 7:37 PM Shoaib Rao <rao.shoaib@oracle.com> 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.
>>
> 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

Re: [PATCH] af_unix: fix holding spinlock in oob handling

[Jakub Kicinski]

On Wed, 11 Aug 2021 15:06:52 -0700 Rao Shoaib wrote:
> From: Rao Shoaib <rao.shoaib@oracle.com>
> 
> 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 <rao.shoaib@oracle.com>

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

Re: [PATCH] af_unix: fix holding spinlock in oob handling

[Shoaib Rao]

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:
>> From: Rao Shoaib <rao.shoaib@oracle.com>
>>
>> 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 <rao.shoaib@oracle.com>
> IIUC issues pointed out by Eric are separate so I removed the spacing
> between the tags and applied, thanks!

[PATCH] af_unix: fix holding spinlock in oob handling

[Rao Shoaib]

From: Rao Shoaib <rao.shoaib@oracle.com>

syzkaller found that OOB code was holding spinlock
while calling a function in which it could sleep.
Also addressed comments from edumazet@google.com.

Reported-by: syzbot+8760ca6c1ee783ac4abd@syzkaller.appspotmail.com
Fixes: 314001f0bf92 ("af_unix: Add OOB support")
Signed-off-by: Rao Shoaib <rao.shoaib@oracle.com>
---
 net/unix/af_unix.c | 47 ++++++++++++++++++++++++++++++++--------------
 1 file changed, 33 insertions(+), 14 deletions(-)

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 00d8b08cdbe1..0f59fed993d8 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -1891,7 +1891,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) {
@@ -1900,11 +1899,19 @@ 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;
 
@@ -2362,19 +2369,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;
+
+	mutex_lock(&u->iolock);
+	unix_state_lock(sk);
 
-	if (sock_flag(sk, SOCK_URGINLINE) || !u->oob_skb)
+	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 +2459,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