diff mbox series

[E/F/Unstable,1/1] crypto: af_alg - fix use-after-free in af_alg_accept() due to bh_lock_sock()

Message ID 20200630230942.1576682-2-mfo@canonical.com
State New
Headers show
Series crypto: fix regression/use-after-free in af_alg_accept() | expand

Commit Message

Mauricio Faria de Oliveira June 30, 2020, 11:09 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>
(cherry picked from commit 34c86f4c4a7be3b3e35aa48bd18299d4c756064d)
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:06 p.m. UTC | #1 
On 01.07.20 01:09, 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>
> (cherry picked from commit 34c86f4c4a7be3b3e35aa48bd18299d4c756064d)
> Signed-off-by: Mauricio Faria de Oliveira <mfo@canonical.com>
Acked-by: Stefan Bader <stefan.bader@canonical.com>
> ---

Glancing over all looks to be logical but as you say it is complex code, so the
major arguments pro are well tested and reviewed and accepted upstream.
Minor nitpick, you say this was written by the maintainer, but the patch and
maintainer say it was you. ;)

-Stefan

>  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 b1cd3535c525..28fc323e3fe3 100644
> --- a/crypto/af_alg.c
> +++ b/crypto/af_alg.c
> @@ -128,21 +128,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);
> @@ -187,7 +181,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);
> @@ -236,7 +230,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;
> @@ -301,12 +295,14 @@ int af_alg_accept(struct sock *sk, struct socket *newsock, bool kern)
>  	if (err)
>  		goto unlock;
>  
> -	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 eb1910b6d434..0ae000a61c7f 100644
> --- a/crypto/algif_aead.c
> +++ b/crypto/algif_aead.c
> @@ -384,7 +384,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;
> @@ -396,11 +396,8 @@ static int aead_check_key(struct socket *sock)
>  	if (crypto_aead_get_flags(tfm->aead) & CRYPTO_TFM_NEED_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 da1ffa4f7f8d..e71727c25a7d 100644
> --- a/crypto/algif_hash.c
> +++ b/crypto/algif_hash.c
> @@ -301,7 +301,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;
> @@ -313,11 +313,8 @@ static int hash_check_key(struct socket *sock)
>  	if (crypto_ahash_get_flags(tfm) & CRYPTO_TFM_NEED_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 4c3bdffe0c3a..ec5567c87a6d 100644
> --- a/crypto/algif_skcipher.c
> +++ b/crypto/algif_skcipher.c
> @@ -211,7 +211,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;
> @@ -223,11 +223,8 @@ static int skcipher_check_key(struct socket *sock)
>  	if (crypto_skcipher_get_flags(tfm) & CRYPTO_TFM_NEED_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 56527c85d122..088c1ded2714 100644
> --- a/include/crypto/if_alg.h
> +++ b/include/crypto/if_alg.h
> @@ -29,8 +29,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;
>
Mauricio Oliveira July 1, 2020, 5:47 p.m. UTC | #2 
On Wed, Jul 1, 2020 at 11:06 AM Stefan Bader <stefan.bader@canonical.com> wrote:
>
> On 01.07.20 01:09, 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>
> > (cherry picked from commit 34c86f4c4a7be3b3e35aa48bd18299d4c756064d)
> > Signed-off-by: Mauricio Faria de Oliveira <mfo@canonical.com>
> Acked-by: Stefan Bader <stefan.bader@canonical.com>
> > ---
>
> Glancing over all looks to be logical but as you say it is complex code, so the
> major arguments pro are well tested and reviewed and accepted upstream.

Thanks for reviewing!

> Minor nitpick, you say this was written by the maintainer, but the patch and
> maintainer say it was you. ;)
>

Well, yes, _this_ patch is written by the maintainer :) I think he
meant credit for the
bug report and analysis, for which I did write _another_ patch (on
lore link above).
It used a different approach; but Herbert preferred to fix it in this
way in the end. :)

cheers,
Mauricio



> -Stefan
>
> >  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 b1cd3535c525..28fc323e3fe3 100644
> > --- a/crypto/af_alg.c
> > +++ b/crypto/af_alg.c
> > @@ -128,21 +128,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);
> > @@ -187,7 +181,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);
> > @@ -236,7 +230,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;
> > @@ -301,12 +295,14 @@ int af_alg_accept(struct sock *sk, struct socket *newsock, bool kern)
> >       if (err)
> >               goto unlock;
> >
> > -     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 eb1910b6d434..0ae000a61c7f 100644
> > --- a/crypto/algif_aead.c
> > +++ b/crypto/algif_aead.c
> > @@ -384,7 +384,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;
> > @@ -396,11 +396,8 @@ static int aead_check_key(struct socket *sock)
> >       if (crypto_aead_get_flags(tfm->aead) & CRYPTO_TFM_NEED_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 da1ffa4f7f8d..e71727c25a7d 100644
> > --- a/crypto/algif_hash.c
> > +++ b/crypto/algif_hash.c
> > @@ -301,7 +301,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;
> > @@ -313,11 +313,8 @@ static int hash_check_key(struct socket *sock)
> >       if (crypto_ahash_get_flags(tfm) & CRYPTO_TFM_NEED_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 4c3bdffe0c3a..ec5567c87a6d 100644
> > --- a/crypto/algif_skcipher.c
> > +++ b/crypto/algif_skcipher.c
> > @@ -211,7 +211,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;
> > @@ -223,11 +223,8 @@ static int skcipher_check_key(struct socket *sock)
> >       if (crypto_skcipher_get_flags(tfm) & CRYPTO_TFM_NEED_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 56527c85d122..088c1ded2714 100644
> > --- a/include/crypto/if_alg.h
> > +++ b/include/crypto/if_alg.h
> > @@ -29,8 +29,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;
> >
>
>
> --
> kernel-team mailing list
> kernel-team@lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team
Andrea Righi July 3, 2020, 6:50 a.m. UTC | #3 
On Tue, Jun 30, 2020 at 08:09:42PM -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>
> (cherry picked from commit 34c86f4c4a7be3b3e35aa48bd18299d4c756064d)
> Signed-off-by: Mauricio Faria de Oliveira <mfo@canonical.com>
> ---

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 b1cd3535c525..28fc323e3fe3 100644
> --- a/crypto/af_alg.c
> +++ b/crypto/af_alg.c
> @@ -128,21 +128,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);
> @@ -187,7 +181,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);
> @@ -236,7 +230,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;
> @@ -301,12 +295,14 @@ int af_alg_accept(struct sock *sk, struct socket *newsock, bool kern)
>  	if (err)
>  		goto unlock;
>  
> -	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 eb1910b6d434..0ae000a61c7f 100644
> --- a/crypto/algif_aead.c
> +++ b/crypto/algif_aead.c
> @@ -384,7 +384,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;
> @@ -396,11 +396,8 @@ static int aead_check_key(struct socket *sock)
>  	if (crypto_aead_get_flags(tfm->aead) & CRYPTO_TFM_NEED_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 da1ffa4f7f8d..e71727c25a7d 100644
> --- a/crypto/algif_hash.c
> +++ b/crypto/algif_hash.c
> @@ -301,7 +301,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;
> @@ -313,11 +313,8 @@ static int hash_check_key(struct socket *sock)
>  	if (crypto_ahash_get_flags(tfm) & CRYPTO_TFM_NEED_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 4c3bdffe0c3a..ec5567c87a6d 100644
> --- a/crypto/algif_skcipher.c
> +++ b/crypto/algif_skcipher.c
> @@ -211,7 +211,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;
> @@ -223,11 +223,8 @@ static int skcipher_check_key(struct socket *sock)
>  	if (crypto_skcipher_get_flags(tfm) & CRYPTO_TFM_NEED_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 56527c85d122..088c1ded2714 100644
> --- a/include/crypto/if_alg.h
> +++ b/include/crypto/if_alg.h
> @@ -29,8 +29,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 b1cd3535c525..28fc323e3fe3 100644
--- a/crypto/af_alg.c
+++ b/crypto/af_alg.c
@@ -128,21 +128,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);
@@ -187,7 +181,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);
@@ -236,7 +230,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;
@@ -301,12 +295,14 @@  int af_alg_accept(struct sock *sk, struct socket *newsock, bool kern)
 	if (err)
 		goto unlock;
 
-	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 eb1910b6d434..0ae000a61c7f 100644
--- a/crypto/algif_aead.c
+++ b/crypto/algif_aead.c
@@ -384,7 +384,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;
@@ -396,11 +396,8 @@  static int aead_check_key(struct socket *sock)
 	if (crypto_aead_get_flags(tfm->aead) & CRYPTO_TFM_NEED_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 da1ffa4f7f8d..e71727c25a7d 100644
--- a/crypto/algif_hash.c
+++ b/crypto/algif_hash.c
@@ -301,7 +301,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;
@@ -313,11 +313,8 @@  static int hash_check_key(struct socket *sock)
 	if (crypto_ahash_get_flags(tfm) & CRYPTO_TFM_NEED_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 4c3bdffe0c3a..ec5567c87a6d 100644
--- a/crypto/algif_skcipher.c
+++ b/crypto/algif_skcipher.c
@@ -211,7 +211,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;
@@ -223,11 +223,8 @@  static int skcipher_check_key(struct socket *sock)
 	if (crypto_skcipher_get_flags(tfm) & CRYPTO_TFM_NEED_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 56527c85d122..088c1ded2714 100644
--- a/include/crypto/if_alg.h
+++ b/include/crypto/if_alg.h
@@ -29,8 +29,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;