Netdev Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] af_unix: fix holding spinlock in oob handling
@ 2021-08-11 22:06 Rao Shoaib
  2021-08-12  7:53 ` Eric Dumazet
  2021-08-13 17:32 ` Jakub Kicinski
  0 siblings, 2 replies; 9+ messages in thread
From: Rao Shoaib @ 2021-08-11 22:06 UTC (permalink / raw)
  To: netdev, kuba, viro, rao.shoaib, edumazet

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] af_unix: fix holding spinlock in oob handling
  2021-08-11 22:06 [PATCH] af_unix: fix holding spinlock in oob handling Rao Shoaib
@ 2021-08-12  7:53 ` Eric Dumazet
  2021-08-12 16:23   ` Shoaib Rao
  2021-08-12 17:37   ` Shoaib Rao
  2021-08-13 17:32 ` Jakub Kicinski
  1 sibling, 2 replies; 9+ messages in thread
From: Eric Dumazet @ 2021-08-12  7:53 UTC (permalink / raw)
  To: Rao Shoaib; +Cc: netdev, Jakub Kicinski, Al Viro

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;

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] af_unix: fix holding spinlock in oob handling
  2021-08-12  7:53 ` Eric Dumazet
@ 2021-08-12 16:23   ` Shoaib Rao
  2021-08-12 17:37   ` Shoaib Rao
  1 sibling, 0 replies; 9+ messages in thread
From: Shoaib Rao @ 2021-08-12 16:23 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: netdev, Jakub Kicinski, Al Viro

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;

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] af_unix: fix holding spinlock in oob handling
  2021-08-12  7:53 ` Eric Dumazet
  2021-08-12 16:23   ` Shoaib Rao
@ 2021-08-12 17:37   ` Shoaib Rao
  2021-08-12 20:33     ` Eric Dumazet
  1 sibling, 1 reply; 9+ messages in thread
From: Shoaib Rao @ 2021-08-12 17:37 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: netdev, Jakub Kicinski, Al Viro


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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] af_unix: fix holding spinlock in oob handling
  2021-08-12 17:37   ` Shoaib Rao
@ 2021-08-12 20:33     ` Eric Dumazet
  2021-08-12 20:38       ` Shoaib Rao
  0 siblings, 1 reply; 9+ messages in thread
From: Eric Dumazet @ 2021-08-12 20:33 UTC (permalink / raw)
  To: Shoaib Rao; +Cc: netdev, Jakub Kicinski, Al Viro

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.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] af_unix: fix holding spinlock in oob handling
  2021-08-12 20:33     ` Eric Dumazet
@ 2021-08-12 20:38       ` Shoaib Rao
  0 siblings, 0 replies; 9+ messages in thread
From: Shoaib Rao @ 2021-08-12 20:38 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: netdev, Jakub Kicinski, Al Viro


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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] af_unix: fix holding spinlock in oob handling
  2021-08-11 22:06 [PATCH] af_unix: fix holding spinlock in oob handling Rao Shoaib
  2021-08-12  7:53 ` Eric Dumazet
@ 2021-08-13 17:32 ` Jakub Kicinski
  2021-08-13 17:59   ` Shoaib Rao
  1 sibling, 1 reply; 9+ messages in thread
From: Jakub Kicinski @ 2021-08-13 17:32 UTC (permalink / raw)
  To: Rao Shoaib; +Cc: netdev, viro, edumazet

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!

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] af_unix: fix holding spinlock in oob handling
  2021-08-13 17:32 ` Jakub Kicinski
@ 2021-08-13 17:59   ` Shoaib Rao
  0 siblings, 0 replies; 9+ messages in thread
From: Shoaib Rao @ 2021-08-13 17:59 UTC (permalink / raw)
  To: Jakub Kicinski; +Cc: netdev, viro, edumazet

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!

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH] af_unix: fix holding spinlock in oob handling
@ 2021-08-13 17:46 Rao Shoaib
  0 siblings, 0 replies; 9+ messages in thread
From: Rao Shoaib @ 2021-08-13 17:46 UTC (permalink / raw)
  To: netdev, kuba, rao.shoaib, viro, edumazet

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-08-13 17:59 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-11 22:06 [PATCH] af_unix: fix holding spinlock in oob handling Rao Shoaib
2021-08-12  7:53 ` Eric Dumazet
2021-08-12 16:23   ` Shoaib Rao
2021-08-12 17:37   ` Shoaib Rao
2021-08-12 20:33     ` Eric Dumazet
2021-08-12 20:38       ` Shoaib Rao
2021-08-13 17:32 ` Jakub Kicinski
2021-08-13 17:59   ` Shoaib Rao
2021-08-13 17:46 Rao Shoaib

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).