[PATCH net] sctp: hold the transport before using it in sctp_hash_cmp

[PATCH net] sctp: hold the transport before using it in sctp_hash_cmp

[Xin Long]

Now sctp uses the transport without holding it in sctp_hash_cmp,
it can cause a use-after-free panic. As after it get transport from
hashtable, another CPU may free it, then the members it accesses
may be unavailable memory.

This patch is to use sctp_transport_hold, in which it checks the
refcnt first, holds it if it's not 0.

Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/sctp/input.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/net/sctp/input.c b/net/sctp/input.c
index 69444d3..1555fb8 100644
--- a/net/sctp/input.c
+++ b/net/sctp/input.c
@@ -796,27 +796,34 @@ struct sctp_hash_cmp_arg {
 static inline int sctp_hash_cmp(struct rhashtable_compare_arg *arg,
 				const void *ptr)
 {
+	struct sctp_transport *t = (struct sctp_transport *)ptr;
 	const struct sctp_hash_cmp_arg *x = arg->key;
-	const struct sctp_transport *t = ptr;
-	struct sctp_association *asoc = t->asoc;
-	const struct net *net = x->net;
+	struct sctp_association *asoc;
+	int err = 1;
 
 	if (!sctp_cmp_addr_exact(&t->ipaddr, x->paddr))
-		return 1;
-	if (!net_eq(sock_net(asoc->base.sk), net))
-		return 1;
+		return err;
+	if (!sctp_transport_hold(t))
+		return err;
+
+	asoc = t->asoc;
+	if (!net_eq(sock_net(asoc->base.sk), x->net))
+		goto out;
 	if (x->ep) {
 		if (x->ep != asoc->ep)
-			return 1;
+			goto out;
 	} else {
 		if (x->laddr->v4.sin_port != htons(asoc->base.bind_addr.port))
-			return 1;
+			goto out;
 		if (!sctp_bind_addr_match(&asoc->base.bind_addr,
 					  x->laddr, sctp_sk(asoc->base.sk)))
-			return 1;
+			goto out;
 	}
 
-	return 0;
+	err = 0;
+out:
+	sctp_transport_put(t);
+	return err;
 }
 
 static inline u32 sctp_hash_obj(const void *data, u32 len, u32 seed)
-- 
2.1.0

[PATCH net] sctp: hold the transport before using it in sctp_hash_cmp

[Xin Long]

Now sctp uses the transport without holding it in sctp_hash_cmp,
it can cause a use-after-free panic. As after it get transport from
hashtable, another CPU may free it, then the members it accesses
may be unavailable memory.

This patch is to use sctp_transport_hold, in which it checks the
refcnt first, holds it if it's not 0.

Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/sctp/input.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/net/sctp/input.c b/net/sctp/input.c
index 69444d3..1555fb8 100644
--- a/net/sctp/input.c
+++ b/net/sctp/input.c
@@ -796,27 +796,34 @@ struct sctp_hash_cmp_arg {
 static inline int sctp_hash_cmp(struct rhashtable_compare_arg *arg,
 				const void *ptr)
 {
+	struct sctp_transport *t = (struct sctp_transport *)ptr;
 	const struct sctp_hash_cmp_arg *x = arg->key;
-	const struct sctp_transport *t = ptr;
-	struct sctp_association *asoc = t->asoc;
-	const struct net *net = x->net;
+	struct sctp_association *asoc;
+	int err = 1;
 
 	if (!sctp_cmp_addr_exact(&t->ipaddr, x->paddr))
-		return 1;
-	if (!net_eq(sock_net(asoc->base.sk), net))
-		return 1;
+		return err;
+	if (!sctp_transport_hold(t))
+		return err;
+
+	asoc = t->asoc;
+	if (!net_eq(sock_net(asoc->base.sk), x->net))
+		goto out;
 	if (x->ep) {
 		if (x->ep != asoc->ep)
-			return 1;
+			goto out;
 	} else {
 		if (x->laddr->v4.sin_port != htons(asoc->base.bind_addr.port))
-			return 1;
+			goto out;
 		if (!sctp_bind_addr_match(&asoc->base.bind_addr,
 					  x->laddr, sctp_sk(asoc->base.sk)))
-			return 1;
+			goto out;
 	}
 
-	return 0;
+	err = 0;
+out:
+	sctp_transport_put(t);
+	return err;
 }
 
 static inline u32 sctp_hash_obj(const void *data, u32 len, u32 seed)
-- 
2.1.0

RE: [PATCH net] sctp: hold the transport before using it in sctp_hash_cmp

[David Laight]

From: Xin Long
> Sent: 08 September 2016 10:49
> Now sctp uses the transport without holding it in sctp_hash_cmp,
> it can cause a use-after-free panic. As after it get transport from
> hashtable, another CPU may free it, then the members it accesses
> may be unavailable memory.

How old is this bug?
Is it in any release kernels?

	David

RE: [PATCH net] sctp: hold the transport before using it in sctp_hash_cmp

[David Laight]

From: Xin Long
> Sent: 08 September 2016 10:49
> Now sctp uses the transport without holding it in sctp_hash_cmp,
> it can cause a use-after-free panic. As after it get transport from
> hashtable, another CPU may free it, then the members it accesses
> may be unavailable memory.

How old is this bug?
Is it in any release kernels?

	David

Re: [PATCH net] sctp: hold the transport before using it in sctp_hash_cmp

[Xin Long]

>> Now sctp uses the transport without holding it in sctp_hash_cmp,
>> it can cause a use-after-free panic. As after it get transport from
>> hashtable, another CPU may free it, then the members it accesses
>> may be unavailable memory.
>
> How old is this bug?
I think since:
$ git describe 33c1529
v4.4-rc7-1158-g33c1529

> Is it in any release kernels?
>
yes

it was found in s390x, we could not reproduce it in x86_64 by now.
Maybe it needs low configuration cpu to reproduce.

Re: [PATCH net] sctp: hold the transport before using it in sctp_hash_cmp

[Xin Long]

>> Now sctp uses the transport without holding it in sctp_hash_cmp,
>> it can cause a use-after-free panic. As after it get transport from
>> hashtable, another CPU may free it, then the members it accesses
>> may be unavailable memory.
>
> How old is this bug?
I think since:
$ git describe 33c1529
v4.4-rc7-1158-g33c1529

> Is it in any release kernels?
>
yes

it was found in s390x, we could not reproduce it in x86_64 by now.
Maybe it needs low configuration cpu to reproduce.

Re: [PATCH net] sctp: hold the transport before using it in sctp_hash_cmp

[Neil Horman]

On Thu, Sep 08, 2016 at 05:49:04PM +0800, Xin Long wrote:
> Now sctp uses the transport without holding it in sctp_hash_cmp,
> it can cause a use-after-free panic. As after it get transport from
> hashtable, another CPU may free it, then the members it accesses
> may be unavailable memory.
> 
> This patch is to use sctp_transport_hold, in which it checks the
> refcnt first, holds it if it's not 0.
> 
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
>  net/sctp/input.c | 27 +++++++++++++++++----------
>  1 file changed, 17 insertions(+), 10 deletions(-)
> 
> diff --git a/net/sctp/input.c b/net/sctp/input.c
> index 69444d3..1555fb8 100644
> --- a/net/sctp/input.c
> +++ b/net/sctp/input.c
> @@ -796,27 +796,34 @@ struct sctp_hash_cmp_arg {
>  static inline int sctp_hash_cmp(struct rhashtable_compare_arg *arg,
>  				const void *ptr)
>  {
> +	struct sctp_transport *t = (struct sctp_transport *)ptr;
>  	const struct sctp_hash_cmp_arg *x = arg->key;
> -	const struct sctp_transport *t = ptr;
> -	struct sctp_association *asoc = t->asoc;
> -	const struct net *net = x->net;
> +	struct sctp_association *asoc;
> +	int err = 1;
>  
>  	if (!sctp_cmp_addr_exact(&t->ipaddr, x->paddr))
> -		return 1;
> -	if (!net_eq(sock_net(asoc->base.sk), net))
> -		return 1;
> +		return err;
> +	if (!sctp_transport_hold(t))
> +		return err;
> +
> +	asoc = t->asoc;
> +	if (!net_eq(sock_net(asoc->base.sk), x->net))
> +		goto out;
>  	if (x->ep) {
>  		if (x->ep != asoc->ep)
> -			return 1;
> +			goto out;
>  	} else {
>  		if (x->laddr->v4.sin_port != htons(asoc->base.bind_addr.port))
> -			return 1;
> +			goto out;
>  		if (!sctp_bind_addr_match(&asoc->base.bind_addr,
>  					  x->laddr, sctp_sk(asoc->base.sk)))
> -			return 1;
> +			goto out;
>  	}
>  
> -	return 0;
> +	err = 0;
> +out:
> +	sctp_transport_put(t);
> +	return err;
>  }
>  
>  static inline u32 sctp_hash_obj(const void *data, u32 len, u32 seed)
> -- 
> 2.1.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
What path does this occur in?  __sctp_lookup_association takes a hold on the
asoc, and that function is protected by the rcu_read_lock, so I'm not sure how
you get into a situation in which the asoc can be deleted while the lookup is
occuring.

Neil

Re: [PATCH net] sctp: hold the transport before using it in sctp_hash_cmp

[Neil Horman]

On Thu, Sep 08, 2016 at 05:49:04PM +0800, Xin Long wrote:
> Now sctp uses the transport without holding it in sctp_hash_cmp,
> it can cause a use-after-free panic. As after it get transport from
> hashtable, another CPU may free it, then the members it accesses
> may be unavailable memory.
> 
> This patch is to use sctp_transport_hold, in which it checks the
> refcnt first, holds it if it's not 0.
> 
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
>  net/sctp/input.c | 27 +++++++++++++++++----------
>  1 file changed, 17 insertions(+), 10 deletions(-)
> 
> diff --git a/net/sctp/input.c b/net/sctp/input.c
> index 69444d3..1555fb8 100644
> --- a/net/sctp/input.c
> +++ b/net/sctp/input.c
> @@ -796,27 +796,34 @@ struct sctp_hash_cmp_arg {
>  static inline int sctp_hash_cmp(struct rhashtable_compare_arg *arg,
>  				const void *ptr)
>  {
> +	struct sctp_transport *t = (struct sctp_transport *)ptr;
>  	const struct sctp_hash_cmp_arg *x = arg->key;
> -	const struct sctp_transport *t = ptr;
> -	struct sctp_association *asoc = t->asoc;
> -	const struct net *net = x->net;
> +	struct sctp_association *asoc;
> +	int err = 1;
>  
>  	if (!sctp_cmp_addr_exact(&t->ipaddr, x->paddr))
> -		return 1;
> -	if (!net_eq(sock_net(asoc->base.sk), net))
> -		return 1;
> +		return err;
> +	if (!sctp_transport_hold(t))
> +		return err;
> +
> +	asoc = t->asoc;
> +	if (!net_eq(sock_net(asoc->base.sk), x->net))
> +		goto out;
>  	if (x->ep) {
>  		if (x->ep != asoc->ep)
> -			return 1;
> +			goto out;
>  	} else {
>  		if (x->laddr->v4.sin_port != htons(asoc->base.bind_addr.port))
> -			return 1;
> +			goto out;
>  		if (!sctp_bind_addr_match(&asoc->base.bind_addr,
>  					  x->laddr, sctp_sk(asoc->base.sk)))
> -			return 1;
> +			goto out;
>  	}
>  
> -	return 0;
> +	err = 0;
> +out:
> +	sctp_transport_put(t);
> +	return err;
>  }
>  
>  static inline u32 sctp_hash_obj(const void *data, u32 len, u32 seed)
> -- 
> 2.1.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
What path does this occur in?  __sctp_lookup_association takes a hold on the
asoc, and that function is protected by the rcu_read_lock, so I'm not sure how
you get into a situation in which the asoc can be deleted while the lookup is
occuring.

Neil

Re: [PATCH net] sctp: hold the transport before using it in sctp_hash_cmp

[Xin Long]

>>
> What path does this occur in?  __sctp_lookup_association takes a hold on the
> asoc, and that function is protected by the rcu_read_lock, so I'm not sure how
> you get into a situation in which the asoc can be deleted while the lookup is
> occuring.
>
yes  that function is protected by the rcu_read_lock, it can only
ensure sctp_transport_destroy_rcu will NOT be called. But,
sctp_transport_destroy can be called by sctp_assocation_free.
in which, sctp free and unhash  all the transport. when transport's
defcnt become 0, sctp will call sctp_transport_destroy, in which,
asoc can be destroyed because of:
if (transport->asoc)
        sctp_association_put(transport->asoc);

Re: [PATCH net] sctp: hold the transport before using it in sctp_hash_cmp

[Xin Long]

>>
> What path does this occur in?  __sctp_lookup_association takes a hold on the
> asoc, and that function is protected by the rcu_read_lock, so I'm not sure how
> you get into a situation in which the asoc can be deleted while the lookup is
> occuring.
>
yes  that function is protected by the rcu_read_lock, it can only
ensure sctp_transport_destroy_rcu will NOT be called. But,
sctp_transport_destroy can be called by sctp_assocation_free.
in which, sctp free and unhash  all the transport. when transport's
defcnt become 0, sctp will call sctp_transport_destroy, in which,
asoc can be destroyed because of:
if (transport->asoc)
        sctp_association_put(transport->asoc);

Re: [PATCH net] sctp: hold the transport before using it in sctp_hash_cmp

[David Miller]

From: Xin Long <lucien.xin@gmail.com>
Date: Thu,  8 Sep 2016 17:49:04 +0800

> Now sctp uses the transport without holding it in sctp_hash_cmp,
> it can cause a use-after-free panic. As after it get transport from
> hashtable, another CPU may free it, then the members it accesses
> may be unavailable memory.
> 
> This patch is to use sctp_transport_hold, in which it checks the
> refcnt first, holds it if it's not 0.
> 
> Signed-off-by: Xin Long <lucien.xin@gmail.com>

Please add more detail to the commit message and add a proper
"Fixes: " tag right before your signoff.

Thanks.

Re: [PATCH net] sctp: hold the transport before using it in sctp_hash_cmp

[David Miller]

From: Xin Long <lucien.xin@gmail.com>
Date: Thu,  8 Sep 2016 17:49:04 +0800

> Now sctp uses the transport without holding it in sctp_hash_cmp,
> it can cause a use-after-free panic. As after it get transport from
> hashtable, another CPU may free it, then the members it accesses
> may be unavailable memory.
> 
> This patch is to use sctp_transport_hold, in which it checks the
> refcnt first, holds it if it's not 0.
> 
> Signed-off-by: Xin Long <lucien.xin@gmail.com>

Please add more detail to the commit message and add a proper
"Fixes: " tag right before your signoff.

Thanks.

Re: [PATCH net] sctp: hold the transport before using it in sctp_hash_cmp

[Xin Long]

>
> Please add more detail to the commit message and add a proper
> "Fixes: " tag right before your signoff.
OK, will repost v2

Re: [PATCH net] sctp: hold the transport before using it in sctp_hash_cmp

[Xin Long]

>
> Please add more detail to the commit message and add a proper
> "Fixes: " tag right before your signoff.
OK, will repost v2