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;
> };
