diff mbox series

[X] crypto: af_alg - fix use-after-free in af_alg_accept() due to bh_lock_sock()

Message ID 20200630231023.1576947-1-mfo@canonical.com
State New
Headers show
Series [X] crypto: af_alg - fix use-after-free in af_alg_accept() due to bh_lock_sock() | expand

Commit Message

Mauricio Faria de Oliveira June 30, 2020, 11:10 p.m. UTC 
From: Herbert Xu <herbert@gondor.apana.org.au>

BugLink: https://bugs.launchpad.net/bugs/1884766

The locking in af_alg_release_parent is broken as the BH socket
lock can only be taken if there is a code-path to handle the case
where the lock is owned by process-context.  Instead of adding
such handling, we can fix this by changing the ref counts to
atomic_t.

This patch also modifies the main refcnt to include both normal
and nokey sockets.  This way we don't have to fudge the nokey
ref count when a socket changes from nokey to normal.

Credits go to Mauricio Faria de Oliveira who diagnosed this bug
and sent a patch for it:

https://lore.kernel.org/linux-crypto/20200605161657.535043-1-mfo@canonical.com/

Reported-by: Brian Moyles <bmoyles@netflix.com>
Reported-by: Mauricio Faria de Oliveira <mfo@canonical.com>
Fixes: 37f96694cf73 ("crypto: af_alg - Use bh_lock_sock in...")
Cc: <stable@vger.kernel.org>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
(backported from commit 34c86f4c4a7be3b3e35aa48bd18299d4c756064d)
[mfo: backport: refresh context lines in af_alg.c hunk 4,
 algif_aead.c hunk 2, algif_skcipher.c hunk 2, algif_hash.c hunk 2]
Signed-off-by: Mauricio Faria de Oliveira <mfo@canonical.com>
---
 crypto/af_alg.c         | 26 +++++++++++---------------
 crypto/algif_aead.c     |  9 +++------
 crypto/algif_hash.c     |  9 +++------
 crypto/algif_skcipher.c |  9 +++------
 include/crypto/if_alg.h |  4 ++--
 5 files changed, 22 insertions(+), 35 deletions(-)

Comments

Stefan Bader July 1, 2020, 2:11 p.m. UTC | #1 
On 01.07.20 01:10, Mauricio Faria de Oliveira wrote:
> From: Herbert Xu <herbert@gondor.apana.org.au>
> 
> BugLink: https://bugs.launchpad.net/bugs/1884766
> 
> The locking in af_alg_release_parent is broken as the BH socket
> lock can only be taken if there is a code-path to handle the case
> where the lock is owned by process-context.  Instead of adding
> such handling, we can fix this by changing the ref counts to
> atomic_t.
> 
> This patch also modifies the main refcnt to include both normal
> and nokey sockets.  This way we don't have to fudge the nokey
> ref count when a socket changes from nokey to normal.
> 
> Credits go to Mauricio Faria de Oliveira who diagnosed this bug
> and sent a patch for it:
> 
> https://lore.kernel.org/linux-crypto/20200605161657.535043-1-mfo@canonical.com/
> 
> Reported-by: Brian Moyles <bmoyles@netflix.com>
> Reported-by: Mauricio Faria de Oliveira <mfo@canonical.com>
> Fixes: 37f96694cf73 ("crypto: af_alg - Use bh_lock_sock in...")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> (backported from commit 34c86f4c4a7be3b3e35aa48bd18299d4c756064d)
> [mfo: backport: refresh context lines in af_alg.c hunk 4,
>  algif_aead.c hunk 2, algif_skcipher.c hunk 2, algif_hash.c hunk 2]
> Signed-off-by: Mauricio Faria de Oliveira <mfo@canonical.com>
Acked-by: Stefan Bader <stefan.bader@canonical.com>
> ---
>  crypto/af_alg.c         | 26 +++++++++++---------------
>  crypto/algif_aead.c     |  9 +++------
>  crypto/algif_hash.c     |  9 +++------
>  crypto/algif_skcipher.c |  9 +++------
>  include/crypto/if_alg.h |  4 ++--
>  5 files changed, 22 insertions(+), 35 deletions(-)
> 
> diff --git a/crypto/af_alg.c b/crypto/af_alg.c
> index cf3975ee4fd8..c48ddeb6c328 100644
> --- a/crypto/af_alg.c
> +++ b/crypto/af_alg.c
> @@ -130,21 +130,15 @@ EXPORT_SYMBOL_GPL(af_alg_release);
>  void af_alg_release_parent(struct sock *sk)
>  {
>  	struct alg_sock *ask = alg_sk(sk);
> -	unsigned int nokey = ask->nokey_refcnt;
> -	bool last = nokey && !ask->refcnt;
> +	unsigned int nokey = atomic_read(&ask->nokey_refcnt);
>  
>  	sk = ask->parent;
>  	ask = alg_sk(sk);
>  
> -	local_bh_disable();
> -	bh_lock_sock(sk);
> -	ask->nokey_refcnt -= nokey;
> -	if (!last)
> -		last = !--ask->refcnt;
> -	bh_unlock_sock(sk);
> -	local_bh_enable();
> +	if (nokey)
> +		atomic_dec(&ask->nokey_refcnt);
>  
> -	if (last)
> +	if (atomic_dec_and_test(&ask->refcnt))
>  		sock_put(sk);
>  }
>  EXPORT_SYMBOL_GPL(af_alg_release_parent);
> @@ -189,7 +183,7 @@ static int alg_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
>  
>  	err = -EBUSY;
>  	lock_sock(sk);
> -	if (ask->refcnt | ask->nokey_refcnt)
> +	if (atomic_read(&ask->refcnt))
>  		goto unlock;
>  
>  	swap(ask->type, type);
> @@ -238,7 +232,7 @@ static int alg_setsockopt(struct socket *sock, int level, int optname,
>  	int err = -EBUSY;
>  
>  	lock_sock(sk);
> -	if (ask->refcnt)
> +	if (atomic_read(&ask->refcnt) != atomic_read(&ask->nokey_refcnt))
>  		goto unlock;
>  
>  	type = ask->type;
> @@ -305,12 +299,14 @@ int af_alg_accept(struct sock *sk, struct socket *newsock)
>  
>  	sk2->sk_family = PF_ALG;
>  
> -	if (nokey || !ask->refcnt++)
> +	if (atomic_inc_return_relaxed(&ask->refcnt) == 1)
>  		sock_hold(sk);
> -	ask->nokey_refcnt += nokey;
> +	if (nokey) {
> +		atomic_inc(&ask->nokey_refcnt);
> +		atomic_set(&alg_sk(sk2)->nokey_refcnt, 1);
> +	}
>  	alg_sk(sk2)->parent = sk;
>  	alg_sk(sk2)->type = type;
> -	alg_sk(sk2)->nokey_refcnt = nokey;
>  
>  	newsock->ops = type->ops;
>  	newsock->state = SS_CONNECTED;
> diff --git a/crypto/algif_aead.c b/crypto/algif_aead.c
> index faea9d728fd2..c50175ad5485 100644
> --- a/crypto/algif_aead.c
> +++ b/crypto/algif_aead.c
> @@ -528,7 +528,7 @@ static int aead_check_key(struct socket *sock)
>  	struct alg_sock *ask = alg_sk(sk);
>  
>  	lock_sock(sk);
> -	if (ask->refcnt)
> +	if (!atomic_read(&ask->nokey_refcnt))
>  		goto unlock_child;
>  
>  	psk = ask->parent;
> @@ -540,11 +540,8 @@ static int aead_check_key(struct socket *sock)
>  	if (!tfm->has_key)
>  		goto unlock;
>  
> -	if (!pask->refcnt++)
> -		sock_hold(psk);
> -
> -	ask->refcnt = 1;
> -	sock_put(psk);
> +	atomic_dec(&pask->nokey_refcnt);
> +	atomic_set(&ask->nokey_refcnt, 0);
>  
>  	err = 0;
>  
> diff --git a/crypto/algif_hash.c b/crypto/algif_hash.c
> index 8d8b3eeba725..fd23261f1d16 100644
> --- a/crypto/algif_hash.c
> +++ b/crypto/algif_hash.c
> @@ -252,7 +252,7 @@ static int hash_check_key(struct socket *sock)
>  	struct alg_sock *ask = alg_sk(sk);
>  
>  	lock_sock(sk);
> -	if (ask->refcnt)
> +	if (!atomic_read(&ask->nokey_refcnt))
>  		goto unlock_child;
>  
>  	psk = ask->parent;
> @@ -264,11 +264,8 @@ static int hash_check_key(struct socket *sock)
>  	if (!tfm->has_key)
>  		goto unlock;
>  
> -	if (!pask->refcnt++)
> -		sock_hold(psk);
> -
> -	ask->refcnt = 1;
> -	sock_put(psk);
> +	atomic_dec(&pask->nokey_refcnt);
> +	atomic_set(&ask->nokey_refcnt, 0);
>  
>  	err = 0;
>  
> diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
> index 9bd4691cc5c5..0e200bf5aa82 100644
> --- a/crypto/algif_skcipher.c
> +++ b/crypto/algif_skcipher.c
> @@ -774,7 +774,7 @@ static int skcipher_check_key(struct socket *sock)
>  	struct alg_sock *ask = alg_sk(sk);
>  
>  	lock_sock(sk);
> -	if (ask->refcnt)
> +	if (!atomic_read(&ask->nokey_refcnt))
>  		goto unlock_child;
>  
>  	psk = ask->parent;
> @@ -786,11 +786,8 @@ static int skcipher_check_key(struct socket *sock)
>  	if (!tfm->has_key)
>  		goto unlock;
>  
> -	if (!pask->refcnt++)
> -		sock_hold(psk);
> -
> -	ask->refcnt = 1;
> -	sock_put(psk);
> +	atomic_dec(&pask->nokey_refcnt);
> +	atomic_set(&ask->nokey_refcnt, 0);
>  
>  	err = 0;
>  
> diff --git a/include/crypto/if_alg.h b/include/crypto/if_alg.h
> index a2bfd7843f18..4bb6b98782e9 100644
> --- a/include/crypto/if_alg.h
> +++ b/include/crypto/if_alg.h
> @@ -30,8 +30,8 @@ struct alg_sock {
>  
>  	struct sock *parent;
>  
> -	unsigned int refcnt;
> -	unsigned int nokey_refcnt;
> +	atomic_t refcnt;
> +	atomic_t nokey_refcnt;
>  
>  	const struct af_alg_type *type;
>  	void *private;
>
Andrea Righi July 2, 2020, 7:16 a.m. UTC | #2 
On Tue, Jun 30, 2020 at 08:10:23PM -0300, Mauricio Faria de Oliveira wrote:
> From: Herbert Xu <herbert@gondor.apana.org.au>
> 
> BugLink: https://bugs.launchpad.net/bugs/1884766
> 
> The locking in af_alg_release_parent is broken as the BH socket
> lock can only be taken if there is a code-path to handle the case
> where the lock is owned by process-context.  Instead of adding
> such handling, we can fix this by changing the ref counts to
> atomic_t.
> 
> This patch also modifies the main refcnt to include both normal
> and nokey sockets.  This way we don't have to fudge the nokey
> ref count when a socket changes from nokey to normal.
> 
> Credits go to Mauricio Faria de Oliveira who diagnosed this bug
> and sent a patch for it:
> 
> https://lore.kernel.org/linux-crypto/20200605161657.535043-1-mfo@canonical.com/
> 
> Reported-by: Brian Moyles <bmoyles@netflix.com>
> Reported-by: Mauricio Faria de Oliveira <mfo@canonical.com>
> Fixes: 37f96694cf73 ("crypto: af_alg - Use bh_lock_sock in...")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> (backported from commit 34c86f4c4a7be3b3e35aa48bd18299d4c756064d)
> [mfo: backport: refresh context lines in af_alg.c hunk 4,
>  algif_aead.c hunk 2, algif_skcipher.c hunk 2, algif_hash.c hunk 2]
> Signed-off-by: Mauricio Faria de Oliveira <mfo@canonical.com>
> ---

Looks good to me.

Acked-by: Andrea Righi <andrea.righi@canonical.com>

>  crypto/af_alg.c         | 26 +++++++++++---------------
>  crypto/algif_aead.c     |  9 +++------
>  crypto/algif_hash.c     |  9 +++------
>  crypto/algif_skcipher.c |  9 +++------
>  include/crypto/if_alg.h |  4 ++--
>  5 files changed, 22 insertions(+), 35 deletions(-)
> 
> diff --git a/crypto/af_alg.c b/crypto/af_alg.c
> index cf3975ee4fd8..c48ddeb6c328 100644
> --- a/crypto/af_alg.c
> +++ b/crypto/af_alg.c
> @@ -130,21 +130,15 @@ EXPORT_SYMBOL_GPL(af_alg_release);
>  void af_alg_release_parent(struct sock *sk)
>  {
>  	struct alg_sock *ask = alg_sk(sk);
> -	unsigned int nokey = ask->nokey_refcnt;
> -	bool last = nokey && !ask->refcnt;
> +	unsigned int nokey = atomic_read(&ask->nokey_refcnt);
>  
>  	sk = ask->parent;
>  	ask = alg_sk(sk);
>  
> -	local_bh_disable();
> -	bh_lock_sock(sk);
> -	ask->nokey_refcnt -= nokey;
> -	if (!last)
> -		last = !--ask->refcnt;
> -	bh_unlock_sock(sk);
> -	local_bh_enable();
> +	if (nokey)
> +		atomic_dec(&ask->nokey_refcnt);
>  
> -	if (last)
> +	if (atomic_dec_and_test(&ask->refcnt))
>  		sock_put(sk);
>  }
>  EXPORT_SYMBOL_GPL(af_alg_release_parent);
> @@ -189,7 +183,7 @@ static int alg_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
>  
>  	err = -EBUSY;
>  	lock_sock(sk);
> -	if (ask->refcnt | ask->nokey_refcnt)
> +	if (atomic_read(&ask->refcnt))
>  		goto unlock;
>  
>  	swap(ask->type, type);
> @@ -238,7 +232,7 @@ static int alg_setsockopt(struct socket *sock, int level, int optname,
>  	int err = -EBUSY;
>  
>  	lock_sock(sk);
> -	if (ask->refcnt)
> +	if (atomic_read(&ask->refcnt) != atomic_read(&ask->nokey_refcnt))
>  		goto unlock;
>  
>  	type = ask->type;
> @@ -305,12 +299,14 @@ int af_alg_accept(struct sock *sk, struct socket *newsock)
>  
>  	sk2->sk_family = PF_ALG;
>  
> -	if (nokey || !ask->refcnt++)
> +	if (atomic_inc_return_relaxed(&ask->refcnt) == 1)
>  		sock_hold(sk);
> -	ask->nokey_refcnt += nokey;
> +	if (nokey) {
> +		atomic_inc(&ask->nokey_refcnt);
> +		atomic_set(&alg_sk(sk2)->nokey_refcnt, 1);
> +	}
>  	alg_sk(sk2)->parent = sk;
>  	alg_sk(sk2)->type = type;
> -	alg_sk(sk2)->nokey_refcnt = nokey;
>  
>  	newsock->ops = type->ops;
>  	newsock->state = SS_CONNECTED;
> diff --git a/crypto/algif_aead.c b/crypto/algif_aead.c
> index faea9d728fd2..c50175ad5485 100644
> --- a/crypto/algif_aead.c
> +++ b/crypto/algif_aead.c
> @@ -528,7 +528,7 @@ static int aead_check_key(struct socket *sock)
>  	struct alg_sock *ask = alg_sk(sk);
>  
>  	lock_sock(sk);
> -	if (ask->refcnt)
> +	if (!atomic_read(&ask->nokey_refcnt))
>  		goto unlock_child;
>  
>  	psk = ask->parent;
> @@ -540,11 +540,8 @@ static int aead_check_key(struct socket *sock)
>  	if (!tfm->has_key)
>  		goto unlock;
>  
> -	if (!pask->refcnt++)
> -		sock_hold(psk);
> -
> -	ask->refcnt = 1;
> -	sock_put(psk);
> +	atomic_dec(&pask->nokey_refcnt);
> +	atomic_set(&ask->nokey_refcnt, 0);
>  
>  	err = 0;
>  
> diff --git a/crypto/algif_hash.c b/crypto/algif_hash.c
> index 8d8b3eeba725..fd23261f1d16 100644
> --- a/crypto/algif_hash.c
> +++ b/crypto/algif_hash.c
> @@ -252,7 +252,7 @@ static int hash_check_key(struct socket *sock)
>  	struct alg_sock *ask = alg_sk(sk);
>  
>  	lock_sock(sk);
> -	if (ask->refcnt)
> +	if (!atomic_read(&ask->nokey_refcnt))
>  		goto unlock_child;
>  
>  	psk = ask->parent;
> @@ -264,11 +264,8 @@ static int hash_check_key(struct socket *sock)
>  	if (!tfm->has_key)
>  		goto unlock;
>  
> -	if (!pask->refcnt++)
> -		sock_hold(psk);
> -
> -	ask->refcnt = 1;
> -	sock_put(psk);
> +	atomic_dec(&pask->nokey_refcnt);
> +	atomic_set(&ask->nokey_refcnt, 0);
>  
>  	err = 0;
>  
> diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
> index 9bd4691cc5c5..0e200bf5aa82 100644
> --- a/crypto/algif_skcipher.c
> +++ b/crypto/algif_skcipher.c
> @@ -774,7 +774,7 @@ static int skcipher_check_key(struct socket *sock)
>  	struct alg_sock *ask = alg_sk(sk);
>  
>  	lock_sock(sk);
> -	if (ask->refcnt)
> +	if (!atomic_read(&ask->nokey_refcnt))
>  		goto unlock_child;
>  
>  	psk = ask->parent;
> @@ -786,11 +786,8 @@ static int skcipher_check_key(struct socket *sock)
>  	if (!tfm->has_key)
>  		goto unlock;
>  
> -	if (!pask->refcnt++)
> -		sock_hold(psk);
> -
> -	ask->refcnt = 1;
> -	sock_put(psk);
> +	atomic_dec(&pask->nokey_refcnt);
> +	atomic_set(&ask->nokey_refcnt, 0);
>  
>  	err = 0;
>  
> diff --git a/include/crypto/if_alg.h b/include/crypto/if_alg.h
> index a2bfd7843f18..4bb6b98782e9 100644
> --- a/include/crypto/if_alg.h
> +++ b/include/crypto/if_alg.h
> @@ -30,8 +30,8 @@ struct alg_sock {
>  
>  	struct sock *parent;
>  
> -	unsigned int refcnt;
> -	unsigned int nokey_refcnt;
> +	atomic_t refcnt;
> +	atomic_t nokey_refcnt;
>  
>  	const struct af_alg_type *type;
>  	void *private;
> -- 
> 2.25.1
> 
> 
> -- 
> kernel-team mailing list
> kernel-team@lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team
Kelsey Skunberg July 20, 2020, 4:45 p.m. UTC | #3 
This was just applied through an upstream patch set. You can find it in
Xenial/master-next. Thank you! 

-Kelsey

On 2020-06-30 20:10:23 , Mauricio Faria de Oliveira wrote:
> From: Herbert Xu <herbert@gondor.apana.org.au>
> 
> BugLink: https://bugs.launchpad.net/bugs/1884766
> 
> The locking in af_alg_release_parent is broken as the BH socket
> lock can only be taken if there is a code-path to handle the case
> where the lock is owned by process-context.  Instead of adding
> such handling, we can fix this by changing the ref counts to
> atomic_t.
> 
> This patch also modifies the main refcnt to include both normal
> and nokey sockets.  This way we don't have to fudge the nokey
> ref count when a socket changes from nokey to normal.
> 
> Credits go to Mauricio Faria de Oliveira who diagnosed this bug
> and sent a patch for it:
> 
> https://lore.kernel.org/linux-crypto/20200605161657.535043-1-mfo@canonical.com/
> 
> Reported-by: Brian Moyles <bmoyles@netflix.com>
> Reported-by: Mauricio Faria de Oliveira <mfo@canonical.com>
> Fixes: 37f96694cf73 ("crypto: af_alg - Use bh_lock_sock in...")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> (backported from commit 34c86f4c4a7be3b3e35aa48bd18299d4c756064d)
> [mfo: backport: refresh context lines in af_alg.c hunk 4,
>  algif_aead.c hunk 2, algif_skcipher.c hunk 2, algif_hash.c hunk 2]
> Signed-off-by: Mauricio Faria de Oliveira <mfo@canonical.com>
> ---
>  crypto/af_alg.c         | 26 +++++++++++---------------
>  crypto/algif_aead.c     |  9 +++------
>  crypto/algif_hash.c     |  9 +++------
>  crypto/algif_skcipher.c |  9 +++------
>  include/crypto/if_alg.h |  4 ++--
>  5 files changed, 22 insertions(+), 35 deletions(-)
> 
> diff --git a/crypto/af_alg.c b/crypto/af_alg.c
> index cf3975ee4fd8..c48ddeb6c328 100644
> --- a/crypto/af_alg.c
> +++ b/crypto/af_alg.c
> @@ -130,21 +130,15 @@ EXPORT_SYMBOL_GPL(af_alg_release);
>  void af_alg_release_parent(struct sock *sk)
>  {
>  	struct alg_sock *ask = alg_sk(sk);
> -	unsigned int nokey = ask->nokey_refcnt;
> -	bool last = nokey && !ask->refcnt;
> +	unsigned int nokey = atomic_read(&ask->nokey_refcnt);
>  
>  	sk = ask->parent;
>  	ask = alg_sk(sk);
>  
> -	local_bh_disable();
> -	bh_lock_sock(sk);
> -	ask->nokey_refcnt -= nokey;
> -	if (!last)
> -		last = !--ask->refcnt;
> -	bh_unlock_sock(sk);
> -	local_bh_enable();
> +	if (nokey)
> +		atomic_dec(&ask->nokey_refcnt);
>  
> -	if (last)
> +	if (atomic_dec_and_test(&ask->refcnt))
>  		sock_put(sk);
>  }
>  EXPORT_SYMBOL_GPL(af_alg_release_parent);
> @@ -189,7 +183,7 @@ static int alg_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
>  
>  	err = -EBUSY;
>  	lock_sock(sk);
> -	if (ask->refcnt | ask->nokey_refcnt)
> +	if (atomic_read(&ask->refcnt))
>  		goto unlock;
>  
>  	swap(ask->type, type);
> @@ -238,7 +232,7 @@ static int alg_setsockopt(struct socket *sock, int level, int optname,
>  	int err = -EBUSY;
>  
>  	lock_sock(sk);
> -	if (ask->refcnt)
> +	if (atomic_read(&ask->refcnt) != atomic_read(&ask->nokey_refcnt))
>  		goto unlock;
>  
>  	type = ask->type;
> @@ -305,12 +299,14 @@ int af_alg_accept(struct sock *sk, struct socket *newsock)
>  
>  	sk2->sk_family = PF_ALG;
>  
> -	if (nokey || !ask->refcnt++)
> +	if (atomic_inc_return_relaxed(&ask->refcnt) == 1)
>  		sock_hold(sk);
> -	ask->nokey_refcnt += nokey;
> +	if (nokey) {
> +		atomic_inc(&ask->nokey_refcnt);
> +		atomic_set(&alg_sk(sk2)->nokey_refcnt, 1);
> +	}
>  	alg_sk(sk2)->parent = sk;
>  	alg_sk(sk2)->type = type;
> -	alg_sk(sk2)->nokey_refcnt = nokey;
>  
>  	newsock->ops = type->ops;
>  	newsock->state = SS_CONNECTED;
> diff --git a/crypto/algif_aead.c b/crypto/algif_aead.c
> index faea9d728fd2..c50175ad5485 100644
> --- a/crypto/algif_aead.c
> +++ b/crypto/algif_aead.c
> @@ -528,7 +528,7 @@ static int aead_check_key(struct socket *sock)
>  	struct alg_sock *ask = alg_sk(sk);
>  
>  	lock_sock(sk);
> -	if (ask->refcnt)
> +	if (!atomic_read(&ask->nokey_refcnt))
>  		goto unlock_child;
>  
>  	psk = ask->parent;
> @@ -540,11 +540,8 @@ static int aead_check_key(struct socket *sock)
>  	if (!tfm->has_key)
>  		goto unlock;
>  
> -	if (!pask->refcnt++)
> -		sock_hold(psk);
> -
> -	ask->refcnt = 1;
> -	sock_put(psk);
> +	atomic_dec(&pask->nokey_refcnt);
> +	atomic_set(&ask->nokey_refcnt, 0);
>  
>  	err = 0;
>  
> diff --git a/crypto/algif_hash.c b/crypto/algif_hash.c
> index 8d8b3eeba725..fd23261f1d16 100644
> --- a/crypto/algif_hash.c
> +++ b/crypto/algif_hash.c
> @@ -252,7 +252,7 @@ static int hash_check_key(struct socket *sock)
>  	struct alg_sock *ask = alg_sk(sk);
>  
>  	lock_sock(sk);
> -	if (ask->refcnt)
> +	if (!atomic_read(&ask->nokey_refcnt))
>  		goto unlock_child;
>  
>  	psk = ask->parent;
> @@ -264,11 +264,8 @@ static int hash_check_key(struct socket *sock)
>  	if (!tfm->has_key)
>  		goto unlock;
>  
> -	if (!pask->refcnt++)
> -		sock_hold(psk);
> -
> -	ask->refcnt = 1;
> -	sock_put(psk);
> +	atomic_dec(&pask->nokey_refcnt);
> +	atomic_set(&ask->nokey_refcnt, 0);
>  
>  	err = 0;
>  
> diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
> index 9bd4691cc5c5..0e200bf5aa82 100644
> --- a/crypto/algif_skcipher.c
> +++ b/crypto/algif_skcipher.c
> @@ -774,7 +774,7 @@ static int skcipher_check_key(struct socket *sock)
>  	struct alg_sock *ask = alg_sk(sk);
>  
>  	lock_sock(sk);
> -	if (ask->refcnt)
> +	if (!atomic_read(&ask->nokey_refcnt))
>  		goto unlock_child;
>  
>  	psk = ask->parent;
> @@ -786,11 +786,8 @@ static int skcipher_check_key(struct socket *sock)
>  	if (!tfm->has_key)
>  		goto unlock;
>  
> -	if (!pask->refcnt++)
> -		sock_hold(psk);
> -
> -	ask->refcnt = 1;
> -	sock_put(psk);
> +	atomic_dec(&pask->nokey_refcnt);
> +	atomic_set(&ask->nokey_refcnt, 0);
>  
>  	err = 0;
>  
> diff --git a/include/crypto/if_alg.h b/include/crypto/if_alg.h
> index a2bfd7843f18..4bb6b98782e9 100644
> --- a/include/crypto/if_alg.h
> +++ b/include/crypto/if_alg.h
> @@ -30,8 +30,8 @@ struct alg_sock {
>  
>  	struct sock *parent;
>  
> -	unsigned int refcnt;
> -	unsigned int nokey_refcnt;
> +	atomic_t refcnt;
> +	atomic_t nokey_refcnt;
>  
>  	const struct af_alg_type *type;
>  	void *private;
> -- 
> 2.25.1
> 
> 
> -- 
> kernel-team mailing list
> kernel-team@lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team
diff mbox series

Patch

diff --git a/crypto/af_alg.c b/crypto/af_alg.c
index cf3975ee4fd8..c48ddeb6c328 100644
--- a/crypto/af_alg.c
+++ b/crypto/af_alg.c
@@ -130,21 +130,15 @@  EXPORT_SYMBOL_GPL(af_alg_release);
 void af_alg_release_parent(struct sock *sk)
 {
 	struct alg_sock *ask = alg_sk(sk);
-	unsigned int nokey = ask->nokey_refcnt;
-	bool last = nokey && !ask->refcnt;
+	unsigned int nokey = atomic_read(&ask->nokey_refcnt);
 
 	sk = ask->parent;
 	ask = alg_sk(sk);
 
-	local_bh_disable();
-	bh_lock_sock(sk);
-	ask->nokey_refcnt -= nokey;
-	if (!last)
-		last = !--ask->refcnt;
-	bh_unlock_sock(sk);
-	local_bh_enable();
+	if (nokey)
+		atomic_dec(&ask->nokey_refcnt);
 
-	if (last)
+	if (atomic_dec_and_test(&ask->refcnt))
 		sock_put(sk);
 }
 EXPORT_SYMBOL_GPL(af_alg_release_parent);
@@ -189,7 +183,7 @@  static int alg_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
 
 	err = -EBUSY;
 	lock_sock(sk);
-	if (ask->refcnt | ask->nokey_refcnt)
+	if (atomic_read(&ask->refcnt))
 		goto unlock;
 
 	swap(ask->type, type);
@@ -238,7 +232,7 @@  static int alg_setsockopt(struct socket *sock, int level, int optname,
 	int err = -EBUSY;
 
 	lock_sock(sk);
-	if (ask->refcnt)
+	if (atomic_read(&ask->refcnt) != atomic_read(&ask->nokey_refcnt))
 		goto unlock;
 
 	type = ask->type;
@@ -305,12 +299,14 @@  int af_alg_accept(struct sock *sk, struct socket *newsock)
 
 	sk2->sk_family = PF_ALG;
 
-	if (nokey || !ask->refcnt++)
+	if (atomic_inc_return_relaxed(&ask->refcnt) == 1)
 		sock_hold(sk);
-	ask->nokey_refcnt += nokey;
+	if (nokey) {
+		atomic_inc(&ask->nokey_refcnt);
+		atomic_set(&alg_sk(sk2)->nokey_refcnt, 1);
+	}
 	alg_sk(sk2)->parent = sk;
 	alg_sk(sk2)->type = type;
-	alg_sk(sk2)->nokey_refcnt = nokey;
 
 	newsock->ops = type->ops;
 	newsock->state = SS_CONNECTED;
diff --git a/crypto/algif_aead.c b/crypto/algif_aead.c
index faea9d728fd2..c50175ad5485 100644
--- a/crypto/algif_aead.c
+++ b/crypto/algif_aead.c
@@ -528,7 +528,7 @@  static int aead_check_key(struct socket *sock)
 	struct alg_sock *ask = alg_sk(sk);
 
 	lock_sock(sk);
-	if (ask->refcnt)
+	if (!atomic_read(&ask->nokey_refcnt))
 		goto unlock_child;
 
 	psk = ask->parent;
@@ -540,11 +540,8 @@  static int aead_check_key(struct socket *sock)
 	if (!tfm->has_key)
 		goto unlock;
 
-	if (!pask->refcnt++)
-		sock_hold(psk);
-
-	ask->refcnt = 1;
-	sock_put(psk);
+	atomic_dec(&pask->nokey_refcnt);
+	atomic_set(&ask->nokey_refcnt, 0);
 
 	err = 0;
 
diff --git a/crypto/algif_hash.c b/crypto/algif_hash.c
index 8d8b3eeba725..fd23261f1d16 100644
--- a/crypto/algif_hash.c
+++ b/crypto/algif_hash.c
@@ -252,7 +252,7 @@  static int hash_check_key(struct socket *sock)
 	struct alg_sock *ask = alg_sk(sk);
 
 	lock_sock(sk);
-	if (ask->refcnt)
+	if (!atomic_read(&ask->nokey_refcnt))
 		goto unlock_child;
 
 	psk = ask->parent;
@@ -264,11 +264,8 @@  static int hash_check_key(struct socket *sock)
 	if (!tfm->has_key)
 		goto unlock;
 
-	if (!pask->refcnt++)
-		sock_hold(psk);
-
-	ask->refcnt = 1;
-	sock_put(psk);
+	atomic_dec(&pask->nokey_refcnt);
+	atomic_set(&ask->nokey_refcnt, 0);
 
 	err = 0;
 
diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
index 9bd4691cc5c5..0e200bf5aa82 100644
--- a/crypto/algif_skcipher.c
+++ b/crypto/algif_skcipher.c
@@ -774,7 +774,7 @@  static int skcipher_check_key(struct socket *sock)
 	struct alg_sock *ask = alg_sk(sk);
 
 	lock_sock(sk);
-	if (ask->refcnt)
+	if (!atomic_read(&ask->nokey_refcnt))
 		goto unlock_child;
 
 	psk = ask->parent;
@@ -786,11 +786,8 @@  static int skcipher_check_key(struct socket *sock)
 	if (!tfm->has_key)
 		goto unlock;
 
-	if (!pask->refcnt++)
-		sock_hold(psk);
-
-	ask->refcnt = 1;
-	sock_put(psk);
+	atomic_dec(&pask->nokey_refcnt);
+	atomic_set(&ask->nokey_refcnt, 0);
 
 	err = 0;
 
diff --git a/include/crypto/if_alg.h b/include/crypto/if_alg.h
index a2bfd7843f18..4bb6b98782e9 100644
--- a/include/crypto/if_alg.h
+++ b/include/crypto/if_alg.h
@@ -30,8 +30,8 @@  struct alg_sock {
 
 	struct sock *parent;
 
-	unsigned int refcnt;
-	unsigned int nokey_refcnt;
+	atomic_t refcnt;
+	atomic_t nokey_refcnt;
 
 	const struct af_alg_type *type;
 	void *private;