From 1f7237109951ebe8dc194461716443a5d8caf308 Mon Sep 17 00:00:00 2001 From: Herbert Xu Date: Thu, 2 Apr 2015 22:31:22 +0800 Subject: crypto: api - Fix races in crypto_unregister_instance There are multiple problems in crypto_unregister_instance: 1) The cra_refcnt BUG_ON check is racy and can cause crashes. 2) The cra_refcnt check shouldn't exist at all. 3) There is no reference on tmpl to protect the tmpl->free call. This patch rewrites the function using crypto_remove_spawn which now morphs into crypto_remove_instance. Signed-off-by: Herbert Xu --- crypto/algapi.c | 23 +++++++---------------- 1 file changed, 7 insertions(+), 16 deletions(-) (limited to 'crypto/algapi.c') diff --git a/crypto/algapi.c b/crypto/algapi.c index 83b04e0884b1..0f1976eceb27 100644 --- a/crypto/algapi.c +++ b/crypto/algapi.c @@ -99,10 +99,9 @@ static struct list_head *crypto_more_spawns(struct crypto_alg *alg, return &n->list == stack ? top : &n->inst->alg.cra_users; } -static void crypto_remove_spawn(struct crypto_spawn *spawn, - struct list_head *list) +static void crypto_remove_instance(struct crypto_instance *inst, + struct list_head *list) { - struct crypto_instance *inst = spawn->inst; struct crypto_template *tmpl = inst->tmpl; if (crypto_is_dead(&inst->alg)) @@ -167,7 +166,7 @@ void crypto_remove_spawns(struct crypto_alg *alg, struct list_head *list, if (spawn->alg) list_move(&spawn->list, &spawn->alg->cra_users); else - crypto_remove_spawn(spawn, list); + crypto_remove_instance(spawn->inst, list); } } EXPORT_SYMBOL_GPL(crypto_remove_spawns); @@ -554,28 +553,20 @@ EXPORT_SYMBOL_GPL(crypto_register_instance); int crypto_unregister_instance(struct crypto_alg *alg) { - int err; struct crypto_instance *inst = (void *)alg; - struct crypto_template *tmpl = inst->tmpl; - LIST_HEAD(users); + LIST_HEAD(list); if (!(alg->cra_flags & CRYPTO_ALG_INSTANCE)) return -EINVAL; - BUG_ON(atomic_read(&alg->cra_refcnt) != 1); - down_write(&crypto_alg_sem); - hlist_del_init(&inst->list); - err = crypto_remove_alg(alg, &users); + crypto_remove_spawns(alg, &list, NULL); + crypto_remove_instance(inst, &list); up_write(&crypto_alg_sem); - if (err) - return err; - - tmpl->free(inst); - crypto_remove_final(&users); + crypto_remove_final(&list); return 0; } -- cgit v1.2.3 From 87b1675634e1e9f1570445c55604204771ac9078 Mon Sep 17 00:00:00 2001 From: Herbert Xu Date: Thu, 2 Apr 2015 22:39:40 +0800 Subject: crypto: api - Change crypto_unregister_instance argument type This patch makes crypto_unregister_instance take a crypto_instance instead of a crypto_alg. This allows us to remove a duplicate CRYPTO_ALG_INSTANCE check in crypto_unregister_instance. Signed-off-by: Herbert Xu --- crypto/algapi.c | 8 ++------ crypto/crypto_user.c | 2 +- include/crypto/algapi.h | 2 +- 3 files changed, 4 insertions(+), 8 deletions(-) (limited to 'crypto/algapi.c') diff --git a/crypto/algapi.c b/crypto/algapi.c index 0f1976eceb27..f1d0307b1d08 100644 --- a/crypto/algapi.c +++ b/crypto/algapi.c @@ -551,17 +551,13 @@ err: } EXPORT_SYMBOL_GPL(crypto_register_instance); -int crypto_unregister_instance(struct crypto_alg *alg) +int crypto_unregister_instance(struct crypto_instance *inst) { - struct crypto_instance *inst = (void *)alg; LIST_HEAD(list); - if (!(alg->cra_flags & CRYPTO_ALG_INSTANCE)) - return -EINVAL; - down_write(&crypto_alg_sem); - crypto_remove_spawns(alg, &list, NULL); + crypto_remove_spawns(&inst->alg, &list, NULL); crypto_remove_instance(inst, &list); up_write(&crypto_alg_sem); diff --git a/crypto/crypto_user.c b/crypto/crypto_user.c index c5148a35ae0a..eab249723830 100644 --- a/crypto/crypto_user.c +++ b/crypto/crypto_user.c @@ -316,7 +316,7 @@ static int crypto_del_alg(struct sk_buff *skb, struct nlmsghdr *nlh, if (atomic_read(&alg->cra_refcnt) != 1) return -EBUSY; - return crypto_unregister_instance(alg); + return crypto_unregister_instance((struct crypto_instance *)alg); } static struct crypto_alg *crypto_user_skcipher_alg(const char *name, u32 type, diff --git a/include/crypto/algapi.h b/include/crypto/algapi.h index 623a59c1ff5a..0ecb7688af71 100644 --- a/include/crypto/algapi.h +++ b/include/crypto/algapi.h @@ -137,7 +137,7 @@ struct crypto_template *crypto_lookup_template(const char *name); int crypto_register_instance(struct crypto_template *tmpl, struct crypto_instance *inst); -int crypto_unregister_instance(struct crypto_alg *alg); +int crypto_unregister_instance(struct crypto_instance *inst); int crypto_init_spawn(struct crypto_spawn *spawn, struct crypto_alg *alg, struct crypto_instance *inst, u32 mask); -- cgit v1.2.3 From e9b8e5beb7befe682f9fe90428ea534b01d66568 Mon Sep 17 00:00:00 2001 From: Herbert Xu Date: Thu, 9 Apr 2015 17:40:35 +0800 Subject: crypto: api - Move alg ref count init to crypto_check_alg We currently initialise the crypto_alg ref count in the function __crypto_register_alg. As one of the callers of that function crypto_register_instance needs to obtain a ref count before it calls __crypto_register_alg, we need to move the initialisation out of there. Since both callers of __crypto_register_alg call crypto_check_alg, this is the logical place to perform the initialisation. Signed-off-by: Herbert Xu Acked-by: Stephan Mueller --- crypto/algapi.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'crypto/algapi.c') diff --git a/crypto/algapi.c b/crypto/algapi.c index f1d0307b1d08..1462c68492ea 100644 --- a/crypto/algapi.c +++ b/crypto/algapi.c @@ -64,6 +64,8 @@ static int crypto_check_alg(struct crypto_alg *alg) if (alg->cra_priority < 0) return -EINVAL; + atomic_set(&alg->cra_refcnt, 1); + return crypto_set_driver_name(alg); } @@ -187,7 +189,6 @@ static struct crypto_larval *__crypto_register_alg(struct crypto_alg *alg) ret = -EEXIST; - atomic_set(&alg->cra_refcnt, 1); list_for_each_entry(q, &crypto_alg_list, cra_list) { if (q == alg) goto err; -- cgit v1.2.3 From 9c521a200bc3c12bd724e48a75c57d5358f672be Mon Sep 17 00:00:00 2001 From: Stephan Mueller Date: Thu, 9 Apr 2015 12:09:55 +0200 Subject: crypto: api - remove instance when test failed A cipher instance is added to the list of instances unconditionally regardless of whether the associated test failed. However, a failed test implies that during another lookup, the cipher instance will be added to the list again as it will not be found by the lookup code. That means that the list can be filled up with instances whose tests failed. Note: tests only fail in reality in FIPS mode when a cipher is not marked as fips_allowed=1. This can be seen with cmac(des3_ede) that does not have a fips_allowed=1. When allocating the cipher, the allocation fails with -ENOENT due to the missing fips_allowed=1 flag (which causes the testmgr to return EINVAL). Yet, the instance of cmac(des3_ede) is shown in /proc/crypto. Allocating the cipher again fails again, but a 2nd instance is listed in /proc/crypto. The patch simply de-registers the instance when the testing failed. Signed-off-by: Stephan Mueller Signed-off-by: Herbert Xu --- crypto/algapi.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) (limited to 'crypto/algapi.c') diff --git a/crypto/algapi.c b/crypto/algapi.c index 1462c68492ea..2d0a1c64ce39 100644 --- a/crypto/algapi.c +++ b/crypto/algapi.c @@ -523,7 +523,10 @@ int crypto_register_instance(struct crypto_template *tmpl, err = crypto_check_alg(&inst->alg); if (err) - goto err; + return err; + + if (unlikely(!crypto_mod_get(&inst->alg))) + return -EAGAIN; inst->alg.cra_module = tmpl->module; inst->alg.cra_flags |= CRYPTO_ALG_INSTANCE; @@ -545,9 +548,14 @@ unlock: goto err; crypto_wait_for_test(larval); + + /* Remove instance if test failed */ + if (!(inst->alg.cra_flags & CRYPTO_ALG_TESTED)) + crypto_unregister_instance(inst); err = 0; err: + crypto_mod_put(&inst->alg); return err; } EXPORT_SYMBOL_GPL(crypto_register_instance); -- cgit v1.2.3