[PATCH] crypto: tcrypt: add option to not exit on success

[PATCH] crypto: tcrypt: add option to not exit on success

[Jarod Wilson]

At present, the tcrypt module always exits with an -EAGAIN upon
successfully completing all the tests its been asked to run. There
are cases where it would be much simpler to verify all tests passed
if tcrypt simply stayed loaded (i.e. returned 0). Specifically, in
fips mode, all self-tests need to be run from the initrd, and its
much simpler to check the ret from modprobe for success than to
scrape dmesg. To make this doable, I've simply added a module param
to allow this behavior, leaving the default behavior more or less
the same as before, although now we're tracking all success/failure
rets as well.

The tcrypt_test() portion of the patch is dependent on my earlier
pair of patches that skip non-fips algs in fips mode, at least to
achieve the fully intended behavior.

Signed-off-by: Jarod Wilson <jarod@redhat.com>

---
 crypto/tcrypt.c |  168 +++++++++++++++++++++++++++++++------------------------
 1 files changed, 94 insertions(+), 74 deletions(-)

diff --git a/crypto/tcrypt.c b/crypto/tcrypt.c
index 9e4974e..dee729c 100644
--- a/crypto/tcrypt.c
+++ b/crypto/tcrypt.c
@@ -27,6 +27,7 @@
 #include <linux/timex.h>
 #include <linux/interrupt.h>
 #include "tcrypt.h"
+#include "internal.h"
 
 /*
  * Need slab memory for testing (size in number of pages).
@@ -45,6 +46,7 @@
 static unsigned int sec;
 
 static int mode;
+static int noexit;
 static char *tvmem[TVMEMSIZE];
 
 static char *check[] = {
@@ -468,248 +470,255 @@ static void test_available(void)
 
 static inline int tcrypt_test(const char *alg)
 {
-	return alg_test(alg, alg, 0, 0);
+	int ret;
+
+	ret = alg_test(alg, alg, 0, 0);
+	/* non-fips algs return -EINVAL in fips mode */
+	if (fips_enabled && ret == -EINVAL)
+		ret = 0;
+	return ret;
 }
 
-static void do_test(int m)
+static int do_test(int m)
 {
 	int i;
+	int ret = 0;
 
 	switch (m) {
 	case 0:
 		for (i = 1; i < 200; i++)
-			do_test(i);
+			ret += do_test(i);
 		break;
 
 	case 1:
-		tcrypt_test("md5");
+		ret += tcrypt_test("md5");
 		break;
 
 	case 2:
-		tcrypt_test("sha1");
+		ret += tcrypt_test("sha1");
 		break;
 
 	case 3:
-		tcrypt_test("ecb(des)");
-		tcrypt_test("cbc(des)");
+		ret += tcrypt_test("ecb(des)");
+		ret += tcrypt_test("cbc(des)");
 		break;
 
 	case 4:
-		tcrypt_test("ecb(des3_ede)");
-		tcrypt_test("cbc(des3_ede)");
+		ret += tcrypt_test("ecb(des3_ede)");
+		ret += tcrypt_test("cbc(des3_ede)");
 		break;
 
 	case 5:
-		tcrypt_test("md4");
+		ret += tcrypt_test("md4");
 		break;
 
 	case 6:
-		tcrypt_test("sha256");
+		ret += tcrypt_test("sha256");
 		break;
 
 	case 7:
-		tcrypt_test("ecb(blowfish)");
-		tcrypt_test("cbc(blowfish)");
+		ret += tcrypt_test("ecb(blowfish)");
+		ret += tcrypt_test("cbc(blowfish)");
 		break;
 
 	case 8:
-		tcrypt_test("ecb(twofish)");
-		tcrypt_test("cbc(twofish)");
+		ret += tcrypt_test("ecb(twofish)");
+		ret += tcrypt_test("cbc(twofish)");
 		break;
 
 	case 9:
-		tcrypt_test("ecb(serpent)");
+		ret += tcrypt_test("ecb(serpent)");
 		break;
 
 	case 10:
-		tcrypt_test("ecb(aes)");
-		tcrypt_test("cbc(aes)");
-		tcrypt_test("lrw(aes)");
-		tcrypt_test("xts(aes)");
-		tcrypt_test("ctr(aes)");
-		tcrypt_test("rfc3686(ctr(aes))");
+		ret += tcrypt_test("ecb(aes)");
+		ret += tcrypt_test("cbc(aes)");
+		ret += tcrypt_test("lrw(aes)");
+		ret += tcrypt_test("xts(aes)");
+		ret += tcrypt_test("ctr(aes)");
+		ret += tcrypt_test("rfc3686(ctr(aes))");
 		break;
 
 	case 11:
-		tcrypt_test("sha384");
+		ret += tcrypt_test("sha384");
 		break;
 
 	case 12:
-		tcrypt_test("sha512");
+		ret += tcrypt_test("sha512");
 		break;
 
 	case 13:
-		tcrypt_test("deflate");
+		ret += tcrypt_test("deflate");
 		break;
 
 	case 14:
-		tcrypt_test("ecb(cast5)");
+		ret += tcrypt_test("ecb(cast5)");
 		break;
 
 	case 15:
-		tcrypt_test("ecb(cast6)");
+		ret += tcrypt_test("ecb(cast6)");
 		break;
 
 	case 16:
-		tcrypt_test("ecb(arc4)");
+		ret += tcrypt_test("ecb(arc4)");
 		break;
 
 	case 17:
-		tcrypt_test("michael_mic");
+		ret += tcrypt_test("michael_mic");
 		break;
 
 	case 18:
-		tcrypt_test("crc32c");
+		ret += tcrypt_test("crc32c");
 		break;
 
 	case 19:
-		tcrypt_test("ecb(tea)");
+		ret += tcrypt_test("ecb(tea)");
 		break;
 
 	case 20:
-		tcrypt_test("ecb(xtea)");
+		ret += tcrypt_test("ecb(xtea)");
 		break;
 
 	case 21:
-		tcrypt_test("ecb(khazad)");
+		ret += tcrypt_test("ecb(khazad)");
 		break;
 
 	case 22:
-		tcrypt_test("wp512");
+		ret += tcrypt_test("wp512");
 		break;
 
 	case 23:
-		tcrypt_test("wp384");
+		ret += tcrypt_test("wp384");
 		break;
 
 	case 24:
-		tcrypt_test("wp256");
+		ret += tcrypt_test("wp256");
 		break;
 
 	case 25:
-		tcrypt_test("ecb(tnepres)");
+		ret += tcrypt_test("ecb(tnepres)");
 		break;
 
 	case 26:
-		tcrypt_test("ecb(anubis)");
-		tcrypt_test("cbc(anubis)");
+		ret += tcrypt_test("ecb(anubis)");
+		ret += tcrypt_test("cbc(anubis)");
 		break;
 
 	case 27:
-		tcrypt_test("tgr192");
+		ret += tcrypt_test("tgr192");
 		break;
 
 	case 28:
 
-		tcrypt_test("tgr160");
+		ret += tcrypt_test("tgr160");
 		break;
 
 	case 29:
-		tcrypt_test("tgr128");
+		ret += tcrypt_test("tgr128");
 		break;
 
 	case 30:
-		tcrypt_test("ecb(xeta)");
+		ret += tcrypt_test("ecb(xeta)");
 		break;
 
 	case 31:
-		tcrypt_test("pcbc(fcrypt)");
+		ret += tcrypt_test("pcbc(fcrypt)");
 		break;
 
 	case 32:
-		tcrypt_test("ecb(camellia)");
-		tcrypt_test("cbc(camellia)");
+		ret += tcrypt_test("ecb(camellia)");
+		ret += tcrypt_test("cbc(camellia)");
 		break;
 	case 33:
-		tcrypt_test("sha224");
+		ret += tcrypt_test("sha224");
 		break;
 
 	case 34:
-		tcrypt_test("salsa20");
+		ret += tcrypt_test("salsa20");
 		break;
 
 	case 35:
-		tcrypt_test("gcm(aes)");
+		ret += tcrypt_test("gcm(aes)");
 		break;
 
 	case 36:
-		tcrypt_test("lzo");
+		ret += tcrypt_test("lzo");
 		break;
 
 	case 37:
-		tcrypt_test("ccm(aes)");
+		ret += tcrypt_test("ccm(aes)");
 		break;
 
 	case 38:
-		tcrypt_test("cts(cbc(aes))");
+		ret += tcrypt_test("cts(cbc(aes))");
 		break;
 
         case 39:
-		tcrypt_test("rmd128");
+		ret += tcrypt_test("rmd128");
 		break;
 
         case 40:
-		tcrypt_test("rmd160");
+		ret += tcrypt_test("rmd160");
 		break;
 
 	case 41:
-		tcrypt_test("rmd256");
+		ret += tcrypt_test("rmd256");
 		break;
 
 	case 42:
-		tcrypt_test("rmd320");
+		ret += tcrypt_test("rmd320");
 		break;
 
 	case 43:
-		tcrypt_test("ecb(seed)");
+		ret += tcrypt_test("ecb(seed)");
 		break;
 
 	case 44:
-		tcrypt_test("zlib");
+		ret += tcrypt_test("zlib");
 		break;
 
 	case 45:
-		tcrypt_test("rfc4309(ccm(aes))");
+		ret += tcrypt_test("rfc4309(ccm(aes))");
 		break;
 
 	case 100:
-		tcrypt_test("hmac(md5)");
+		ret += tcrypt_test("hmac(md5)");
 		break;
 
 	case 101:
-		tcrypt_test("hmac(sha1)");
+		ret += tcrypt_test("hmac(sha1)");
 		break;
 
 	case 102:
-		tcrypt_test("hmac(sha256)");
+		ret += tcrypt_test("hmac(sha256)");
 		break;
 
 	case 103:
-		tcrypt_test("hmac(sha384)");
+		ret += tcrypt_test("hmac(sha384)");
 		break;
 
 	case 104:
-		tcrypt_test("hmac(sha512)");
+		ret += tcrypt_test("hmac(sha512)");
 		break;
 
 	case 105:
-		tcrypt_test("hmac(sha224)");
+		ret += tcrypt_test("hmac(sha224)");
 		break;
 
 	case 106:
-		tcrypt_test("xcbc(aes)");
+		ret += tcrypt_test("xcbc(aes)");
 		break;
 
 	case 107:
-		tcrypt_test("hmac(rmd128)");
+		ret += tcrypt_test("hmac(rmd128)");
 		break;
 
 	case 108:
-		tcrypt_test("hmac(rmd160)");
+		ret += tcrypt_test("hmac(rmd160)");
 		break;
 
 	case 150:
-		tcrypt_test("ansi_cprng");
+		ret += tcrypt_test("ansi_cprng");
 		break;
 
 	case 200:
@@ -873,6 +882,8 @@ static void do_test(int m)
 		test_available();
 		break;
 	}
+
+	return ret;
 }
 
 static int __init tcrypt_mod_init(void)
@@ -886,15 +897,21 @@ static int __init tcrypt_mod_init(void)
 			goto err_free_tv;
 	}
 
-	do_test(mode);
+	err = do_test(mode);
+	if (err) {
+		printk(KERN_ERR "tcrypt: one or more tests failed!\n");
+		goto err_free_tv;
+	}
 
-	/* We intentionaly return -EAGAIN to prevent keeping
-	 * the module. It does all its work from init()
-	 * and doesn't offer any runtime functionality 
+	/* We intentionaly return -EAGAIN to prevent keeping the module,
+	 * unless it was loaded w/the noexit param set. It does all its
+	 * work from init() and doesn't offer any runtime functionality,
+	 * but in some cases, checking for a successful load is helpful.
 	 * => we don't need it in the memory, do we?
 	 *                                        -- mludvig
 	 */
-	err = -EAGAIN;
+	if (!noexit)
+		err = -EAGAIN;
 
 err_free_tv:
 	for (i = 0; i < TVMEMSIZE && tvmem[i]; i++)
@@ -916,6 +933,9 @@ module_param(mode, int, 0);
 module_param(sec, uint, 0);
 MODULE_PARM_DESC(sec, "Length in seconds of speed tests "
 		      "(defaults to zero which uses CPU cycles instead)");
+module_param(noexit, int, 0);
+MODULE_PARM_DESC(noexit, "Whether or not the module should simply exit "
+		 "after running tests. (default is to exit).");
 
 MODULE_LICENSE("GPL");
 MODULE_DESCRIPTION("Quick & dirty crypto testing module");


-- 
Jarod Wilson
jarod@redhat.com

Re: [PATCH] crypto: tcrypt: add option to not exit on success

[Jarod Wilson]

On Monday 11 May 2009 10:06:32 Jarod Wilson wrote:
> At present, the tcrypt module always exits with an -EAGAIN upon
> successfully completing all the tests its been asked to run. There
> are cases where it would be much simpler to verify all tests passed
> if tcrypt simply stayed loaded (i.e. returned 0). Specifically, in
> fips mode, all self-tests need to be run from the initrd, and its
> much simpler to check the ret from modprobe for success than to
> scrape dmesg. To make this doable, I've simply added a module param
> to allow this behavior, leaving the default behavior more or less
> the same as before, although now we're tracking all success/failure
> rets as well.

I've been reminded that a self-test failure in fips mode means an
immediate panic, so modprobe never sees the ret in that case, but if
the module load failed for other reasons, a non-zero return value
from modprobe is possible w/o traversing the code paths that trigger
a self-test failure panic. For one, if the tcrypt module were to go
missing for some reason, modprobe would have a non-zero ret, and the
initrd would need to handle panicking the system.

Would there be any objections to dropping the noexit parameter
entirely and just making its behavior the default? It would make
all users regardless of fips mode notice failures more readily.

-- 
Jarod Wilson
jarod@redhat.com

Re: [PATCH] crypto: tcrypt: add option to not exit on success

[Neil Horman]

On Tue, May 12, 2009 at 04:02:45PM -0400, Jarod Wilson wrote:
> On Monday 11 May 2009 10:06:32 Jarod Wilson wrote:
> > At present, the tcrypt module always exits with an -EAGAIN upon
> > successfully completing all the tests its been asked to run. There
> > are cases where it would be much simpler to verify all tests passed
> > if tcrypt simply stayed loaded (i.e. returned 0). Specifically, in
> > fips mode, all self-tests need to be run from the initrd, and its
> > much simpler to check the ret from modprobe for success than to
> > scrape dmesg. To make this doable, I've simply added a module param
> > to allow this behavior, leaving the default behavior more or less
> > the same as before, although now we're tracking all success/failure
> > rets as well.
> 
> I've been reminded that a self-test failure in fips mode means an
> immediate panic, so modprobe never sees the ret in that case, but if
> the module load failed for other reasons, a non-zero return value
> from modprobe is possible w/o traversing the code paths that trigger
> a self-test failure panic. For one, if the tcrypt module were to go
> missing for some reason, modprobe would have a non-zero ret, and the
> initrd would need to handle panicking the system.
> 
> Would there be any objections to dropping the noexit parameter
> entirely and just making its behavior the default? It would make
> all users regardless of fips mode notice failures more readily.
> 
I think thats a fine idea.  Theres no reason that a user of the tcrypt module
can't manually rmmod it when the testing is done.  Doing it that way just seems
more sane to me to begin with anyway.

Neil

> -- 
> Jarod Wilson
> jarod@redhat.com
> 

Re: [PATCH] crypto: tcrypt: add option to not exit on success

[Herbert Xu]

On Tue, May 12, 2009 at 08:37:27PM -0400, Neil Horman wrote:
>
> > Would there be any objections to dropping the noexit parameter
> > entirely and just making its behavior the default? It would make
> > all users regardless of fips mode notice failures more readily.
> > 
> I think thats a fine idea.  Theres no reason that a user of the tcrypt module
> can't manually rmmod it when the testing is done.  Doing it that way just seems
> more sane to me to begin with anyway.

No, tcrypt is only a relic for correctness testing.  Its main
purpose these days is for speed testing.  Having to rmmod it
is silly.

There's really no need to load tcrypt for correctness testing
anymore.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <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] crypto: tcrypt: add option to not exit on success

[Neil Horman]

On Wed, May 13, 2009 at 11:30:50AM +1000, Herbert Xu wrote:
> On Tue, May 12, 2009 at 08:37:27PM -0400, Neil Horman wrote:
> >
> > > Would there be any objections to dropping the noexit parameter
> > > entirely and just making its behavior the default? It would make
> > > all users regardless of fips mode notice failures more readily.
> > > 
> > I think thats a fine idea.  Theres no reason that a user of the tcrypt module
> > can't manually rmmod it when the testing is done.  Doing it that way just seems
> > more sane to me to begin with anyway.
> 
> No, tcrypt is only a relic for correctness testing.  Its main
> purpose these days is for speed testing.  Having to rmmod it
> is silly.
> 
> There's really no need to load tcrypt for correctness testing
> anymore.
> 

Not really sure I agree with the logic here.  I agree that its pretty clear that
its major value is for quickly testing all the algorithms in a system, but
universally failing the loading of the module simply to save a few milliseconds
seems like a poor choice.  In so doing you create an alias effect, as jarod
noted between a non-existent module and a module that failed to load.  The
aliasing can be resolved, if you want to parse dmesg, but if speed is the issue
at hand, that parsing is a significant impact.  If you allow the module to load
properly, then for the cost of an rmmod, you can tell simply from the exit code
of modprobe:
1) If the module was found 
2) If the tests passed

And if the rmmod is simply to expensive for whatever reason, then for the cost
of a few k of ram taken up by the module, you can choose not to unload it.

Of course, if tcrypt is really as much of a relic as you say, perhaps that is an
argument for removing the module entirely.  Perhaps the testmgr interface could
be exported to userspace and the tcrypt tests be packaged as a userspace suite.

Regards
Neil

> Cheers,
> -- 
> Visit Openswan at http://www.openswan.org/
> Email: Herbert Xu ~{PmV>HI~} <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] crypto: tcrypt: add option to not exit on success

[Herbert Xu]

On Wed, May 13, 2009 at 07:08:26AM -0400, Neil Horman wrote:
>
> Not really sure I agree with the logic here.  I agree that its pretty clear that
> its major value is for quickly testing all the algorithms in a system, but
> universally failing the loading of the module simply to save a few milliseconds
> seems like a poor choice.  In so doing you create an alias effect, as jarod
> noted between a non-existent module and a module that failed to load.  The
> aliasing can be resolved, if you want to parse dmesg, but if speed is the issue
> at hand, that parsing is a significant impact.  If you allow the module to load
> properly, then for the cost of an rmmod, you can tell simply from the exit code
> of modprobe:
> 1) If the module was found 
> 2) If the tests passed

As I said, tcrypt should no longer be used to test algorithms.
Algorithms are tested as they load, with the result available
in /proc/crypto.  tcrypt is just a relic that will be going away.

Do not rely on it.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <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] crypto: tcrypt: add option to not exit on success

[Jarod Wilson]

On Wednesday 13 May 2009 07:38:19 Herbert Xu wrote:
> On Wed, May 13, 2009 at 07:08:26AM -0400, Neil Horman wrote:
> >
> > Not really sure I agree with the logic here.  I agree that its pretty clear that
> > its major value is for quickly testing all the algorithms in a system, but
> > universally failing the loading of the module simply to save a few milliseconds
> > seems like a poor choice.  In so doing you create an alias effect, as jarod
> > noted between a non-existent module and a module that failed to load.  The
> > aliasing can be resolved, if you want to parse dmesg, but if speed is the issue
> > at hand, that parsing is a significant impact.  If you allow the module to load
> > properly, then for the cost of an rmmod, you can tell simply from the exit code
> > of modprobe:
> > 1) If the module was found 
> > 2) If the tests passed
> 
> As I said, tcrypt should no longer be used to test algorithms.
> Algorithms are tested as they load, with the result available
> in /proc/crypto.  tcrypt is just a relic that will be going away.
> 
> Do not rely on it.

Hm... FIPS has the requirement that we test all algs before we use any
algs, self-tests on demand before first use for each alg is
insufficient. At first blush, I'm not seeing how we ensure this
happens. How can we trigger a cbc(des3_ede) self-test from userspace?
I see that modprobe'ing des.ko runs the base des and des3_ede
self-tests, but modprobe'ing cbc.ko doesn't lead to any self-tests
being run.

-- 
Jarod Wilson
jarod@redhat.com

Re: [PATCH] crypto: tcrypt: add option to not exit on success

[Herbert Xu]

On Wed, May 13, 2009 at 09:12:46AM -0400, Jarod Wilson wrote:
> 
> Hm... FIPS has the requirement that we test all algs before we use any
> algs, self-tests on demand before first use for each alg is
> insufficient. At first blush, I'm not seeing how we ensure this
> happens. How can we trigger a cbc(des3_ede) self-test from userspace?
> I see that modprobe'ing des.ko runs the base des and des3_ede
> self-tests, but modprobe'ing cbc.ko doesn't lead to any self-tests
> being run.

Once we have a user-space interface crypto API you will be able
to instantiate any given algorithm.

For now I suggest that you create your own module to instantiate
these FIPS algorithms.  Or just load tcrypt and ignore the exit
status, or make tcrypt return 0 if we're in FIPS mode.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <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] crypto: tcrypt: add option to not exit on success

[Jarod Wilson]

On Wednesday 13 May 2009 09:27:52 Herbert Xu wrote:
> On Wed, May 13, 2009 at 09:12:46AM -0400, Jarod Wilson wrote:
> > 
> > Hm... FIPS has the requirement that we test all algs before we use any
> > algs, self-tests on demand before first use for each alg is
> > insufficient. At first blush, I'm not seeing how we ensure this
> > happens. How can we trigger a cbc(des3_ede) self-test from userspace?
> > I see that modprobe'ing des.ko runs the base des and des3_ede
> > self-tests, but modprobe'ing cbc.ko doesn't lead to any self-tests
> > being run.
> 
> Once we have a user-space interface crypto API you will be able
> to instantiate any given algorithm.
> 
> For now I suggest that you create your own module to instantiate
> these FIPS algorithms.  Or just load tcrypt and ignore the exit
> status, or make tcrypt return 0 if we're in FIPS mode.

The latter option is more or less what the patch at the start of this
thread did, although via a param to tcrypt, not keying off the fips
flag. If I were to modify the patch to drop the mod param usage, and
instead key off the fips flag to not exit, would that be acceptable
for committing until such time as the userspace interface is ready?

-- 
Jarod Wilson
jarod@redhat.com

Re: [PATCH] crypto: tcrypt: add option to not exit on success

[Neil Horman]

On Wed, May 13, 2009 at 11:27:52PM +1000, Herbert Xu wrote:
> On Wed, May 13, 2009 at 09:12:46AM -0400, Jarod Wilson wrote:
> > 
> > Hm... FIPS has the requirement that we test all algs before we use any
> > algs, self-tests on demand before first use for each alg is
> > insufficient. At first blush, I'm not seeing how we ensure this
> > happens. How can we trigger a cbc(des3_ede) self-test from userspace?
> > I see that modprobe'ing des.ko runs the base des and des3_ede
> > self-tests, but modprobe'ing cbc.ko doesn't lead to any self-tests
> > being run.
> 
> Once we have a user-space interface crypto API you will be able
> to instantiate any given algorithm.
> 
Thats a good idea.  Jarod, didn't you create a generic netlink socket family
module that created just such an interface for testing purposes?  That might be
worth polishing and submitting to provide that user space crypto api

Neil

> 

Re: [PATCH] crypto: tcrypt: add option to not exit on success

[Herbert Xu]

On Wed, May 13, 2009 at 09:37:32AM -0400, Jarod Wilson wrote:
> 
> The latter option is more or less what the patch at the start of this
> thread did, although via a param to tcrypt, not keying off the fips
> flag. If I were to modify the patch to drop the mod param usage, and
> instead key off the fips flag to not exit, would that be acceptable
> for committing until such time as the userspace interface is ready?

Yes that sounds like the way to go.

Thanks,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[PATCH v2] crypto: tcrypt: do not exit on success in fips mode

[Jarod Wilson]

On Wednesday 13 May 2009 10:03:29 Herbert Xu wrote:
> On Wed, May 13, 2009 at 09:37:32AM -0400, Jarod Wilson wrote:
> > 
> > The latter option is more or less what the patch at the start of this
> > thread did, although via a param to tcrypt, not keying off the fips
> > flag. If I were to modify the patch to drop the mod param usage, and
> > instead key off the fips flag to not exit, would that be acceptable
> > for committing until such time as the userspace interface is ready?
> 
> Yes that sounds like the way to go.

Okay, here it is.

At present, the tcrypt module always exits with an -EAGAIN upon
successfully completing all the tests its been asked to run. In fips
mode, integrity checking is done by running all self-tests from the
initrd, and its much simpler to check the ret from modprobe for
success than to scrape dmesg and/or /proc/crypto. Simply stay
loaded, giving modprobe a retval of 0, if self-tests all pass and
we're in fips mode.

A side-effect of tracking success/failure for fips mode is that in
non-fips mode, self-test failures will return the actual failure
return codes, rather than always returning -EAGAIN, which seems more
correct anyway.

The tcrypt_test() portion of the patch is dependent on my earlier
pair of patches that skip non-fips algs in fips mode, at least to
achieve the fully intended behavior.

Nb: testing this patch against the cryptodev tree revealed a test
failure for sha384, which I have yet to look into...

Signed-off-by: Jarod Wilson <jarod@redhat.com>

---
 crypto/tcrypt.c |  164 ++++++++++++++++++++++++++++++-------------------------
 1 files changed, 90 insertions(+), 74 deletions(-)

diff --git a/crypto/tcrypt.c b/crypto/tcrypt.c
index 9e4974e..d59ba50 100644
--- a/crypto/tcrypt.c
+++ b/crypto/tcrypt.c
@@ -27,6 +27,7 @@
 #include <linux/timex.h>
 #include <linux/interrupt.h>
 #include "tcrypt.h"
+#include "internal.h"
 
 /*
  * Need slab memory for testing (size in number of pages).
@@ -468,248 +469,255 @@ static void test_available(void)
 
 static inline int tcrypt_test(const char *alg)
 {
-	return alg_test(alg, alg, 0, 0);
+	int ret;
+
+	ret = alg_test(alg, alg, 0, 0);
+	/* non-fips algs return -EINVAL in fips mode */
+	if (fips_enabled && ret == -EINVAL)
+		ret = 0;
+	return ret;
 }
 
-static void do_test(int m)
+static int do_test(int m)
 {
 	int i;
+	int ret = 0;
 
 	switch (m) {
 	case 0:
 		for (i = 1; i < 200; i++)
-			do_test(i);
+			ret += do_test(i);
 		break;
 
 	case 1:
-		tcrypt_test("md5");
+		ret += tcrypt_test("md5");
 		break;
 
 	case 2:
-		tcrypt_test("sha1");
+		ret += tcrypt_test("sha1");
 		break;
 
 	case 3:
-		tcrypt_test("ecb(des)");
-		tcrypt_test("cbc(des)");
+		ret += tcrypt_test("ecb(des)");
+		ret += tcrypt_test("cbc(des)");
 		break;
 
 	case 4:
-		tcrypt_test("ecb(des3_ede)");
-		tcrypt_test("cbc(des3_ede)");
+		ret += tcrypt_test("ecb(des3_ede)");
+		ret += tcrypt_test("cbc(des3_ede)");
 		break;
 
 	case 5:
-		tcrypt_test("md4");
+		ret += tcrypt_test("md4");
 		break;
 
 	case 6:
-		tcrypt_test("sha256");
+		ret += tcrypt_test("sha256");
 		break;
 
 	case 7:
-		tcrypt_test("ecb(blowfish)");
-		tcrypt_test("cbc(blowfish)");
+		ret += tcrypt_test("ecb(blowfish)");
+		ret += tcrypt_test("cbc(blowfish)");
 		break;
 
 	case 8:
-		tcrypt_test("ecb(twofish)");
-		tcrypt_test("cbc(twofish)");
+		ret += tcrypt_test("ecb(twofish)");
+		ret += tcrypt_test("cbc(twofish)");
 		break;
 
 	case 9:
-		tcrypt_test("ecb(serpent)");
+		ret += tcrypt_test("ecb(serpent)");
 		break;
 
 	case 10:
-		tcrypt_test("ecb(aes)");
-		tcrypt_test("cbc(aes)");
-		tcrypt_test("lrw(aes)");
-		tcrypt_test("xts(aes)");
-		tcrypt_test("ctr(aes)");
-		tcrypt_test("rfc3686(ctr(aes))");
+		ret += tcrypt_test("ecb(aes)");
+		ret += tcrypt_test("cbc(aes)");
+		ret += tcrypt_test("lrw(aes)");
+		ret += tcrypt_test("xts(aes)");
+		ret += tcrypt_test("ctr(aes)");
+		ret += tcrypt_test("rfc3686(ctr(aes))");
 		break;
 
 	case 11:
-		tcrypt_test("sha384");
+		ret += tcrypt_test("sha384");
 		break;
 
 	case 12:
-		tcrypt_test("sha512");
+		ret += tcrypt_test("sha512");
 		break;
 
 	case 13:
-		tcrypt_test("deflate");
+		ret += tcrypt_test("deflate");
 		break;
 
 	case 14:
-		tcrypt_test("ecb(cast5)");
+		ret += tcrypt_test("ecb(cast5)");
 		break;
 
 	case 15:
-		tcrypt_test("ecb(cast6)");
+		ret += tcrypt_test("ecb(cast6)");
 		break;
 
 	case 16:
-		tcrypt_test("ecb(arc4)");
+		ret += tcrypt_test("ecb(arc4)");
 		break;
 
 	case 17:
-		tcrypt_test("michael_mic");
+		ret += tcrypt_test("michael_mic");
 		break;
 
 	case 18:
-		tcrypt_test("crc32c");
+		ret += tcrypt_test("crc32c");
 		break;
 
 	case 19:
-		tcrypt_test("ecb(tea)");
+		ret += tcrypt_test("ecb(tea)");
 		break;
 
 	case 20:
-		tcrypt_test("ecb(xtea)");
+		ret += tcrypt_test("ecb(xtea)");
 		break;
 
 	case 21:
-		tcrypt_test("ecb(khazad)");
+		ret += tcrypt_test("ecb(khazad)");
 		break;
 
 	case 22:
-		tcrypt_test("wp512");
+		ret += tcrypt_test("wp512");
 		break;
 
 	case 23:
-		tcrypt_test("wp384");
+		ret += tcrypt_test("wp384");
 		break;
 
 	case 24:
-		tcrypt_test("wp256");
+		ret += tcrypt_test("wp256");
 		break;
 
 	case 25:
-		tcrypt_test("ecb(tnepres)");
+		ret += tcrypt_test("ecb(tnepres)");
 		break;
 
 	case 26:
-		tcrypt_test("ecb(anubis)");
-		tcrypt_test("cbc(anubis)");
+		ret += tcrypt_test("ecb(anubis)");
+		ret += tcrypt_test("cbc(anubis)");
 		break;
 
 	case 27:
-		tcrypt_test("tgr192");
+		ret += tcrypt_test("tgr192");
 		break;
 
 	case 28:
 
-		tcrypt_test("tgr160");
+		ret += tcrypt_test("tgr160");
 		break;
 
 	case 29:
-		tcrypt_test("tgr128");
+		ret += tcrypt_test("tgr128");
 		break;
 
 	case 30:
-		tcrypt_test("ecb(xeta)");
+		ret += tcrypt_test("ecb(xeta)");
 		break;
 
 	case 31:
-		tcrypt_test("pcbc(fcrypt)");
+		ret += tcrypt_test("pcbc(fcrypt)");
 		break;
 
 	case 32:
-		tcrypt_test("ecb(camellia)");
-		tcrypt_test("cbc(camellia)");
+		ret += tcrypt_test("ecb(camellia)");
+		ret += tcrypt_test("cbc(camellia)");
 		break;
 	case 33:
-		tcrypt_test("sha224");
+		ret += tcrypt_test("sha224");
 		break;
 
 	case 34:
-		tcrypt_test("salsa20");
+		ret += tcrypt_test("salsa20");
 		break;
 
 	case 35:
-		tcrypt_test("gcm(aes)");
+		ret += tcrypt_test("gcm(aes)");
 		break;
 
 	case 36:
-		tcrypt_test("lzo");
+		ret += tcrypt_test("lzo");
 		break;
 
 	case 37:
-		tcrypt_test("ccm(aes)");
+		ret += tcrypt_test("ccm(aes)");
 		break;
 
 	case 38:
-		tcrypt_test("cts(cbc(aes))");
+		ret += tcrypt_test("cts(cbc(aes))");
 		break;
 
         case 39:
-		tcrypt_test("rmd128");
+		ret += tcrypt_test("rmd128");
 		break;
 
         case 40:
-		tcrypt_test("rmd160");
+		ret += tcrypt_test("rmd160");
 		break;
 
 	case 41:
-		tcrypt_test("rmd256");
+		ret += tcrypt_test("rmd256");
 		break;
 
 	case 42:
-		tcrypt_test("rmd320");
+		ret += tcrypt_test("rmd320");
 		break;
 
 	case 43:
-		tcrypt_test("ecb(seed)");
+		ret += tcrypt_test("ecb(seed)");
 		break;
 
 	case 44:
-		tcrypt_test("zlib");
+		ret += tcrypt_test("zlib");
 		break;
 
 	case 45:
-		tcrypt_test("rfc4309(ccm(aes))");
+		ret += tcrypt_test("rfc4309(ccm(aes))");
 		break;
 
 	case 100:
-		tcrypt_test("hmac(md5)");
+		ret += tcrypt_test("hmac(md5)");
 		break;
 
 	case 101:
-		tcrypt_test("hmac(sha1)");
+		ret += tcrypt_test("hmac(sha1)");
 		break;
 
 	case 102:
-		tcrypt_test("hmac(sha256)");
+		ret += tcrypt_test("hmac(sha256)");
 		break;
 
 	case 103:
-		tcrypt_test("hmac(sha384)");
+		ret += tcrypt_test("hmac(sha384)");
 		break;
 
 	case 104:
-		tcrypt_test("hmac(sha512)");
+		ret += tcrypt_test("hmac(sha512)");
 		break;
 
 	case 105:
-		tcrypt_test("hmac(sha224)");
+		ret += tcrypt_test("hmac(sha224)");
 		break;
 
 	case 106:
-		tcrypt_test("xcbc(aes)");
+		ret += tcrypt_test("xcbc(aes)");
 		break;
 
 	case 107:
-		tcrypt_test("hmac(rmd128)");
+		ret += tcrypt_test("hmac(rmd128)");
 		break;
 
 	case 108:
-		tcrypt_test("hmac(rmd160)");
+		ret += tcrypt_test("hmac(rmd160)");
 		break;
 
 	case 150:
-		tcrypt_test("ansi_cprng");
+		ret += tcrypt_test("ansi_cprng");
 		break;
 
 	case 200:
@@ -873,6 +881,8 @@ static void do_test(int m)
 		test_available();
 		break;
 	}
+
+	return ret;
 }
 
 static int __init tcrypt_mod_init(void)
@@ -886,15 +896,21 @@ static int __init tcrypt_mod_init(void)
 			goto err_free_tv;
 	}
 
-	do_test(mode);
+	err = do_test(mode);
+	if (err) {
+		printk(KERN_ERR "tcrypt: one or more tests failed!\n");
+		goto err_free_tv;
+	}
 
-	/* We intentionaly return -EAGAIN to prevent keeping
-	 * the module. It does all its work from init()
-	 * and doesn't offer any runtime functionality 
+	/* We intentionaly return -EAGAIN to prevent keeping the module,
+	 * unless we're running in fips mode. It does all its work from
+	 * init() and doesn't offer any runtime functionality, but in
+	 * the fips case, checking for a successful load is helpful.
 	 * => we don't need it in the memory, do we?
 	 *                                        -- mludvig
 	 */
-	err = -EAGAIN;
+	if (!fips_enabled)
+		err = -EAGAIN;
 
 err_free_tv:
 	for (i = 0; i < TVMEMSIZE && tvmem[i]; i++)


-- 
Jarod Wilson
jarod@redhat.com

Re: [PATCH] crypto: tcrypt: add option to not exit on success

[Jarod Wilson]

On Wednesday 13 May 2009 10:02:25 Neil Horman wrote:
> On Wed, May 13, 2009 at 11:27:52PM +1000, Herbert Xu wrote:
> > On Wed, May 13, 2009 at 09:12:46AM -0400, Jarod Wilson wrote:
> > > 
> > > Hm... FIPS has the requirement that we test all algs before we use any
> > > algs, self-tests on demand before first use for each alg is
> > > insufficient. At first blush, I'm not seeing how we ensure this
> > > happens. How can we trigger a cbc(des3_ede) self-test from userspace?
> > > I see that modprobe'ing des.ko runs the base des and des3_ede
> > > self-tests, but modprobe'ing cbc.ko doesn't lead to any self-tests
> > > being run.
> > 
> > Once we have a user-space interface crypto API you will be able
> > to instantiate any given algorithm.
> > 
> Thats a good idea.  Jarod, didn't you create a generic netlink socket family
> module that created just such an interface for testing purposes?

Indeed. Works quite well for my somewhat specific needs...

> That might be
> worth polishing and submitting to provide that user space crypto api

It would likely need a LOT of polish, and I'm not sure if its at all
close to what we have (Herbert has?) in mind.... At the moment, it
consists of:

1) a kernel module, which duplicates many of the functions in testmgr,
more or less, but gets its input over a generic netlink socket, rather
than from a static array, and sends the result back out over the same
socket.

2) a userspace binary that feeds very specifically formatted vectors
to the kernel via the netlink socket, listens for the result and
spits it out -- it doesn't actually verify the correctness of the
result, but adding support for passing in an expected result and
checking against it would be trivial.

I guess the place to start would be to ask exactly what sort of
user-space interface to the crypto API did we have in mind here?
i.e., what sort of user-space<->kernel-space communication is
envisioned, and what sort of functionality should be present?

I could see the desire for a simpler interface that doesn't rely upon
a specific userspace binary -- something sysfs or proc-based -- but
netlink is reasonably flexible...

-- 
Jarod Wilson
jarod@redhat.com

Re: [PATCH v2] crypto: tcrypt: do not exit on success in fips mode

[Neil Horman]

On Wed, May 13, 2009 at 12:40:10PM -0400, Jarod Wilson wrote:
> On Wednesday 13 May 2009 10:03:29 Herbert Xu wrote:
> > On Wed, May 13, 2009 at 09:37:32AM -0400, Jarod Wilson wrote:
> > > 
> > > The latter option is more or less what the patch at the start of this
> > > thread did, although via a param to tcrypt, not keying off the fips
> > > flag. If I were to modify the patch to drop the mod param usage, and
> > > instead key off the fips flag to not exit, would that be acceptable
> > > for committing until such time as the userspace interface is ready?
> > 
> > Yes that sounds like the way to go.
> 
> Okay, here it is.
> 
> At present, the tcrypt module always exits with an -EAGAIN upon
> successfully completing all the tests its been asked to run. In fips
> mode, integrity checking is done by running all self-tests from the
> initrd, and its much simpler to check the ret from modprobe for
> success than to scrape dmesg and/or /proc/crypto. Simply stay
> loaded, giving modprobe a retval of 0, if self-tests all pass and
> we're in fips mode.
> 
> A side-effect of tracking success/failure for fips mode is that in
> non-fips mode, self-test failures will return the actual failure
> return codes, rather than always returning -EAGAIN, which seems more
> correct anyway.
> 
> The tcrypt_test() portion of the patch is dependent on my earlier
> pair of patches that skip non-fips algs in fips mode, at least to
> achieve the fully intended behavior.
> 
> Nb: testing this patch against the cryptodev tree revealed a test
> failure for sha384, which I have yet to look into...
> 
> Signed-off-by: Jarod Wilson <jarod@redhat.com>
> 
Acked-by: Neil Horman <nhorman@tuxdriver.com>

> 

Re: [PATCH] crypto: tcrypt: add option to not exit on success

[Herbert Xu]

On Wed, May 13, 2009 at 12:53:16PM -0400, Jarod Wilson wrote:
> 
> It would likely need a LOT of polish, and I'm not sure if its at all
> close to what we have (Herbert has?) in mind.... At the moment, it
> consists of:

The interface I had in mind can be found in the recent discussions
in the linux-crypto archives.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <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] crypto: tcrypt: add option to not exit on success

[Jarod Wilson]

On Wednesday 13 May 2009 19:45:50 Herbert Xu wrote:
> On Wed, May 13, 2009 at 12:53:16PM -0400, Jarod Wilson wrote:
> > 
> > It would likely need a LOT of polish, and I'm not sure if its at all
> > close to what we have (Herbert has?) in mind.... At the moment, it
> > consists of:
> 
> The interface I had in mind can be found in the recent discussions
> in the linux-crypto archives.

Ah, sorry, I found it. So pretty much absolutely nothing like the
netlink-based stuff I've done. :) I'll go further familiarize
myself with the proposed patch...

-- 
Jarod Wilson
jarod@redhat.com

Re: [PATCH v2] crypto: tcrypt: do not exit on success in fips mode

[Herbert Xu]

On Wed, May 13, 2009 at 01:32:19PM -0400, Neil Horman wrote:
>
> > Signed-off-by: Jarod Wilson <jarod@redhat.com>
> > 
> Acked-by: Neil Horman <nhorman@tuxdriver.com>

Applied to cryptodev.  Thanks!
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt