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

Stefan Bader stefan.bader at canonical.com
Wed Jul 1 14:06:26 UTC 2020


On 01.07.20 01:09, Mauricio Faria de Oliveira wrote:
> From: Herbert Xu <herbert at 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 at netflix.com>
> Reported-by: Mauricio Faria de Oliveira <mfo at canonical.com>
> Fixes: 37f96694cf73 ("crypto: af_alg - Use bh_lock_sock in...")
> Cc: <stable at vger.kernel.org>
> Signed-off-by: Herbert Xu <herbert at gondor.apana.org.au>
> (cherry picked from commit 34c86f4c4a7be3b3e35aa48bd18299d4c756064d)
> Signed-off-by: Mauricio Faria de Oliveira <mfo at canonical.com>
Acked-by: Stefan Bader <stefan.bader at 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;
> 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20200701/3f69fa63/attachment.sig>


More information about the kernel-team mailing list