Re: crypto: api - Use work queue in crypto_destroy_instance

[Florent Revest]

On Thu, Aug 3, 2023 at 11:59 AM Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxx>
Acked-by: Florent Revest <revest@xxxxxxxxxxxx>

> ---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@xxxxxxxxxxxx>
> Reported-by: syzbot+d769eed29cc42d75e2a3@xxxxxxxxxxxxxxxxxxxxxxxxx
> Reported-by: syzbot+610ec0671f51e838436e@xxxxxxxxxxxxxxxxxxxxxxxxx
> Signed-off-by: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>
>
> 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;
>  };