| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 7 Jul 2020 17:15:38 +0200
From: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To: linux-kernel@...r.kernel.org
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
stable@...r.kernel.org, Brian Moyles <bmoyles@...flix.com>,
Mauricio Faria de Oliveira <mfo@...onical.com>,
Herbert Xu <herbert@...dor.apana.org.au>
Subject: [PATCH 4.14 11/27] crypto: af_alg - fix use-after-free in af_alg_accept() due to bh_lock_sock()
From: Herbert Xu <herbert@...dor.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.com/
Reported-by: Brian Moyles <bmoyles@...flix.com>
Reported-by: Mauricio Faria de Oliveira <mfo@...onical.com>
Fixes: 37f96694cf73 ("crypto: af_alg - Use bh_lock_sock in...")
Cc: <stable@...r.kernel.org>
Signed-off-by: Herbert Xu <herbert@...dor.apana.org.au>
Signed-off-by: Greg Kroah-Hartman <gregkh@...uxfoundation.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
@@ -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,
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
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, 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
@@ -389,7 +389,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;
@@ -401,11 +401,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
@@ -309,7 +309,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;
@@ -321,11 +321,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
@@ -223,7 +223,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;
@@ -235,11 +235,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
@@ -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;
Powered by blists - more mailing lists