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

Mauricio Oliveira mauricio.oliveira at canonical.com
Wed Jul 1 17:47:13 UTC 2020


On Wed, Jul 1, 2020 at 11:06 AM Stefan Bader <stefan.bader at canonical.com> wrote:
>
> 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.

Thanks for reviewing!

> Minor nitpick, you say this was written by the maintainer, but the patch and
> maintainer say it was you. ;)
>

Well, yes, _this_ patch is written by the maintainer :) I think he
meant credit for the
bug report and analysis, for which I did write _another_ patch (on
lore link above).
It used a different approach; but Herbert preferred to fix it in this
way in the end. :)

cheers,
Mauricio



> -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;
> >
>
>
> --
> kernel-team mailing list
> kernel-team at lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team



-- 
Mauricio Faria de Oliveira



More information about the kernel-team mailing list