[PATCH v3] crypto: remove instance when test failed

[PATCH v3] crypto: remove instance when test failed

[Stephan Mueller]

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 <smueller@chronox.de>
---
 crypto/algapi.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/crypto/algapi.c b/crypto/algapi.c
index f1d0307..cfca1de 100644
--- a/crypto/algapi.c
+++ b/crypto/algapi.c
@@ -533,6 +533,13 @@ int crypto_register_instance(struct crypto_template *tmpl,
 	if (IS_ERR(larval))
 		goto unlock;
 
+	err = -EAGAIN;
+	if (unlikely(!crypto_mod_get(&inst->alg))) {
+		up_write(&crypto_alg_sem);
+		crypto_unregister_instance(inst);
+		goto err;
+	}
+
 	hlist_add_head(&inst->list, &tmpl->instances);
 	inst->tmpl = tmpl;
 
@@ -544,8 +551,13 @@ 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;
 
+	crypto_mod_put(&inst->alg);
 err:
 	return err;
 }
-- 
2.1.0

Re: [PATCH v3] crypto: remove instance when test failed

[Herbert Xu]

On Thu, Apr 09, 2015 at 09:36:03AM +0200, Stephan Mueller wrote:
>
> diff --git a/crypto/algapi.c b/crypto/algapi.c
> index f1d0307..cfca1de 100644
> --- a/crypto/algapi.c
> +++ b/crypto/algapi.c
> @@ -533,6 +533,13 @@ int crypto_register_instance(struct crypto_template *tmpl,
>  	if (IS_ERR(larval))
>  		goto unlock;
>  
> +	err = -EAGAIN;
> +	if (unlikely(!crypto_mod_get(&inst->alg))) {
> +		up_write(&crypto_alg_sem);
> +		crypto_unregister_instance(inst);
> +		goto err;
> +	}

Just grab the reference count as soon as you enter the function
and then you can unconditionally drop the reference count at the
end.  If you fail to grab it then just return an error and the
caller will free it for you.

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

Re: [PATCH v3] crypto: remove instance when test failed

[Stephan Mueller]

Am Donnerstag, 9. April 2015, 15:41:41 schrieb Herbert Xu:

Hi Herbert,

>On Thu, Apr 09, 2015 at 09:36:03AM +0200, Stephan Mueller wrote:
>> diff --git a/crypto/algapi.c b/crypto/algapi.c
>> index f1d0307..cfca1de 100644
>> --- a/crypto/algapi.c
>> +++ b/crypto/algapi.c
>> @@ -533,6 +533,13 @@ int crypto_register_instance(struct crypto_template
>> *tmpl,> 
>>  	if (IS_ERR(larval))
>>  	
>>  		goto unlock;
>> 
>> +	err = -EAGAIN;
>> +	if (unlikely(!crypto_mod_get(&inst->alg))) {
>> +		up_write(&crypto_alg_sem);
>> +		crypto_unregister_instance(inst);
>> +		goto err;
>> +	}
>
>Just grab the reference count as soon as you enter the function
>and then you can unconditionally drop the reference count at the
>end.  If you fail to grab it then just return an error and the
>caller will free it for you.

I tested it and this approach does not work.

If I see that right, the reason for that is the following: The suggestion is 
to grab the ref count at the start of the function followed by a 
__crypto_register_alg. __crypto_register_alg however sets the refcount to 1 
unconditionally. That means that the final put of the alg will most likely set 
the refcount to 0 that causes an issue with all other operations (at least I 
cannot allocate HMAC or CMAC any more -- the ones I currently test).

So, the grabing of the alg must happen after the invocation of 
__crypto_register_alg.
>
>Cheers,


Ciao
Stephan

Re: [PATCH v3] crypto: remove instance when test failed

[Herbert Xu]

On Thu, Apr 09, 2015 at 11:22:19AM +0200, Stephan Mueller wrote:
>
> I tested it and this approach does not work.
> 
> If I see that right, the reason for that is the following: The suggestion is 
> to grab the ref count at the start of the function followed by a 
> __crypto_register_alg. __crypto_register_alg however sets the refcount to 1 
> unconditionally. That means that the final put of the alg will most likely set 
> the refcount to 0 that causes an issue with all other operations (at least I 
> cannot allocate HMAC or CMAC any more -- the ones I currently test).
> 
> So, the grabing of the alg must happen after the invocation of 
> __crypto_register_alg.

Well let's move it then.

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

diff --git a/crypto/algapi.c b/crypto/algapi.c
index f1d0307..1462c68 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;
-- 
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: [PATCH v3] crypto: remove instance when test failed

[Stephan Mueller]

Am Donnerstag, 9. April 2015, 17:40:35 schrieb Herbert Xu:

Hi Herbert,

>On Thu, Apr 09, 2015 at 11:22:19AM +0200, Stephan Mueller wrote:
>> I tested it and this approach does not work.
>> 
>> If I see that right, the reason for that is the following: The suggestion
>> is
>> to grab the ref count at the start of the function followed by a
>> __crypto_register_alg. __crypto_register_alg however sets the refcount to 1
>> unconditionally. That means that the final put of the alg will most likely
>> set the refcount to 0 that causes an issue with all other operations (at
>> least I cannot allocate HMAC or CMAC any more -- the ones I currently
>> test).
>> 
>> So, the grabing of the alg must happen after the invocation of
>> __crypto_register_alg.
>
>Well let's move it then.

Perfect. This now works with the changed patch too. I will resend my patch as 
v4 -- note that this new patch depends on your patch here as otherwise 
instances do not work at all. :-)
>
>---8<---
>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 <herbert@gondor.apana.org.au>

Acked-by: Stephan Mueller <smueller@chronox.de>
>
>diff --git a/crypto/algapi.c b/crypto/algapi.c
>index f1d0307..1462c68 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;


Ciao
Stephan