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

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.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>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

diff --git a/crypto/af_alg.c b/crypto/af_alg.c
index 4fc8e6a7abb2e9a87447ba247beb56c12c680a81..272879d7b0d1f5c203ca5999130d86264aa99550 100644 (file)
--- a/crypto/af_alg.c
+++ b/crypto/af_alg.c
@@ -133,21 +133,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);
@@ -192,7 +186,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);
@@ -241,7 +235,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;
@@ -308,12 +302,14 @@ int af_alg_accept(struct sock *sk, struct socket *newsock, bool kern)
 
        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 c40a8c7ee8aedcb0f6adb3afb1e0bb60a233d68c..b4f16202ab7af36e625f02ff1d4f8e9f2de5b6f6 100644 (file)
--- a/crypto/algif_aead.c
+++ b/crypto/algif_aead.c
@@ -388,7 +388,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;
@@ -400,11 +400,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 bfcf595fd8f9850b00166a7b3f16852fce393583..42521d0d7bfeb6e0186a7cf9f44d354186c5f6cb 100644 (file)
--- a/crypto/algif_hash.c
+++ b/crypto/algif_hash.c
@@ -306,7 +306,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;
@@ -318,11 +318,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 1380a208b8cab8118af36da4cf9725742432e9fd..1cb106c46043d808cda900ede2529ca1ac463ad2 100644 (file)
--- a/crypto/algif_skcipher.c
+++ b/crypto/algif_skcipher.c
@@ -215,7 +215,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;
@@ -227,11 +227,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 482461d8931d9186c4a11b7b2d9a24f981a595bc..11f107df78dc34c70a1f37589e1dbb4308f46287 100644 (file)
--- a/include/crypto/if_alg.h
+++ b/include/crypto/if_alg.h
@@ -34,8 +34,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;