crypto: api - Fix races in crypto_unregister_instance

crypto: api - Fix races in crypto_unregister_instance

[Herbert Xu]

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 <herbert@gondor.apana.org.au>

diff --git a/crypto/algapi.c b/crypto/algapi.c
index 83b04e0..0f1976e 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;
 }
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

crypto: api - Change crypto_unregister_instance argument type

[Herbert Xu]

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 <herbert@gondor.apana.org.au>

diff --git a/crypto/algapi.c b/crypto/algapi.c
index 0f1976e..f1d0307 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 c5148a3..eab2497 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 623a59c..0ecb768 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);
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: crypto: api - Change crypto_unregister_instance argument type

[Stephan Mueller]

Am Donnerstag, 2. April 2015, 22:39:40 schrieb Herbert Xu:

Hi Herbert,

>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 <herbert@gondor.apana.org.au>
>
>diff --git a/crypto/algapi.c b/crypto/algapi.c
>index 0f1976e..f1d0307 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 c5148a3..eab2497 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);

Instead of using type casts, isn't container_of() a cleaner way (and hopefully 
a safer way) of casting? 
> }
>
> 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 623a59c..0ecb768 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);


Ciao
Stephan

Re: crypto: api - Change crypto_unregister_instance argument type

[Herbert Xu]

On Thu, Apr 02, 2015 at 04:54:03PM +0200, Stephan Mueller wrote:
>
> >@@ -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);
> 
> Instead of using type casts, isn't container_of() a cleaner way (and hopefully 
> a safer way) of casting? 

Actually no that would only make sense if every alg sat within
an instance which is not the case.  So I don't think using that
would buy us much clarity.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: crypto: api - Change crypto_unregister_instance argument type

[Stephan Mueller]

Am Donnerstag, 2. April 2015, 22:59:11 schrieb Herbert Xu:

Hi Herbert,

>On Thu, Apr 02, 2015 at 04:54:03PM +0200, Stephan Mueller wrote:
>> >@@ -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);
>> 
>> Instead of using type casts, isn't container_of() a cleaner way (and
>> hopefully a safer way) of casting?
>
>Actually no that would only make sense if every alg sat within
>an instance which is not the case.  So I don't think using that
>would buy us much clarity.

In crypto_remove_instance, the code dereferences inst->tmpl. If you say that 
it is not guaranteed that the alg is always wrapped by an inst, wouldn't we 
have a potential invalid pointer?
>
>Thanks,


Ciao
Stephan

Re: crypto: api - Change crypto_unregister_instance argument type

[Herbert Xu]

On Thu, Apr 02, 2015 at 05:13:23PM +0200, Stephan Mueller wrote:
>
> In crypto_remove_instance, the code dereferences inst->tmpl. If you say that 
> it is not guaranteed that the alg is always wrapped by an inst, wouldn't we 
> have a potential invalid pointer?

This particular spot is always an instance because we just checked
it.  In any case the container_of buys us nothing and simply makes
the code more convoluted.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt