From: Herbert Xu herbert@gondor.apana.org.au
commit 34c86f4c4a7be3b3e35aa48bd18299d4c756064d upstream.
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.c...
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 Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org
--- 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(-)
--- 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,
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 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, struc
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; --- a/crypto/algif_aead.c +++ b/crypto/algif_aead.c @@ -747,7 +747,7 @@ static int aead_check_key(struct socket 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; @@ -759,11 +759,8 @@ static int aead_check_key(struct socket 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;
--- a/crypto/algif_hash.c +++ b/crypto/algif_hash.c @@ -308,7 +308,7 @@ static int hash_check_key(struct socket 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; @@ -320,11 +320,8 @@ static int hash_check_key(struct socket 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;
--- a/crypto/algif_skcipher.c +++ b/crypto/algif_skcipher.c @@ -774,7 +774,7 @@ static int skcipher_check_key(struct soc 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 soc 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;
--- 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;