linux-kernel - Re: crypto: api - Use work queue in crypto_destroy

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CABRcYmLWYr19MVsnrw2OEkmkpJLUNjPULgwooKxYO=R2J2mLww@mail.gmail.com>
Date:   Thu, 3 Aug 2023 14:13:05 +0200
From:   Florent Revest <revest@...omium.org>
To:     Herbert Xu <herbert@...dor.apana.org.au>
Cc:     linux-crypto@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-sctp@...r.kernel.org, davem@...emloft.net,
        hillf.zj@...baba-inc.com, marcelo.leitner@...il.com,
        lucien.xin@...il.com
Subject: Re: crypto: api - Use work queue in crypto_destroy_instance

On Thu, Aug 3, 2023 at 11:59 AM Herbert Xu <herbert@...dor.apana.org.au> wrote:
>
> On Wed, Aug 02, 2023 at 07:09:22PM +0200, Florent Revest wrote:
> >
> > I found that the following program reliably reproduces a "BUG: sleeping function
> > called from invalid context" backtrace in crypto code:
>
> Great detective work! And thanks for cc'ing me :)

Thank you!

> This is definitely a bug in the Crypto API.  Although it's hard to
> trigger because you need to unregister the instance before the last
> user frees it in atomic context.  The fact that it triggers for your
> test program probably means that we're not creating the template
> correctly and it gets unregistered as soon as it's created.
>
> As to the fix I think we should move the work into crypto_destroy_instance
> since that's the function that is being called from atomic context
> and then does something that should only be done from process context.

Sounds good to me :)

> So here's my patch based on your work:

FWIW,

Tested-by: Florent Revest <revest@...omium.org>
Acked-by: Florent Revest <revest@...omium.org>

> ---8<---
> The function crypto_drop_spawn expects to be called in process
> context.  However, when an instance is unregistered while it still
> has active users, the last user may cause the instance to be freed
> in atomic context.
>
> Fix this by delaying the freeing to a work queue.
>
> Fixes: 6bfd48096ff8 ("[CRYPTO] api: Added spawns")
> Reported-by: Florent Revest <revest@...omium.org>
> Reported-by: syzbot+d769eed29cc42d75e2a3@...kaller.appspotmail.com
> Reported-by: syzbot+610ec0671f51e838436e@...kaller.appspotmail.com
> Signed-off-by: Herbert Xu <herbert@...dor.apana.org.au>
>
> diff --git a/crypto/algapi.c b/crypto/algapi.c
> index 5e7cd603d489..4fe95c448047 100644
> --- a/crypto/algapi.c
> +++ b/crypto/algapi.c
> @@ -17,6 +17,7 @@
>  #include <linux/rtnetlink.h>
>  #include <linux/slab.h>
>  #include <linux/string.h>
> +#include <linux/workqueue.h>
>
>  #include "internal.h"
>
> @@ -74,15 +75,26 @@ static void crypto_free_instance(struct crypto_instance *inst)
>         inst->alg.cra_type->free(inst);
>  }
>
> -static void crypto_destroy_instance(struct crypto_alg *alg)
> +static void crypto_destroy_instance_workfn(struct work_struct *w)
>  {
> -       struct crypto_instance *inst = (void *)alg;
> +       struct crypto_instance *inst = container_of(w, struct crypto_instance,
> +                                                   free_work);
>         struct crypto_template *tmpl = inst->tmpl;
>
>         crypto_free_instance(inst);
>         crypto_tmpl_put(tmpl);
>  }
>
> +static void crypto_destroy_instance(struct crypto_alg *alg)
> +{
> +       struct crypto_instance *inst = container_of(alg,
> +                                                   struct crypto_instance,
> +                                                   alg);
> +
> +       INIT_WORK(&inst->free_work, crypto_destroy_instance_workfn);
> +       schedule_work(&inst->free_work);
> +}
> +
>  /*
>   * This function adds a spawn to the list secondary_spawns which
>   * will be used at the end of crypto_remove_spawns to unregister
> diff --git a/include/crypto/algapi.h b/include/crypto/algapi.h
> index 6156161b181f..ca86f4c6ba43 100644
> --- a/include/crypto/algapi.h
> +++ b/include/crypto/algapi.h
> @@ -12,6 +12,7 @@
>  #include <linux/cache.h>
>  #include <linux/crypto.h>
>  #include <linux/types.h>
> +#include <linux/workqueue.h>
>
>  /*
>   * Maximum values for blocksize and alignmask, used to allocate
> @@ -82,6 +83,8 @@ struct crypto_instance {
>                 struct crypto_spawn *spawns;
>         };
>
> +       struct work_struct free_work;
> +
>         void *__ctx[] CRYPTO_MINALIGN_ATTR;
>  };