linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] crypto: remove instance when test failed
@ 2015-04-02 17:05 Stephan Mueller
  2015-04-03  9:58 ` Herbert Xu
  0 siblings, 1 reply; 6+ messages in thread
From: Stephan Mueller @ 2015-04-02 17:05 UTC (permalink / raw)
  To: herbert; +Cc: linux-crypto, linux-kernel

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 | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/crypto/algapi.c b/crypto/algapi.c
index 83b04e0..215c604 100644
--- a/crypto/algapi.c
+++ b/crypto/algapi.c
@@ -545,6 +545,10 @@ 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:
-- 
2.1.0



^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] crypto: remove instance when test failed
  2015-04-02 17:05 [PATCH v2] crypto: remove instance when test failed Stephan Mueller
@ 2015-04-03  9:58 ` Herbert Xu
  2015-04-03 15:17   ` Stephan Mueller
  0 siblings, 1 reply; 6+ messages in thread
From: Herbert Xu @ 2015-04-03  9:58 UTC (permalink / raw)
  To: Stephan Mueller; +Cc: linux-crypto, linux-kernel

On Thu, Apr 02, 2015 at 07:05:27PM +0200, Stephan Mueller wrote:
>
> diff --git a/crypto/algapi.c b/crypto/algapi.c
> index 83b04e0..215c604 100644
> --- a/crypto/algapi.c
> +++ b/crypto/algapi.c
> @@ -545,6 +545,10 @@ 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);

Unfortunately I don't think this is safe because the moment you
release the mutex the instance can be freed.  So you'll need to
hold a reference to it.

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] crypto: remove instance when test failed
  2015-04-03  9:58 ` Herbert Xu
@ 2015-04-03 15:17   ` Stephan Mueller
  2015-04-03 15:52     ` Herbert Xu
  0 siblings, 1 reply; 6+ messages in thread
From: Stephan Mueller @ 2015-04-03 15:17 UTC (permalink / raw)
  To: Herbert Xu; +Cc: linux-crypto, linux-kernel

Am Freitag, 3. April 2015, 17:58:28 schrieb Herbert Xu:

Hi Herbert,

> On Thu, Apr 02, 2015 at 07:05:27PM +0200, Stephan Mueller wrote:
> > diff --git a/crypto/algapi.c b/crypto/algapi.c
> > index 83b04e0..215c604 100644
> > --- a/crypto/algapi.c
> > +++ b/crypto/algapi.c
> > 
> > @@ -545,6 +545,10 @@ 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);
> 
> Unfortunately I don't think this is safe because the moment you
> release the mutex the instance can be freed.  So you'll need to
> hold a reference to it.
> 
> Cheers,

Wpuldn't crypto_del_alg suffer from the same issue? I see that the cra_refcnt 
is checked. But I guess there would be the same kind of race?

-- 
Ciao
Stephan

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] crypto: remove instance when test failed
  2015-04-03 15:17   ` Stephan Mueller
@ 2015-04-03 15:52     ` Herbert Xu
  2015-04-03 19:29       ` Stephan Mueller
  0 siblings, 1 reply; 6+ messages in thread
From: Herbert Xu @ 2015-04-03 15:52 UTC (permalink / raw)
  To: Stephan Mueller; +Cc: linux-crypto, linux-kernel

On Fri, Apr 03, 2015 at 05:17:13PM +0200, Stephan Mueller wrote:
>
> Wpuldn't crypto_del_alg suffer from the same issue? I see that the cra_refcnt 
> is checked. But I guess there would be the same kind of race?

You're quite right.  It too needs to take a ref count on the
algorithm in crypto_alg_match before releasing the mutex.

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] crypto: remove instance when test failed
  2015-04-03 15:52     ` Herbert Xu
@ 2015-04-03 19:29       ` Stephan Mueller
  2015-04-06  4:45         ` Herbert Xu
  0 siblings, 1 reply; 6+ messages in thread
From: Stephan Mueller @ 2015-04-03 19:29 UTC (permalink / raw)
  To: Herbert Xu; +Cc: linux-crypto, linux-kernel

Am Freitag, 3. April 2015, 23:52:46 schrieb Herbert Xu:

Hi Herbert,

> On Fri, Apr 03, 2015 at 05:17:13PM +0200, Stephan Mueller wrote:
> > Wpuldn't crypto_del_alg suffer from the same issue? I see that the
> > cra_refcnt is checked. But I guess there would be the same kind of race?
> 
> You're quite right.  It too needs to take a ref count on the
> algorithm in crypto_alg_match before releasing the mutex.
> 

What about moving the current crypto_unregister_instance into 
__crypto_unregister_instance and creating a new crypto_unregister_instance 
that takes the ref count and once it got it, it calls 
__crypto_unregister_instance. If it did not get the ref count, it simply 
returns because the inst apparently was already removed.

Only the crypto_unregister_instance is EXPORT_SYMBOL whereas the 
__crypto_unregister_instance is static.

> Cheers,


-- 
Ciao
Stephan

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] crypto: remove instance when test failed
  2015-04-03 19:29       ` Stephan Mueller
@ 2015-04-06  4:45         ` Herbert Xu
  0 siblings, 0 replies; 6+ messages in thread
From: Herbert Xu @ 2015-04-06  4:45 UTC (permalink / raw)
  To: Stephan Mueller; +Cc: linux-crypto, linux-kernel

On Fri, Apr 03, 2015 at 09:29:44PM +0200, Stephan Mueller wrote:
>
> What about moving the current crypto_unregister_instance into 
> __crypto_unregister_instance and creating a new crypto_unregister_instance 
> that takes the ref count and once it got it, it calls 
> __crypto_unregister_instance. If it did not get the ref count, it simply 
> returns because the inst apparently was already removed.
> 
> Only the crypto_unregister_instance is EXPORT_SYMBOL whereas the 
> __crypto_unregister_instance is static.

That's not going to work because you need to grab the reference
count before you release the mutex.  Let me fix up crypto_user first
and then you can use it for your case too (or at least borrow the
idea).

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2015-04-06  4:45 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-04-02 17:05 [PATCH v2] crypto: remove instance when test failed Stephan Mueller
2015-04-03  9:58 ` Herbert Xu
2015-04-03 15:17   ` Stephan Mueller
2015-04-03 15:52     ` Herbert Xu
2015-04-03 19:29       ` Stephan Mueller
2015-04-06  4:45         ` Herbert Xu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).