APPLIED: [B][PATCH] crypto: af_alg - fix use-after-free in af_alg_accept() due to bh_lock_sock()
Kelsey Skunberg
kelsey.skunberg at canonical.com
Thu Jul 30 21:15:05 UTC 2020
Applied to Bionic/master-next. Thank you!
-Kelsey
On 2020-06-30 20:10:11 , 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>
> (backported from commit 34c86f4c4a7be3b3e35aa48bd18299d4c756064d)
> [mfo: backport: refresh context lines in af_alg.c hunk 4,
> algif_aead.c hunk 2, and algif_skcipher.c hunk 2]
> Signed-off-by: Mauricio Faria de Oliveira <mfo at 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 ff6d7d32e92f..29a8746de715 100644
> --- 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 e9885a35ef6e..799957341348 100644
> --- a/crypto/algif_aead.c
> +++ b/crypto/algif_aead.c
> @@ -389,7 +389,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;
> @@ -401,11 +401,8 @@ static int aead_check_key(struct socket *sock)
> 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;
>
> diff --git a/crypto/algif_hash.c b/crypto/algif_hash.c
> index 6c9b1927a520..891eab7bd421 100644
> --- a/crypto/algif_hash.c
> +++ b/crypto/algif_hash.c
> @@ -307,7 +307,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;
> @@ -319,11 +319,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 c5c47b680152..8f45dc3db335 100644
> --- a/crypto/algif_skcipher.c
> +++ b/crypto/algif_skcipher.c
> @@ -226,7 +226,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;
> @@ -238,11 +238,8 @@ static int skcipher_check_key(struct socket *sock)
> 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;
>
> diff --git a/include/crypto/if_alg.h b/include/crypto/if_alg.h
> index f38227a78eae..93297e69e42e 100644
> --- 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;
> --
> 2.25.1
>
>
> --
> kernel-team mailing list
> kernel-team at lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team
More information about the kernel-team
mailing list