[PATCH 1/2] dccp: Unlock sock before calling sk_free()

[PATCH 1/2] dccp: Unlock sock before calling sk_free()

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

The code where sk_clone() came from created a new socket and locked it,
but then, on the error path didn't unlock it.

This problem stayed there for a long while, till b0691c8ee7c2 ("net:
Unlock sock before calling sk_free()") fixed it, but unfortunately the
callers of sk_clone() (now sk_clone_locked()) were not audited and the
one in dccp_create_openreq_child() remained.

Now in the age of the syskaller fuzzer, this was finally uncovered, as
reported by Dmitry:

 ---- 8< ----

I've got the following report while running syzkaller fuzzer on
86292b33d4b7 ("Merge branch 'akpm' (patches from Andrew)")

  [ BUG: held lock freed! ]
  4.10.0+ #234 Not tainted
  -------------------------
  syz-executor6/6898 is freeing memory
  ffff88006286cac0-ffff88006286d3b7, with a lock still held there!
   (slock-AF_INET6){+.-...}, at: [<ffffffff8362c2c9>] spin_lock
  include/linux/spinlock.h:299 [inline]
   (slock-AF_INET6){+.-...}, at: [<ffffffff8362c2c9>]
  sk_clone_lock+0x3d9/0x12c0 net/core/sock.c:1504
  5 locks held by syz-executor6/6898:
   #0:  (sk_lock-AF_INET6){+.+.+.}, at: [<ffffffff839a34b4>] lock_sock
  include/net/sock.h:1460 [inline]
   #0:  (sk_lock-AF_INET6){+.+.+.}, at: [<ffffffff839a34b4>]
  inet_stream_connect+0x44/0xa0 net/ipv4/af_inet.c:681
   #1:  (rcu_read_lock){......}, at: [<ffffffff83bc1c2a>]
  inet6_csk_xmit+0x12a/0x5d0 net/ipv6/inet6_connection_sock.c:126
   #2:  (rcu_read_lock){......}, at: [<ffffffff8369b424>] __skb_unlink
  include/linux/skbuff.h:1767 [inline]
   #2:  (rcu_read_lock){......}, at: [<ffffffff8369b424>] __skb_dequeue
  include/linux/skbuff.h:1783 [inline]
   #2:  (rcu_read_lock){......}, at: [<ffffffff8369b424>]
  process_backlog+0x264/0x730 net/core/dev.c:4835
   #3:  (rcu_read_lock){......}, at: [<ffffffff83aeb5c0>]
  ip6_input_finish+0x0/0x1700 net/ipv6/ip6_input.c:59
   #4:  (slock-AF_INET6){+.-...}, at: [<ffffffff8362c2c9>] spin_lock
  include/linux/spinlock.h:299 [inline]
   #4:  (slock-AF_INET6){+.-...}, at: [<ffffffff8362c2c9>]
  sk_clone_lock+0x3d9/0x12c0 net/core/sock.c:1504

Fix it just like was done by b0691c8ee7c2 ("net: Unlock sock before calling
sk_free()").

Reported-by: Dmitry Vyukov <dvyukov@google.com>
Cc: Cong Wang <xiyou.wangcong@gmail.com>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Gerrit Renker <gerrit@erg.abdn.ac.uk>
Cc: Thomas Gleixner <tglx@linutronix.de>
Link: http://lkml.kernel.org/r/20170301153510.GE15145@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 net/dccp/minisocks.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/dccp/minisocks.c b/net/dccp/minisocks.c
index 53eddf99e4f6..d20d948a98ed 100644
--- a/net/dccp/minisocks.c
+++ b/net/dccp/minisocks.c
@@ -122,6 +122,7 @@ struct sock *dccp_create_openreq_child(const struct sock *sk,
 			/* It is still raw copy of parent, so invalidate
 			 * destructor and make plain sk_free() */
 			newsk->sk_destruct = NULL;
+			bh_unlock_sock(newsk);
 			sk_free(newsk);
 			return NULL;
 		}
-- 
2.9.3

[PATCH 2/2] net: Introduce sk_clone_lock() error path routine

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

When handling problems in cloning a socket with the sk_clone_locked()
function we need to perform several steps that were open coded in it and
its callers, so introduce a routine to avoid this duplication:
sk_free_unlock_clone().

Cc: Cong Wang <xiyou.wangcong@gmail.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Gerrit Renker <gerrit@erg.abdn.ac.uk>
Cc: Thomas Gleixner <tglx@linutronix.de>
Link: http://lkml.kernel.org/n/net-ui6laqkotycunhtmqryl9bfx@git.kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 include/net/sock.h   |  1 +
 net/core/sock.c      | 16 +++++++++++-----
 net/dccp/minisocks.c |  6 +-----
 3 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/include/net/sock.h b/include/net/sock.h
index c4f5e6fca17c..93d1160bcd32 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -1520,6 +1520,7 @@ struct sock *sk_alloc(struct net *net, int family, gfp_t priority,
 void sk_free(struct sock *sk);
 void sk_destruct(struct sock *sk);
 struct sock *sk_clone_lock(const struct sock *sk, const gfp_t priority);
+void sk_free_unlock_clone(struct sock *sk);
 
 struct sk_buff *sock_wmalloc(struct sock *sk, unsigned long size, int force,
 			     gfp_t priority);
diff --git a/net/core/sock.c b/net/core/sock.c
index 4eca27dc5c94..a3d9bb20f65d 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -1540,11 +1540,7 @@ struct sock *sk_clone_lock(const struct sock *sk, const gfp_t priority)
 			is_charged = sk_filter_charge(newsk, filter);
 
 		if (unlikely(!is_charged || xfrm_sk_clone_policy(newsk, sk))) {
-			/* It is still raw copy of parent, so invalidate
-			 * destructor and make plain sk_free() */
-			newsk->sk_destruct = NULL;
-			bh_unlock_sock(newsk);
-			sk_free(newsk);
+			sk_free_unlock_clone(newsk);
 			newsk = NULL;
 			goto out;
 		}
@@ -1593,6 +1589,16 @@ struct sock *sk_clone_lock(const struct sock *sk, const gfp_t priority)
 }
 EXPORT_SYMBOL_GPL(sk_clone_lock);
 
+void sk_free_unlock_clone(struct sock *sk)
+{
+	/* It is still raw copy of parent, so invalidate
+	 * destructor and make plain sk_free() */
+	sk->sk_destruct = NULL;
+	bh_unlock_sock(sk);
+	sk_free(sk);
+}
+EXPORT_SYMBOL_GPL(sk_free_unlock_clone);
+
 void sk_setup_caps(struct sock *sk, struct dst_entry *dst)
 {
 	u32 max_segs = 1;
diff --git a/net/dccp/minisocks.c b/net/dccp/minisocks.c
index d20d948a98ed..e267e6f4c9a5 100644
--- a/net/dccp/minisocks.c
+++ b/net/dccp/minisocks.c
@@ -119,11 +119,7 @@ struct sock *dccp_create_openreq_child(const struct sock *sk,
 		 * Activate features: initialise CCIDs, sequence windows etc.
 		 */
 		if (dccp_feat_activate_values(newsk, &dreq->dreq_featneg)) {
-			/* It is still raw copy of parent, so invalidate
-			 * destructor and make plain sk_free() */
-			newsk->sk_destruct = NULL;
-			bh_unlock_sock(newsk);
-			sk_free(newsk);
+			sk_free_unlock_clone(newsk);
 			return NULL;
 		}
 		dccp_init_xmit_timers(newsk);
-- 
2.9.3

Re: [PATCH 2/2] net: Introduce sk_clone_lock() error path routine

[David Miller]

From: Arnaldo Carvalho de Melo <acme@kernel.org>
Date: Wed,  1 Mar 2017 16:35:08 -0300

> From: Arnaldo Carvalho de Melo <acme@redhat.com>
> 
> When handling problems in cloning a socket with the sk_clone_locked()
> function we need to perform several steps that were open coded in it and
> its callers, so introduce a routine to avoid this duplication:
> sk_free_unlock_clone().
> 
> Cc: Cong Wang <xiyou.wangcong@gmail.com>
> Cc: Dmitry Vyukov <dvyukov@google.com>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Gerrit Renker <gerrit@erg.abdn.ac.uk>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Link: http://lkml.kernel.org/n/net-ui6laqkotycunhtmqryl9bfx@git.kernel.org
> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>

Applied.

Re: [PATCH 2/2] net: Introduce sk_clone_lock() error path routine

[Sergei Shtylyov]

Hello!

On 3/1/2017 10:35 PM, Arnaldo Carvalho de Melo wrote:

> From: Arnaldo Carvalho de Melo <acme@redhat.com>
>
> When handling problems in cloning a socket with the sk_clone_locked()
> function we need to perform several steps that were open coded in it and
> its callers, so introduce a routine to avoid this duplication:
> sk_free_unlock_clone().
>
> Cc: Cong Wang <xiyou.wangcong@gmail.com>
> Cc: Dmitry Vyukov <dvyukov@google.com>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Gerrit Renker <gerrit@erg.abdn.ac.uk>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Link: http://lkml.kernel.org/n/net-ui6laqkotycunhtmqryl9bfx@git.kernel.org
> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
[...]
> diff --git a/net/core/sock.c b/net/core/sock.c
> index 4eca27dc5c94..a3d9bb20f65d 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -1540,11 +1540,7 @@ struct sock *sk_clone_lock(const struct sock *sk, const gfp_t priority)
>  			is_charged = sk_filter_charge(newsk, filter);
>
>  		if (unlikely(!is_charged || xfrm_sk_clone_policy(newsk, sk))) {
> -			/* It is still raw copy of parent, so invalidate
> -			 * destructor and make plain sk_free() */
> -			newsk->sk_destruct = NULL;
> -			bh_unlock_sock(newsk);
> -			sk_free(newsk);
> +			sk_free_unlock_clone(newsk);
>  			newsk = NULL;
>  			goto out;
>  		}
> @@ -1593,6 +1589,16 @@ struct sock *sk_clone_lock(const struct sock *sk, const gfp_t priority)
>  }
>  EXPORT_SYMBOL_GPL(sk_clone_lock);
>
> +void sk_free_unlock_clone(struct sock *sk)
> +{
> +	/* It is still raw copy of parent, so invalidate
> +	 * destructor and make plain sk_free() */

    Could fix the comment style to the preferred one, while at it:

/* bla
  * bla
  */

> +	sk->sk_destruct = NULL;
> +	bh_unlock_sock(sk);
> +	sk_free(sk);
> +}
> +EXPORT_SYMBOL_GPL(sk_free_unlock_clone);
> +
>  void sk_setup_caps(struct sock *sk, struct dst_entry *dst)
>  {
>  	u32 max_segs = 1;
[...]

MBR, Sergei

Re: [PATCH 1/2] dccp: Unlock sock before calling sk_free()

[David Miller]

From: Arnaldo Carvalho de Melo <acme@kernel.org>
Date: Wed,  1 Mar 2017 16:35:07 -0300

> From: Arnaldo Carvalho de Melo <acme@redhat.com>
> 
> The code where sk_clone() came from created a new socket and locked it,
> but then, on the error path didn't unlock it.
> 
> This problem stayed there for a long while, till b0691c8ee7c2 ("net:
> Unlock sock before calling sk_free()") fixed it, but unfortunately the
> callers of sk_clone() (now sk_clone_locked()) were not audited and the
> one in dccp_create_openreq_child() remained.
> 
> Now in the age of the syskaller fuzzer, this was finally uncovered, as
> reported by Dmitry:
 ...
> Fix it just like was done by b0691c8ee7c2 ("net: Unlock sock before calling
> sk_free()").
> 
> Reported-by: Dmitry Vyukov <dvyukov@google.com>
> Cc: Cong Wang <xiyou.wangcong@gmail.com>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Gerrit Renker <gerrit@erg.abdn.ac.uk>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Link: http://lkml.kernel.org/r/20170301153510.GE15145@kernel.org
> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>

Applied and queued up for -stable.