I'm trying to understand the Linux crypto layer, and a lot of
the code I read for guidance I instead end up wanting to fix.
My current itch to scratch is crypto/ansi_cprng.c.
There is a lot of questionable code I'll submit patches for, particularly:
- The "rand_data_valid" variable, which is actually the amount of
INvalid data in ctx->rand_data[]. (I'm renaming it to rand_data_pos.)
- The ctx->I and ctx->last_rand_data buffers, which are completely
unnecessary (and in the latter case, violate anti-backtracking).
- The fact that cprng_init() calls reset_prng_context() with NULL
key and V inputs, and the latter has special-case code to handle
that, when cprng_init() sets PRNG_NEED_RESET, so can just omit
the call entirely.
But there's one thing that I can't figure out, and that is whether
the code is meant to be an implementation of the ANSI X9.17/X9.31 RNG.
It's currently definitely not, because the spec requires periodic input
of a timestamp with some seed entropy, while the code just uses
an incrementing counter.
So I have two paths available:
1. Clarify in comments that, although "Based on" X9.31 Appendix A.2.4,
this is very much NOT an implementation thereof. This is a fully
deterministic PRNG, while the spec is for an RNG.
2. Alternativelt, change the code to actually use high-resolution
timestamps for seed material.
In the latter case, I'd use jiffies and random_get_entropy, and provide
a compatible deterministic option for self-testing. I'd drop the
recommended seedsize to DEFAULT_PRNG_KSZ + DEFAULT_BLK_SZ, but keep the
current implementation's support for an optional starting DT value.
If it isn't provided (the default), the code would generate it fresh
on each call to _get_more_prng_bytes, rather than the current default
to zero.
My problem is I don't know which option is intended. Is it guaranteed that
CRYPTO_ALG_TYPE_RNG is deterministic?
Sorry for the duplicate; I had a crash and I thought the mail was lost.
First message was not quite finished, second is a rewrite from scratch.
On Sat, Nov 29, 2014 at 12:26:49PM -0500, George Spelvin wrote:
> Sorry for the duplicate; I had a crash and I thought the mail was lost.
> First message was not quite finished, second is a rewrite from scratch.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
Responded to your first note
Neil
Thank you all for your hospitality to my amateurish efforts.
Given that this all started with me reading the code in an attempt to
understand it, it is likely that some of the "problems" I address are
actually misunderstandings on my part. If all I get from this patch series
is some enlightenment, that's okay.
It's even more likely that some parts contain the germ of a good idea,
but will need considerable rework to be acceptable. I look forward
to that too.
Expecting that much more work will be needed, I've run the testmgr.h
test vectors on this code, but haven't desk-checked it as thoroughly as
security-sensitive code should be before final acceptance. If the ideas
pass muster, I'll double-check the implementatoin details.
Really, I'm just trying to understand the code. Patches are just
a very specific way of talking about it.
Here's an overview of the series. It's a lot of code cleanup, and
a bit of semantic change.
[01/17] crypto: ansi_cprng - Rename rand_data_valid more sensibly
I find it confusing, so I rename it to rand_data_pos
[02/17] crypto: ansi_cprng - Eliminate ctx->last_rand_data
Shrink the context structure.
[03/17] crypto: ansi_cprng - Eliminate ctx->I
Shrink it some more.
[04/17] crypto: ansi_cprng - simplify xor_vectors() to xor_block()
We don't need a size parameter.
[05/17] crypto: ansi_cprng - Add const annotations to hexdump()
I like const.
[06/17] crypto: ansi_cprng - Eliminate unused PRNG_FIXED_SIZE flag
It's dead code ACAICT.
[07/17] crypto: ansi_cprng - Shrink rand_read_pos & flags
A little more context shrinking.
[08/17] crypto: ansi_cprng - Require non-null key & V in reset_prng_context
Sue to the PRNG_NEED_RESET flag, cprng_init() doesn't need to call
reset_prng_context) directly.
[09/17] crypto: ansi_cprng - Clean up some variable types
Try to be more consistent about signed vs. unsigned.
[10/17] crypto: ansi_cprng - simplify get_prng_bytes
Code shrink & simplification.
[11/17] crypto: ansi_cprng - unroll _get_more_prng_bytes
Slight object code size savings, and definite readability improvement.
[12/17] crypto: ansi_cprng - Create a "block buffer" data type
union { u8 bytes[16]; u32 ints[4]; u64 longs[2]; }, sort of.
[13/17] crypto: ansi_cprng - If DT is not provided, use a fresh timestamp
This is the big semantic change. I'm rather proud of the use
of get_random_int() as a timestamp, but feedback is definitely
solicited!
[14/17] crypto: ansi_cprng - If DT is omitted, don't buffer old output
Not sure if this is desirable or not.
[15/17] crypto: testmgr - Teach test_cprng to handle non-default seed sizes
I consider this a latent bug that patch 17 sould expose.
[16/17] crypto: testmgr - Merge seed arrays in struct cprng_testvec
I think this is a better way of solving the preceding problem,
but it's more intrusive. All the consts I add are not a critical
part of the patch, but an example of what I'd like to do to the
rest of the code.
[17/17] crypto: ansi_cprng - Shrink default seed size
This makes (some amount of) true entropy the default.
It's more like rand_data_invalid (data which has already been output),
so it's a pretty bad misnomer. But rand_data_pos is even better.
Signed-off-by: George Spelvin <[email protected]>
---
crypto/ansi_cprng.c | 25 +++++++++++--------------
1 file changed, 11 insertions(+), 14 deletions(-)
diff --git a/crypto/ansi_cprng.c b/crypto/ansi_cprng.c
index 97fe3110..c9e1684b 100644
--- a/crypto/ansi_cprng.c
+++ b/crypto/ansi_cprng.c
@@ -50,7 +50,7 @@ struct prng_context {
unsigned char DT[DEFAULT_BLK_SZ];
unsigned char I[DEFAULT_BLK_SZ];
unsigned char V[DEFAULT_BLK_SZ];
- u32 rand_data_valid;
+ u32 rand_read_pos; /* Offset into rand_data[] */
struct crypto_cipher *tfm;
u32 flags;
};
@@ -174,7 +174,7 @@ static int _get_more_prng_bytes(struct prng_context *ctx, int cont_test)
}
dbgprint("Returning new block for context %p\n", ctx);
- ctx->rand_data_valid = 0;
+ ctx->rand_read_pos = 0;
hexdump("Output DT: ", ctx->DT, DEFAULT_BLK_SZ);
hexdump("Output I: ", ctx->I, DEFAULT_BLK_SZ);
@@ -217,7 +217,7 @@ static int get_prng_bytes(char *buf, size_t nbytes, struct prng_context *ctx,
remainder:
- if (ctx->rand_data_valid == DEFAULT_BLK_SZ) {
+ if (ctx->rand_read_pos == DEFAULT_BLK_SZ) {
if (_get_more_prng_bytes(ctx, do_cont_test) < 0) {
memset(buf, 0, nbytes);
err = -EINVAL;
@@ -230,12 +230,9 @@ remainder:
*/
if (byte_count < DEFAULT_BLK_SZ) {
empty_rbuf:
- while (ctx->rand_data_valid < DEFAULT_BLK_SZ) {
- *ptr = ctx->rand_data[ctx->rand_data_valid];
- ptr++;
- byte_count--;
- ctx->rand_data_valid++;
- if (byte_count == 0)
+ while (ctx->rand_read_pos < DEFAULT_BLK_SZ) {
+ *ptr++ = ctx->rand_data[ctx->rand_read_pos++];
+ if (--byte_count == 0)
goto done;
}
}
@@ -244,17 +241,17 @@ empty_rbuf:
* Now copy whole blocks
*/
for (; byte_count >= DEFAULT_BLK_SZ; byte_count -= DEFAULT_BLK_SZ) {
- if (ctx->rand_data_valid == DEFAULT_BLK_SZ) {
+ if (ctx->rand_read_pos == DEFAULT_BLK_SZ) {
if (_get_more_prng_bytes(ctx, do_cont_test) < 0) {
memset(buf, 0, nbytes);
err = -EINVAL;
goto done;
}
}
- if (ctx->rand_data_valid > 0)
+ if (ctx->rand_read_pos > 0)
goto empty_rbuf;
memcpy(ptr, ctx->rand_data, DEFAULT_BLK_SZ);
- ctx->rand_data_valid += DEFAULT_BLK_SZ;
+ ctx->rand_read_pos += DEFAULT_BLK_SZ;
ptr += DEFAULT_BLK_SZ;
}
@@ -304,7 +301,7 @@ static int reset_prng_context(struct prng_context *ctx,
memset(ctx->rand_data, 0, DEFAULT_BLK_SZ);
memset(ctx->last_rand_data, 0, DEFAULT_BLK_SZ);
- ctx->rand_data_valid = DEFAULT_BLK_SZ;
+ ctx->rand_read_pos = DEFAULT_BLK_SZ; /* Force immediate refill */
ret = crypto_cipher_setkey(ctx->tfm, prng_key, klen);
if (ret) {
@@ -413,7 +410,7 @@ static int fips_cprng_reset(struct crypto_rng *tfm, u8 *seed, unsigned int slen)
/* this primes our continuity test */
rc = get_prng_bytes(rdata, DEFAULT_BLK_SZ, prng, 0);
- prng->rand_data_valid = DEFAULT_BLK_SZ;
+ prng->rand_read_pos = DEFAULT_BLK_SZ;
out:
return rc;
--
2.1.3
It's simply not necessary.
Signed-off-by: George Spelvin <[email protected]>
---
crypto/ansi_cprng.c | 28 +++++++++++-----------------
1 file changed, 11 insertions(+), 17 deletions(-)
diff --git a/crypto/ansi_cprng.c b/crypto/ansi_cprng.c
index c9e1684b..c0a27288 100644
--- a/crypto/ansi_cprng.c
+++ b/crypto/ansi_cprng.c
@@ -46,7 +46,6 @@
struct prng_context {
spinlock_t prng_lock;
unsigned char rand_data[DEFAULT_BLK_SZ];
- unsigned char last_rand_data[DEFAULT_BLK_SZ];
unsigned char DT[DEFAULT_BLK_SZ];
unsigned char I[DEFAULT_BLK_SZ];
unsigned char V[DEFAULT_BLK_SZ];
@@ -89,8 +88,6 @@ static int _get_more_prng_bytes(struct prng_context *ctx, int cont_test)
{
int i;
unsigned char tmp[DEFAULT_BLK_SZ];
- unsigned char *output = NULL;
-
dbgprint(KERN_CRIT "Calling _get_more_prng_bytes for context %p\n",
ctx);
@@ -103,6 +100,7 @@ static int _get_more_prng_bytes(struct prng_context *ctx, int cont_test)
* This algorithm is a 3 stage state machine
*/
for (i = 0; i < 3; i++) {
+ unsigned char *output;
switch (i) {
case 0:
@@ -115,23 +113,23 @@ static int _get_more_prng_bytes(struct prng_context *ctx, int cont_test)
hexdump("tmp stage 0: ", tmp, DEFAULT_BLK_SZ);
break;
case 1:
-
/*
- * Next xor I with our secret vector V
- * encrypt that result to obtain our
- * pseudo random data which we output
+ * Next xor I with our secret vector V.
+ * Encrypt that result to obtain our pseudo random
+ * data which we output. It is kept temporarily
+ * in (no longer used) V until we have done the
+ * anti-repetition compare.
*/
xor_vectors(ctx->I, ctx->V, tmp, DEFAULT_BLK_SZ);
hexdump("tmp stage 1: ", tmp, DEFAULT_BLK_SZ);
- output = ctx->rand_data;
+ output = ctx->V;
break;
case 2:
/*
* First check that we didn't produce the same
- * random data that we did last time around through this
+ * random data that we did last time around.
*/
- if (!memcmp(ctx->rand_data, ctx->last_rand_data,
- DEFAULT_BLK_SZ)) {
+ if (!memcmp(ctx->V, ctx->rand_data, DEFAULT_BLK_SZ)) {
if (cont_test) {
panic("cprng %p Failed repetition check!\n",
ctx);
@@ -144,15 +142,13 @@ static int _get_more_prng_bytes(struct prng_context *ctx, int cont_test)
ctx->flags |= PRNG_NEED_RESET;
return -EINVAL;
}
- memcpy(ctx->last_rand_data, ctx->rand_data,
- DEFAULT_BLK_SZ);
+ memcpy(ctx->rand_data, ctx->V, DEFAULT_BLK_SZ);
/*
* Lastly xor the random data with I
* and encrypt that to obtain a new secret vector V
*/
- xor_vectors(ctx->rand_data, ctx->I, tmp,
- DEFAULT_BLK_SZ);
+ xor_vectors(ctx->I, ctx->V, tmp, DEFAULT_BLK_SZ);
output = ctx->V;
hexdump("tmp stage 2: ", tmp, DEFAULT_BLK_SZ);
break;
@@ -161,7 +157,6 @@ static int _get_more_prng_bytes(struct prng_context *ctx, int cont_test)
/* do the encryption */
crypto_cipher_encrypt_one(ctx->tfm, output, tmp);
-
}
/*
@@ -299,7 +294,6 @@ static int reset_prng_context(struct prng_context *ctx,
memset(ctx->DT, 0, DEFAULT_BLK_SZ);
memset(ctx->rand_data, 0, DEFAULT_BLK_SZ);
- memset(ctx->last_rand_data, 0, DEFAULT_BLK_SZ);
ctx->rand_read_pos = DEFAULT_BLK_SZ; /* Force immediate refill */
--
2.1.3
It's also not necessary. We do have to change some debugging
output.
Signed-off-by: George Spelvin <[email protected]>
---
crypto/ansi_cprng.c | 39 ++++++++++++++++++++-------------------
1 file changed, 20 insertions(+), 19 deletions(-)
diff --git a/crypto/ansi_cprng.c b/crypto/ansi_cprng.c
index c0a27288..6b844f13 100644
--- a/crypto/ansi_cprng.c
+++ b/crypto/ansi_cprng.c
@@ -35,19 +35,22 @@
#define PRNG_NEED_RESET 0x2
/*
- * Note: DT is our counter value
- * I is our intermediate value
- * V is our seed vector
+ * Note: In addition to the fixed encryption key, there are three
+ * block-sized state buffers:
+ * 1. rand_data is the current output data (R in the spec).
+ * 2. V is our main state vector
+ * 3. DT is the current "data/time" used for seeding. The fact that
+ * this is a deterministic counter rather than an actual timestamp
+ * (with some small amount of seed entropy) means that this code is
+ * NOT an implmentation of X9.31.
+ *
* See http://csrc.nist.gov/groups/STM/cavp/documents/rng/931rngext.pdf
* for implementation details
*/
-
-
struct prng_context {
spinlock_t prng_lock;
unsigned char rand_data[DEFAULT_BLK_SZ];
unsigned char DT[DEFAULT_BLK_SZ];
- unsigned char I[DEFAULT_BLK_SZ];
unsigned char V[DEFAULT_BLK_SZ];
u32 rand_read_pos; /* Offset into rand_data[] */
struct crypto_cipher *tfm;
@@ -93,13 +96,13 @@ static int _get_more_prng_bytes(struct prng_context *ctx, int cont_test)
ctx);
hexdump("Input DT: ", ctx->DT, DEFAULT_BLK_SZ);
- hexdump("Input I: ", ctx->I, DEFAULT_BLK_SZ);
hexdump("Input V: ", ctx->V, DEFAULT_BLK_SZ);
/*
* This algorithm is a 3 stage state machine
*/
for (i = 0; i < 3; i++) {
+ unsigned char const *input;
unsigned char *output;
switch (i) {
@@ -108,9 +111,9 @@ static int _get_more_prng_bytes(struct prng_context *ctx, int cont_test)
* Start by encrypting the counter value
* This gives us an intermediate value I
*/
- memcpy(tmp, ctx->DT, DEFAULT_BLK_SZ);
- output = ctx->I;
- hexdump("tmp stage 0: ", tmp, DEFAULT_BLK_SZ);
+ input = ctx->DT;
+ output = tmp;
+ hexdump("input stage 0: ", ctx->DT, DEFAULT_BLK_SZ);
break;
case 1:
/*
@@ -120,9 +123,9 @@ static int _get_more_prng_bytes(struct prng_context *ctx, int cont_test)
* in (no longer used) V until we have done the
* anti-repetition compare.
*/
- xor_vectors(ctx->I, ctx->V, tmp, DEFAULT_BLK_SZ);
- hexdump("tmp stage 1: ", tmp, DEFAULT_BLK_SZ);
- output = ctx->V;
+ xor_vectors(tmp, ctx->V, ctx->V, DEFAULT_BLK_SZ);
+ hexdump("input stage 1: ", ctx->V, DEFAULT_BLK_SZ);
+ input = output = ctx->V;
break;
case 2:
/*
@@ -148,15 +151,14 @@ static int _get_more_prng_bytes(struct prng_context *ctx, int cont_test)
* Lastly xor the random data with I
* and encrypt that to obtain a new secret vector V
*/
- xor_vectors(ctx->I, ctx->V, tmp, DEFAULT_BLK_SZ);
- output = ctx->V;
- hexdump("tmp stage 2: ", tmp, DEFAULT_BLK_SZ);
+ xor_vectors(tmp, ctx->V, ctx->V, DEFAULT_BLK_SZ);
+ hexdump("input stage 2: ", ctx->V, DEFAULT_BLK_SZ);
+ input = output = ctx->V;
break;
}
-
/* do the encryption */
- crypto_cipher_encrypt_one(ctx->tfm, output, tmp);
+ crypto_cipher_encrypt_one(ctx->tfm, output, input);
}
/*
@@ -172,7 +174,6 @@ static int _get_more_prng_bytes(struct prng_context *ctx, int cont_test)
ctx->rand_read_pos = 0;
hexdump("Output DT: ", ctx->DT, DEFAULT_BLK_SZ);
- hexdump("Output I: ", ctx->I, DEFAULT_BLK_SZ);
hexdump("Output V: ", ctx->V, DEFAULT_BLK_SZ);
hexdump("New Random Data: ", ctx->rand_data, DEFAULT_BLK_SZ);
--
2.1.3
It doesn't need a second input or a length parameter.
Signed-off-by: George Spelvin <[email protected]>
---
crypto/ansi_cprng.c | 12 +++++-------
1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/crypto/ansi_cprng.c b/crypto/ansi_cprng.c
index 6b844f13..4856c84c7 100644
--- a/crypto/ansi_cprng.c
+++ b/crypto/ansi_cprng.c
@@ -74,14 +74,12 @@ if (dbg)\
printk(format, ##args);\
} while (0)
-static void xor_vectors(unsigned char *in1, unsigned char *in2,
- unsigned char *out, unsigned int size)
+static void xor_block(unsigned char const *in, unsigned char *out)
{
int i;
- for (i = 0; i < size; i++)
- out[i] = in1[i] ^ in2[i];
-
+ for (i = 0; i < DEFAULT_BLK_SZ; i++)
+ out[i] ^= in[i];
}
/*
* Returns DEFAULT_BLK_SZ bytes of random data per call
@@ -123,7 +121,7 @@ static int _get_more_prng_bytes(struct prng_context *ctx, int cont_test)
* in (no longer used) V until we have done the
* anti-repetition compare.
*/
- xor_vectors(tmp, ctx->V, ctx->V, DEFAULT_BLK_SZ);
+ xor_block(tmp, ctx->V);
hexdump("input stage 1: ", ctx->V, DEFAULT_BLK_SZ);
input = output = ctx->V;
break;
@@ -151,7 +149,7 @@ static int _get_more_prng_bytes(struct prng_context *ctx, int cont_test)
* Lastly xor the random data with I
* and encrypt that to obtain a new secret vector V
*/
- xor_vectors(tmp, ctx->V, ctx->V, DEFAULT_BLK_SZ);
+ xor_block(tmp, ctx->V);
hexdump("input stage 2: ", ctx->V, DEFAULT_BLK_SZ);
input = output = ctx->V;
break;
--
2.1.3
So I can pass the "input" variable to it.
Signed-off-by: George Spelvin <[email protected]>
---
crypto/ansi_cprng.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
I like to declare things const, and the lack of this in the prototype
causes compiler complaints.
diff --git a/crypto/ansi_cprng.c b/crypto/ansi_cprng.c
index 4856c84c7..1a0ba6a3 100644
--- a/crypto/ansi_cprng.c
+++ b/crypto/ansi_cprng.c
@@ -59,7 +59,7 @@ struct prng_context {
static int dbg;
-static void hexdump(char *note, unsigned char *buf, unsigned int len)
+static void hexdump(char const *note, void const *buf, unsigned int len)
{
if (dbg) {
printk(KERN_CRIT "%s", note);
@@ -111,7 +111,7 @@ static int _get_more_prng_bytes(struct prng_context *ctx, int cont_test)
*/
input = ctx->DT;
output = tmp;
- hexdump("input stage 0: ", ctx->DT, DEFAULT_BLK_SZ);
+ hexdump("input stage 0: ", input, DEFAULT_BLK_SZ);
break;
case 1:
/*
@@ -122,8 +122,8 @@ static int _get_more_prng_bytes(struct prng_context *ctx, int cont_test)
* anti-repetition compare.
*/
xor_block(tmp, ctx->V);
- hexdump("input stage 1: ", ctx->V, DEFAULT_BLK_SZ);
input = output = ctx->V;
+ hexdump("input stage 1: ", input, DEFAULT_BLK_SZ);
break;
case 2:
/*
@@ -150,8 +150,8 @@ static int _get_more_prng_bytes(struct prng_context *ctx, int cont_test)
* and encrypt that to obtain a new secret vector V
*/
xor_block(tmp, ctx->V);
- hexdump("input stage 2: ", ctx->V, DEFAULT_BLK_SZ);
input = output = ctx->V;
+ hexdump("input stage 2: ", input, DEFAULT_BLK_SZ);
break;
}
--
2.1.3
There's no way to set it, so it's dead code.
Signed-off-by: George Spelvin <[email protected]>
---
crypto/ansi_cprng.c | 16 +---------------
1 file changed, 1 insertion(+), 15 deletions(-)
I really hope I'm reading this right!
diff --git a/crypto/ansi_cprng.c b/crypto/ansi_cprng.c
index 1a0ba6a3..93ed00f6 100644
--- a/crypto/ansi_cprng.c
+++ b/crypto/ansi_cprng.c
@@ -31,8 +31,7 @@
* Flags for the prng_context flags field
*/
-#define PRNG_FIXED_SIZE 0x1
-#define PRNG_NEED_RESET 0x2
+#define PRNG_NEED_RESET 0x1
/*
* Note: In addition to the fixed encryption key, there are three
@@ -186,30 +185,17 @@ static int get_prng_bytes(char *buf, size_t nbytes, struct prng_context *ctx,
unsigned int byte_count = (unsigned int)nbytes;
int err;
-
spin_lock_bh(&ctx->prng_lock);
err = -EINVAL;
if (ctx->flags & PRNG_NEED_RESET)
goto done;
- /*
- * If the FIXED_SIZE flag is on, only return whole blocks of
- * pseudo random data
- */
- err = -EINVAL;
- if (ctx->flags & PRNG_FIXED_SIZE) {
- if (nbytes < DEFAULT_BLK_SZ)
- goto done;
- byte_count = DEFAULT_BLK_SZ;
- }
-
err = byte_count;
dbgprint(KERN_CRIT "getting %d random bytes for context %p\n",
byte_count, ctx);
-
remainder:
if (ctx->rand_read_pos == DEFAULT_BLK_SZ) {
if (_get_more_prng_bytes(ctx, do_cont_test) < 0) {
--
2.1.3
Am Dienstag, 2. Dezember 2014, 03:34:41 schrieb George Spelvin:
Hi George,
>It's more like rand_data_invalid (data which has already been output),
>so it's a pretty bad misnomer. But rand_data_pos is even better.
>
>Signed-off-by: George Spelvin <[email protected]>
>---
> crypto/ansi_cprng.c | 25 +++++++++++--------------
> 1 file changed, 11 insertions(+), 14 deletions(-)
>
>diff --git a/crypto/ansi_cprng.c b/crypto/ansi_cprng.c
>index 97fe3110..c9e1684b 100644
>--- a/crypto/ansi_cprng.c
>+++ b/crypto/ansi_cprng.c
>@@ -50,7 +50,7 @@ struct prng_context {
> unsigned char DT[DEFAULT_BLK_SZ];
> unsigned char I[DEFAULT_BLK_SZ];
> unsigned char V[DEFAULT_BLK_SZ];
>- u32 rand_data_valid;
>+ u32 rand_read_pos; /* Offset into rand_data[] */
> struct crypto_cipher *tfm;
> u32 flags;
> };
>@@ -174,7 +174,7 @@ static int _get_more_prng_bytes(struct prng_context
>*ctx, int cont_test) }
>
> dbgprint("Returning new block for context %p\n", ctx);
>- ctx->rand_data_valid = 0;
>+ ctx->rand_read_pos = 0;
>
> hexdump("Output DT: ", ctx->DT, DEFAULT_BLK_SZ);
> hexdump("Output I: ", ctx->I, DEFAULT_BLK_SZ);
>@@ -217,7 +217,7 @@ static int get_prng_bytes(char *buf, size_t nbytes,
>struct prng_context *ctx,
>
>
> remainder:
>- if (ctx->rand_data_valid == DEFAULT_BLK_SZ) {
>+ if (ctx->rand_read_pos == DEFAULT_BLK_SZ) {
> if (_get_more_prng_bytes(ctx, do_cont_test) < 0) {
> memset(buf, 0, nbytes);
> err = -EINVAL;
>@@ -230,12 +230,9 @@ remainder:
> */
> if (byte_count < DEFAULT_BLK_SZ) {
> empty_rbuf:
>- while (ctx->rand_data_valid < DEFAULT_BLK_SZ) {
>- *ptr = ctx->rand_data[ctx->rand_data_valid];
>- ptr++;
>- byte_count--;
>- ctx->rand_data_valid++;
>- if (byte_count == 0)
>+ while (ctx->rand_read_pos < DEFAULT_BLK_SZ) {
>+ *ptr++ = ctx->rand_data[ctx->rand_read_pos++];
>+ if (--byte_count == 0)
> goto done;
I am against such collapsing of constructs into one-liners. It is not
obvious at first sight, which value gets incremented in what order. Such
collapsing was the cause for CVE-2013-4345 as it caused an off-by one.
> }
> }
>@@ -244,17 +241,17 @@ empty_rbuf:
> * Now copy whole blocks
> */
> for (; byte_count >= DEFAULT_BLK_SZ; byte_count -=
DEFAULT_BLK_SZ) {
>- if (ctx->rand_data_valid == DEFAULT_BLK_SZ) {
>+ if (ctx->rand_read_pos == DEFAULT_BLK_SZ) {
> if (_get_more_prng_bytes(ctx, do_cont_test) < 0)
{
> memset(buf, 0, nbytes);
> err = -EINVAL;
> goto done;
> }
> }
>- if (ctx->rand_data_valid > 0)
>+ if (ctx->rand_read_pos > 0)
> goto empty_rbuf;
> memcpy(ptr, ctx->rand_data, DEFAULT_BLK_SZ);
>- ctx->rand_data_valid += DEFAULT_BLK_SZ;
>+ ctx->rand_read_pos += DEFAULT_BLK_SZ;
> ptr += DEFAULT_BLK_SZ;
> }
>
>@@ -304,7 +301,7 @@ static int reset_prng_context(struct prng_context
>*ctx, memset(ctx->rand_data, 0, DEFAULT_BLK_SZ);
> memset(ctx->last_rand_data, 0, DEFAULT_BLK_SZ);
>
>- ctx->rand_data_valid = DEFAULT_BLK_SZ;
>+ ctx->rand_read_pos = DEFAULT_BLK_SZ; /* Force immediate
refill */
>
> ret = crypto_cipher_setkey(ctx->tfm, prng_key, klen);
> if (ret) {
>@@ -413,7 +410,7 @@ static int fips_cprng_reset(struct crypto_rng *tfm,
>u8 *seed, unsigned int slen)
>
> /* this primes our continuity test */
> rc = get_prng_bytes(rdata, DEFAULT_BLK_SZ, prng, 0);
>- prng->rand_data_valid = DEFAULT_BLK_SZ;
>+ prng->rand_read_pos = DEFAULT_BLK_SZ;
>
> out:
> return rc;
Ciao
Stephan
rand_read_pos is never more than 16, while there's only 1 flag
bit allocated, so we can shrink the context a little.
Signed-off-by: George Spelvin <[email protected]>
---
crypto/ansi_cprng.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
They're also reordered to avoid alignment holes.
diff --git a/crypto/ansi_cprng.c b/crypto/ansi_cprng.c
index 93ed00f6..f40f54cd 100644
--- a/crypto/ansi_cprng.c
+++ b/crypto/ansi_cprng.c
@@ -51,9 +51,9 @@ struct prng_context {
unsigned char rand_data[DEFAULT_BLK_SZ];
unsigned char DT[DEFAULT_BLK_SZ];
unsigned char V[DEFAULT_BLK_SZ];
- u32 rand_read_pos; /* Offset into rand_data[] */
+ unsigned char rand_read_pos; /* Offset into rand_data[] */
+ unsigned char flags;
struct crypto_cipher *tfm;
- u32 flags;
};
static int dbg;
--
2.1.3
The PRNG_NEED_RESET flag forces a call to reset_prng_context(), so there's
no need to include one in cprng_init() at all. That allows considerable
simplification of reset_prng_context().
Signed-off-by: George Spelvin <[email protected]>
---
crypto/ansi_cprng.c | 34 ++++++++--------------------------
1 file changed, 8 insertions(+), 26 deletions(-)
I'm worried someone may seriously object to leaving part of the
context uninitialized, but it definitely simplifies the code.
I'm quite interested in comments.
diff --git a/crypto/ansi_cprng.c b/crypto/ansi_cprng.c
index f40f54cd..dff27a7a 100644
--- a/crypto/ansi_cprng.c
+++ b/crypto/ansi_cprng.c
@@ -22,10 +22,8 @@
#include "internal.h"
-#define DEFAULT_PRNG_KEY "0123456789abcdef"
#define DEFAULT_PRNG_KSZ 16
#define DEFAULT_BLK_SZ 16
-#define DEFAULT_V_SEED "zaybxcwdveuftgsh"
/*
* Flags for the prng_context flags field
@@ -254,24 +252,15 @@ static void free_prng_context(struct prng_context *ctx)
}
static int reset_prng_context(struct prng_context *ctx,
- unsigned char *key, size_t klen,
- unsigned char *V, unsigned char *DT)
+ unsigned char const *key, size_t klen,
+ unsigned char const *V, unsigned char const *DT)
{
int ret;
- unsigned char *prng_key;
spin_lock_bh(&ctx->prng_lock);
ctx->flags |= PRNG_NEED_RESET;
- prng_key = (key != NULL) ? key : (unsigned char *)DEFAULT_PRNG_KEY;
-
- if (!key)
- klen = DEFAULT_PRNG_KSZ;
-
- if (V)
- memcpy(ctx->V, V, DEFAULT_BLK_SZ);
- else
- memcpy(ctx->V, DEFAULT_V_SEED, DEFAULT_BLK_SZ);
+ memcpy(ctx->V, V, DEFAULT_BLK_SZ);
if (DT)
memcpy(ctx->DT, DT, DEFAULT_BLK_SZ);
@@ -282,16 +271,13 @@ static int reset_prng_context(struct prng_context *ctx,
ctx->rand_read_pos = DEFAULT_BLK_SZ; /* Force immediate refill */
- ret = crypto_cipher_setkey(ctx->tfm, prng_key, klen);
+ ret = crypto_cipher_setkey(ctx->tfm, key, klen);
if (ret) {
dbgprint(KERN_CRIT "PRNG: setkey() failed flags=%x\n",
crypto_cipher_get_flags(ctx->tfm));
- goto out;
+ } else {
+ ctx->flags &= ~PRNG_NEED_RESET;
}
-
- ret = 0;
- ctx->flags &= ~PRNG_NEED_RESET;
-out:
spin_unlock_bh(&ctx->prng_lock);
return ret;
}
@@ -308,13 +294,9 @@ static int cprng_init(struct crypto_tfm *tfm)
return PTR_ERR(ctx->tfm);
}
- if (reset_prng_context(ctx, NULL, DEFAULT_PRNG_KSZ, NULL, NULL) < 0)
- return -EINVAL;
-
/*
- * after allocation, we should always force the user to reset
- * so they don't inadvertently use the insecure default values
- * without specifying them intentially
+ * After allocation, we always force the user to reset, which
+ * completes initialization of the context.
*/
ctx->flags |= PRNG_NEED_RESET;
return 0;
--
2.1.3
Add a few const annotations, use unsigned bytes consistently, and
make do_cont_test a bool.
Signed-off-by: George Spelvin <[email protected]>
---
crypto/ansi_cprng.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/crypto/ansi_cprng.c b/crypto/ansi_cprng.c
index dff27a7a..6723a561 100644
--- a/crypto/ansi_cprng.c
+++ b/crypto/ansi_cprng.c
@@ -82,7 +82,7 @@ static void xor_block(unsigned char const *in, unsigned char *out)
* Returns DEFAULT_BLK_SZ bytes of random data per call
* returns 0 if generation succeeded, <0 if something went wrong
*/
-static int _get_more_prng_bytes(struct prng_context *ctx, int cont_test)
+static int _get_more_prng_bytes(struct prng_context *ctx, bool cont_test)
{
int i;
unsigned char tmp[DEFAULT_BLK_SZ];
@@ -176,8 +176,8 @@ static int _get_more_prng_bytes(struct prng_context *ctx, int cont_test)
}
/* Our exported functions */
-static int get_prng_bytes(char *buf, size_t nbytes, struct prng_context *ctx,
- int do_cont_test)
+static int get_prng_bytes(unsigned char *buf, unsigned int nbytes,
+ struct prng_context *ctx, bool do_cont_test)
{
unsigned char *ptr = buf;
unsigned int byte_count = (unsigned int)nbytes;
@@ -312,7 +312,7 @@ static int cprng_get_random(struct crypto_rng *tfm, u8 *rdata,
{
struct prng_context *prng = crypto_rng_ctx(tfm);
- return get_prng_bytes(rdata, dlen, prng, 0);
+ return get_prng_bytes(rdata, dlen, prng, false);
}
/*
@@ -324,8 +324,8 @@ static int cprng_get_random(struct crypto_rng *tfm, u8 *rdata,
static int cprng_reset(struct crypto_rng *tfm, u8 *seed, unsigned int slen)
{
struct prng_context *prng = crypto_rng_ctx(tfm);
- u8 *key = seed + DEFAULT_BLK_SZ;
- u8 *dt = NULL;
+ u8 const *key = seed + DEFAULT_BLK_SZ;
+ u8 const *dt = NULL;
if (slen < DEFAULT_PRNG_KSZ + DEFAULT_BLK_SZ)
return -EINVAL;
@@ -346,13 +346,13 @@ static int fips_cprng_get_random(struct crypto_rng *tfm, u8 *rdata,
{
struct prng_context *prng = crypto_rng_ctx(tfm);
- return get_prng_bytes(rdata, dlen, prng, 1);
+ return get_prng_bytes(rdata, dlen, prng, true);
}
static int fips_cprng_reset(struct crypto_rng *tfm, u8 *seed, unsigned int slen)
{
u8 rdata[DEFAULT_BLK_SZ];
- u8 *key = seed + DEFAULT_BLK_SZ;
+ u8 const *key = seed + DEFAULT_BLK_SZ;
int rc;
struct prng_context *prng = crypto_rng_ctx(tfm);
--
2.1.3
The crypto is so slow that there's no point unrolling this function.
A simpler and clearer implementation will do just fine.
Also move all modification of rand_read_pos out of _get_more_prng_bytes;
that's variable belongs to the byte-at-a-time layer outside the
block-oriented primitive.
Signed-off-by: George Spelvin <[email protected]>
---
crypto/ansi_cprng.c | 67 ++++++++++++++---------------------------------------
1 file changed, 18 insertions(+), 49 deletions(-)
Friends don't let friends micro-optimize non-inner loops.
diff --git a/crypto/ansi_cprng.c b/crypto/ansi_cprng.c
index 6723a561..de13e741 100644
--- a/crypto/ansi_cprng.c
+++ b/crypto/ansi_cprng.c
@@ -166,7 +166,6 @@ static int _get_more_prng_bytes(struct prng_context *ctx, bool cont_test)
}
dbgprint("Returning new block for context %p\n", ctx);
- ctx->rand_read_pos = 0;
hexdump("Output DT: ", ctx->DT, DEFAULT_BLK_SZ);
hexdump("Output V: ", ctx->V, DEFAULT_BLK_SZ);
@@ -179,65 +178,36 @@ static int _get_more_prng_bytes(struct prng_context *ctx, bool cont_test)
static int get_prng_bytes(unsigned char *buf, unsigned int nbytes,
struct prng_context *ctx, bool do_cont_test)
{
- unsigned char *ptr = buf;
- unsigned int byte_count = (unsigned int)nbytes;
- int err;
+ unsigned int pos = 0;
+ unsigned int len;
+ int read_pos = ctx->rand_read_pos;
+ int err = -EINVAL;
+
+ dbgprint(KERN_CRIT "getting %u random bytes for context %p\n",
+ nbytes, ctx);
spin_lock_bh(&ctx->prng_lock);
- err = -EINVAL;
if (ctx->flags & PRNG_NEED_RESET)
goto done;
- err = byte_count;
+ while (nbytes - pos > DEFAULT_BLK_SZ - read_pos) {
+ len = DEFAULT_BLK_SZ - read_pos;
- dbgprint(KERN_CRIT "getting %d random bytes for context %p\n",
- byte_count, ctx);
-
-remainder:
- if (ctx->rand_read_pos == DEFAULT_BLK_SZ) {
+ memcpy(buf + pos, ctx->rand_data + read_pos, len);
if (_get_more_prng_bytes(ctx, do_cont_test) < 0) {
memset(buf, 0, nbytes);
- err = -EINVAL;
goto done;
}
+ pos += len;
+ read_pos = 0;
}
- /*
- * Copy any data less than an entire block
- */
- if (byte_count < DEFAULT_BLK_SZ) {
-empty_rbuf:
- while (ctx->rand_read_pos < DEFAULT_BLK_SZ) {
- *ptr++ = ctx->rand_data[ctx->rand_read_pos++];
- if (--byte_count == 0)
- goto done;
- }
- }
-
- /*
- * Now copy whole blocks
- */
- for (; byte_count >= DEFAULT_BLK_SZ; byte_count -= DEFAULT_BLK_SZ) {
- if (ctx->rand_read_pos == DEFAULT_BLK_SZ) {
- if (_get_more_prng_bytes(ctx, do_cont_test) < 0) {
- memset(buf, 0, nbytes);
- err = -EINVAL;
- goto done;
- }
- }
- if (ctx->rand_read_pos > 0)
- goto empty_rbuf;
- memcpy(ptr, ctx->rand_data, DEFAULT_BLK_SZ);
- ctx->rand_read_pos += DEFAULT_BLK_SZ;
- ptr += DEFAULT_BLK_SZ;
- }
-
- /*
- * Now go back and get any remaining partial block
- */
- if (byte_count)
- goto remainder;
+ /* The final partial block */
+ len = nbytes - pos;
+ memcpy(buf + pos, ctx->rand_data + read_pos, len);
+ ctx->rand_read_pos = read_pos + len;
+ err = nbytes;
done:
spin_unlock_bh(&ctx->prng_lock);
@@ -351,7 +321,6 @@ static int fips_cprng_get_random(struct crypto_rng *tfm, u8 *rdata,
static int fips_cprng_reset(struct crypto_rng *tfm, u8 *seed, unsigned int slen)
{
- u8 rdata[DEFAULT_BLK_SZ];
u8 const *key = seed + DEFAULT_BLK_SZ;
int rc;
@@ -370,7 +339,7 @@ static int fips_cprng_reset(struct crypto_rng *tfm, u8 *seed, unsigned int slen)
goto out;
/* this primes our continuity test */
- rc = get_prng_bytes(rdata, DEFAULT_BLK_SZ, prng, 0);
+ rc = _get_more_prng_bytes(prng, false);
prng->rand_read_pos = DEFAULT_BLK_SZ;
out:
--
2.1.3
It's more legible, and the code is 15 bytes smaller (i386).
Signed-off-by: George Spelvin <[email protected]>
---
crypto/ansi_cprng.c | 87 ++++++++++++++++++++---------------------------------
1 file changed, 32 insertions(+), 55 deletions(-)
I'm not really sure why this was implemented this convoluted way
in the first place. Did crypto_cipher_encrypt_one() used to be
an enormous inline function?
diff --git a/crypto/ansi_cprng.c b/crypto/ansi_cprng.c
index de13e741..09bb1252 100644
--- a/crypto/ansi_cprng.c
+++ b/crypto/ansi_cprng.c
@@ -94,67 +94,44 @@ static int _get_more_prng_bytes(struct prng_context *ctx, bool cont_test)
hexdump("Input V: ", ctx->V, DEFAULT_BLK_SZ);
/*
- * This algorithm is a 3 stage state machine
+ * Start by encrypting the counter value.
+ * This gives us an intermediate value I.
*/
- for (i = 0; i < 3; i++) {
- unsigned char const *input;
- unsigned char *output;
+ crypto_cipher_encrypt_one(ctx->tfm, tmp, ctx->DT);
+ hexdump("input I: ", tmp, DEFAULT_BLK_SZ);
- switch (i) {
- case 0:
- /*
- * Start by encrypting the counter value
- * This gives us an intermediate value I
- */
- input = ctx->DT;
- output = tmp;
- hexdump("input stage 0: ", input, DEFAULT_BLK_SZ);
- break;
- case 1:
- /*
- * Next xor I with our secret vector V.
- * Encrypt that result to obtain our pseudo random
- * data which we output. It is kept temporarily
- * in (no longer used) V until we have done the
- * anti-repetition compare.
- */
- xor_block(tmp, ctx->V);
- input = output = ctx->V;
- hexdump("input stage 1: ", input, DEFAULT_BLK_SZ);
- break;
- case 2:
- /*
- * First check that we didn't produce the same
- * random data that we did last time around.
- */
- if (!memcmp(ctx->V, ctx->rand_data, DEFAULT_BLK_SZ)) {
- if (cont_test) {
- panic("cprng %p Failed repetition check!\n",
- ctx);
- }
-
- printk(KERN_ERR
- "ctx %p Failed repetition check!\n",
- ctx);
-
- ctx->flags |= PRNG_NEED_RESET;
- return -EINVAL;
- }
- memcpy(ctx->rand_data, ctx->V, DEFAULT_BLK_SZ);
+ /*
+ * Next xor I with our secret vector V.
+ * Encrypt that result to obtain our pseudo random data which
+ * we output. It is kept temporarily in (no longer used)
+ * V until we have done the anti-repetition compare.
+ */
+ xor_block(tmp, ctx->V);
+ hexdump("input stage 1: ", ctx->V, DEFAULT_BLK_SZ);
+ crypto_cipher_encrypt_one(ctx->tfm, ctx->V, ctx->V);
- /*
- * Lastly xor the random data with I
- * and encrypt that to obtain a new secret vector V
- */
- xor_block(tmp, ctx->V);
- input = output = ctx->V;
- hexdump("input stage 2: ", input, DEFAULT_BLK_SZ);
- break;
+ /*
+ * Check that we didn't produce the same random data
+ * that we did last time around.
+ */
+ if (!memcmp(ctx->V, ctx->rand_data, DEFAULT_BLK_SZ)) {
+ if (cont_test) {
+ panic("cprng %p Failed repetition check!\n", ctx);
}
- /* do the encryption */
- crypto_cipher_encrypt_one(ctx->tfm, output, input);
+ printk(KERN_ERR "ctx %p Failed repetition check!\n", ctx);
+ ctx->flags |= PRNG_NEED_RESET;
+ return -EINVAL;
}
+ memcpy(ctx->rand_data, ctx->V, DEFAULT_BLK_SZ);
+
+ /*
+ * Lastly xor the random data with I and encrypt that to
+ * obtain a new secret vector V
+ */
+ xor_block(tmp, ctx->V);
+ hexdump("input stage 2: ", ctx->V, DEFAULT_BLK_SZ);
+ crypto_cipher_encrypt_one(ctx->tfm, ctx->V, ctx->V);
/*
* Now update our DT value
--
2.1.3
It's just a union of various word sizes to address the buffer.
This achieves three things:
1) Aligns the buffers for (hopefully) slight performance benefit
2) Allows XOR to be done long-at-a-time
3) Prepares for later patches where I want int-at-a-time access
Signed-off-by: George Spelvin <[email protected]>
---
crypto/ansi_cprng.c | 78 ++++++++++++++++++++++++++++++-----------------------
1 file changed, 44 insertions(+), 34 deletions(-)
This is the sort of style issue I fear will attract loud screams.
diff --git a/crypto/ansi_cprng.c b/crypto/ansi_cprng.c
index 09bb1252..7b6b263d 100644
--- a/crypto/ansi_cprng.c
+++ b/crypto/ansi_cprng.c
@@ -26,6 +26,19 @@
#define DEFAULT_BLK_SZ 16
/*
+ * A cipher block is defined as a union, so we can address individual bytes,
+ * or do things word-at-a-time when byte order doesn't matter, such as XOR
+ * or adding in entropy.
+ */
+#define BLK_INTS ((DEFAULT_BLK_SZ-1) / sizeof(int) + 1)
+#define BLK_LONGS ((DEFAULT_BLK_SZ-1) / sizeof(long) + 1)
+union block {
+ unsigned char bytes[DEFAULT_BLK_SZ];
+ unsigned long ints[BLK_INTS];
+ unsigned long longs[BLK_LONGS];
+};
+
+/*
* Flags for the prng_context flags field
*/
@@ -46,23 +59,20 @@
*/
struct prng_context {
spinlock_t prng_lock;
- unsigned char rand_data[DEFAULT_BLK_SZ];
- unsigned char DT[DEFAULT_BLK_SZ];
- unsigned char V[DEFAULT_BLK_SZ];
- unsigned char rand_read_pos; /* Offset into rand_data[] */
+ unsigned char rand_read_pos; /* Offset into rand_data.bytes[] */
unsigned char flags;
+ union block rand_data, DT, V;
struct crypto_cipher *tfm;
};
static int dbg;
-static void hexdump(char const *note, void const *buf, unsigned int len)
+static void hexdump(char const *note, union block const *block)
{
if (dbg) {
printk(KERN_CRIT "%s", note);
print_hex_dump(KERN_CONT, "", DUMP_PREFIX_OFFSET,
- 16, 1,
- buf, len, false);
+ 16, 1, block->bytes, DEFAULT_BLK_SZ, false);
}
}
@@ -71,12 +81,12 @@ if (dbg)\
printk(format, ##args);\
} while (0)
-static void xor_block(unsigned char const *in, unsigned char *out)
+static void xor_block(union block const *in, union block *out)
{
int i;
- for (i = 0; i < DEFAULT_BLK_SZ; i++)
- out[i] ^= in[i];
+ for (i = 0; i < BLK_LONGS; i++)
+ out->longs[i] ^= in->longs[i];
}
/*
* Returns DEFAULT_BLK_SZ bytes of random data per call
@@ -85,20 +95,20 @@ static void xor_block(unsigned char const *in, unsigned char *out)
static int _get_more_prng_bytes(struct prng_context *ctx, bool cont_test)
{
int i;
- unsigned char tmp[DEFAULT_BLK_SZ];
+ union block tmp;
dbgprint(KERN_CRIT "Calling _get_more_prng_bytes for context %p\n",
ctx);
- hexdump("Input DT: ", ctx->DT, DEFAULT_BLK_SZ);
- hexdump("Input V: ", ctx->V, DEFAULT_BLK_SZ);
+ hexdump("Input DT: ", &ctx->DT);
+ hexdump("Input V: ", &ctx->V);
/*
* Start by encrypting the counter value.
* This gives us an intermediate value I.
*/
- crypto_cipher_encrypt_one(ctx->tfm, tmp, ctx->DT);
- hexdump("input I: ", tmp, DEFAULT_BLK_SZ);
+ crypto_cipher_encrypt_one(ctx->tfm, tmp.bytes, ctx->DT.bytes);
+ hexdump("input I: ", &tmp);
/*
* Next xor I with our secret vector V.
@@ -106,15 +116,15 @@ static int _get_more_prng_bytes(struct prng_context *ctx, bool cont_test)
* we output. It is kept temporarily in (no longer used)
* V until we have done the anti-repetition compare.
*/
- xor_block(tmp, ctx->V);
- hexdump("input stage 1: ", ctx->V, DEFAULT_BLK_SZ);
- crypto_cipher_encrypt_one(ctx->tfm, ctx->V, ctx->V);
+ xor_block(&tmp, &ctx->V);
+ hexdump("input stage 1: ", &ctx->V);
+ crypto_cipher_encrypt_one(ctx->tfm, ctx->V.bytes, ctx->V.bytes);
/*
* Check that we didn't produce the same random data
* that we did last time around.
*/
- if (!memcmp(ctx->V, ctx->rand_data, DEFAULT_BLK_SZ)) {
+ if (!memcmp(ctx->V.bytes, ctx->rand_data.bytes, DEFAULT_BLK_SZ)) {
if (cont_test) {
panic("cprng %p Failed repetition check!\n", ctx);
}
@@ -123,30 +133,30 @@ static int _get_more_prng_bytes(struct prng_context *ctx, bool cont_test)
ctx->flags |= PRNG_NEED_RESET;
return -EINVAL;
}
- memcpy(ctx->rand_data, ctx->V, DEFAULT_BLK_SZ);
+ ctx->rand_data = ctx->V;
/*
* Lastly xor the random data with I and encrypt that to
* obtain a new secret vector V
*/
- xor_block(tmp, ctx->V);
- hexdump("input stage 2: ", ctx->V, DEFAULT_BLK_SZ);
- crypto_cipher_encrypt_one(ctx->tfm, ctx->V, ctx->V);
+ xor_block(&tmp, &ctx->V);
+ hexdump("input stage 2: ", &ctx->V);
+ crypto_cipher_encrypt_one(ctx->tfm, ctx->V.bytes, ctx->V.bytes);
/*
* Now update our DT value
*/
for (i = DEFAULT_BLK_SZ - 1; i >= 0; i--) {
- ctx->DT[i] += 1;
- if (ctx->DT[i] != 0)
+ ctx->DT.bytes[i] += 1;
+ if (ctx->DT.bytes[i] != 0)
break;
}
dbgprint("Returning new block for context %p\n", ctx);
- hexdump("Output DT: ", ctx->DT, DEFAULT_BLK_SZ);
- hexdump("Output V: ", ctx->V, DEFAULT_BLK_SZ);
- hexdump("New Random Data: ", ctx->rand_data, DEFAULT_BLK_SZ);
+ hexdump("Output DT: ", &ctx->DT);
+ hexdump("Output V: ", &ctx->V);
+ hexdump("New Random Data: ", &ctx->rand_data);
return 0;
}
@@ -171,7 +181,7 @@ static int get_prng_bytes(unsigned char *buf, unsigned int nbytes,
while (nbytes - pos > DEFAULT_BLK_SZ - read_pos) {
len = DEFAULT_BLK_SZ - read_pos;
- memcpy(buf + pos, ctx->rand_data + read_pos, len);
+ memcpy(buf + pos, ctx->rand_data.bytes + read_pos, len);
if (_get_more_prng_bytes(ctx, do_cont_test) < 0) {
memset(buf, 0, nbytes);
goto done;
@@ -182,7 +192,7 @@ static int get_prng_bytes(unsigned char *buf, unsigned int nbytes,
/* The final partial block */
len = nbytes - pos;
- memcpy(buf + pos, ctx->rand_data + read_pos, len);
+ memcpy(buf + pos, ctx->rand_data.bytes + read_pos, len);
ctx->rand_read_pos = read_pos + len;
err = nbytes;
@@ -207,14 +217,14 @@ static int reset_prng_context(struct prng_context *ctx,
spin_lock_bh(&ctx->prng_lock);
ctx->flags |= PRNG_NEED_RESET;
- memcpy(ctx->V, V, DEFAULT_BLK_SZ);
+ memcpy(ctx->V.bytes, V, DEFAULT_BLK_SZ);
if (DT)
- memcpy(ctx->DT, DT, DEFAULT_BLK_SZ);
+ memcpy(ctx->DT.bytes, DT, DEFAULT_BLK_SZ);
else
- memset(ctx->DT, 0, DEFAULT_BLK_SZ);
+ memset(ctx->DT.bytes, 0, DEFAULT_BLK_SZ);
- memset(ctx->rand_data, 0, DEFAULT_BLK_SZ);
+ memset(ctx->rand_data.bytes, 0, DEFAULT_BLK_SZ);
ctx->rand_read_pos = DEFAULT_BLK_SZ; /* Force immediate refill */
--
2.1.3
Except for the switch from triple DES to AES-128, this results in an
actual implementation of the X9.31 Appendix A.2.4 generator, which is
the same as the X9.17 Appendix C one.
If a DT seed value is provided, it is the same fully deterministic
algorithm it has always been. If DT is omitted (something already
allowed), it is generated fresh each time.
Signed-off-by: George Spelvin <[email protected]>
---
crypto/ansi_cprng.c | 67 +++++++++++++++++++++++++++++++++++++++--------------
1 file changed, 50 insertions(+), 17 deletions(-)
diff --git a/crypto/ansi_cprng.c b/crypto/ansi_cprng.c
index 7b6b263d..c2c285f3 100644
--- a/crypto/ansi_cprng.c
+++ b/crypto/ansi_cprng.c
@@ -1,7 +1,11 @@
/*
* PRNG: Pseudo Random Number Generator
- * Based on NIST Recommended PRNG From ANSI X9.31 Appendix A.2.4 using
- * AES 128 cipher
+ * Based on "NIST Recommended PRNG From ANSI X9.31 Appendix A.2.4"
+ * using AES 128 cipher. It may be seeded with a fixed DT value
+ * for determinsitic generaton purposes, or if no DT value is
+ * provided for seed material, it generates a fresh one each time
+ * using a high-resolution timestamp, as specified in the X9.17
+ * and X9.31 standards.
*
* (C) Neil Horman <[email protected]>
*
@@ -9,8 +13,6 @@
* under the terms of the GNU General Public License as published by the
* Free Software Foundation; either version 2 of the License, or (at your
* any later version.
- *
- *
*/
#include <crypto/internal/rng.h>
@@ -41,8 +43,8 @@ union block {
/*
* Flags for the prng_context flags field
*/
-
-#define PRNG_NEED_RESET 0x1
+#define PRNG_NEED_RESET 0x1
+#define PRNG_DETERMINISTIC 0x2
/*
* Note: In addition to the fixed encryption key, there are three
@@ -104,6 +106,16 @@ static int _get_more_prng_bytes(struct prng_context *ctx, bool cont_test)
hexdump("Input V: ", &ctx->V);
/*
+ * get_random_int produces a result based on the system jiffies
+ * and random_get_entropy(), the highest-resolution timestamp
+ * available. This meets the spirit of the X9.17/X9.31 generation
+ * specifications, but it's masked by hashing, so it can't be used
+ * to leak information about the seed to /dev/random.
+ */
+ if (!(ctx->flags & PRNG_DETERMINISTIC))
+ ctx->DT.ints[0] = get_random_int();
+
+ /*
* Start by encrypting the counter value.
* This gives us an intermediate value I.
*/
@@ -144,12 +156,19 @@ static int _get_more_prng_bytes(struct prng_context *ctx, bool cont_test)
crypto_cipher_encrypt_one(ctx->tfm, ctx->V.bytes, ctx->V.bytes);
/*
- * Now update our DT value
+ * Now update our DT value.
*/
- for (i = DEFAULT_BLK_SZ - 1; i >= 0; i--) {
- ctx->DT.bytes[i] += 1;
- if (ctx->DT.bytes[i] != 0)
- break;
+ if (ctx->flags & PRNG_DETERMINISTIC) {
+ /* The big-endian byte order matches the NIST test vectors */
+ for (i = DEFAULT_BLK_SZ - 1; i >= 0; i--) {
+ ctx->DT.bytes[i] += 1;
+ if (ctx->DT.bytes[i] != 0)
+ break;
+ }
+ } else {
+ /* Prevent backtracking */
+ ctx->DT.ints[0] = 0; /* Prevent backtracking */
+ memzero_explicit(tmp.bytes, DEFAULT_BLK_SZ);
}
dbgprint("Returning new block for context %p\n", ctx);
@@ -193,7 +212,9 @@ static int get_prng_bytes(unsigned char *buf, unsigned int nbytes,
/* The final partial block */
len = nbytes - pos;
memcpy(buf + pos, ctx->rand_data.bytes + read_pos, len);
- ctx->rand_read_pos = read_pos + len;
+ read_pos += len;
+ memzero_explicit(ctx->rand_data.bytes, read_pos);
+ ctx->rand_read_pos = read_pos;
err = nbytes;
done:
@@ -215,14 +236,26 @@ static int reset_prng_context(struct prng_context *ctx,
int ret;
spin_lock_bh(&ctx->prng_lock);
- ctx->flags |= PRNG_NEED_RESET;
+ ctx->flags = PRNG_NEED_RESET;
memcpy(ctx->V.bytes, V, DEFAULT_BLK_SZ);
- if (DT)
+ if (DT) {
+ /* Predictable DT, when repeatability is desired */
+ ctx->flags |= PRNG_DETERMINISTIC;
memcpy(ctx->DT.bytes, DT, DEFAULT_BLK_SZ);
- else
- memset(ctx->DT.bytes, 0, DEFAULT_BLK_SZ);
+ } else {
+ int i;
+ /*
+ * Generate a fresh DT based on timestamp each time.
+ * Also pad rest of buffer with seed, on general principles.
+ * We reserve the first int for fresh entropy, in case
+ * the key is not an even multiple of an int in size and
+ * the last int is partially ignored.
+ */
+ for (i = 1; i < BLK_INTS; i++)
+ ctx->DT.ints[i] = get_random_int();
+ }
memset(ctx->rand_data.bytes, 0, DEFAULT_BLK_SZ);
@@ -255,7 +288,7 @@ static int cprng_init(struct crypto_tfm *tfm)
* After allocation, we always force the user to reset, which
* completes initialization of the context.
*/
- ctx->flags |= PRNG_NEED_RESET;
+ ctx->flags = PRNG_NEED_RESET;
return 0;
}
--
2.1.3
Am Dienstag, 2. Dezember 2014, 03:35:50 schrieb George Spelvin:
Hi George,
>It's simply not necessary.
Can you please be a bit more verbose on why you think this is not
necessary?
Have you tested that change with reference test vectors -- what do
testmgr test vectors say?
>
>Signed-off-by: George Spelvin <[email protected]>
>---
> crypto/ansi_cprng.c | 28 +++++++++++-----------------
> 1 file changed, 11 insertions(+), 17 deletions(-)
>
>diff --git a/crypto/ansi_cprng.c b/crypto/ansi_cprng.c
>index c9e1684b..c0a27288 100644
>--- a/crypto/ansi_cprng.c
>+++ b/crypto/ansi_cprng.c
>@@ -46,7 +46,6 @@
> struct prng_context {
> spinlock_t prng_lock;
> unsigned char rand_data[DEFAULT_BLK_SZ];
>- unsigned char last_rand_data[DEFAULT_BLK_SZ];
> unsigned char DT[DEFAULT_BLK_SZ];
> unsigned char I[DEFAULT_BLK_SZ];
> unsigned char V[DEFAULT_BLK_SZ];
>@@ -89,8 +88,6 @@ static int _get_more_prng_bytes(struct prng_context
>*ctx, int cont_test) {
> int i;
> unsigned char tmp[DEFAULT_BLK_SZ];
>- unsigned char *output = NULL;
>-
>
> dbgprint(KERN_CRIT "Calling _get_more_prng_bytes for context
%p\n",
> ctx);
>@@ -103,6 +100,7 @@ static int _get_more_prng_bytes(struct prng_context
>*ctx, int cont_test) * This algorithm is a 3 stage state machine
> */
> for (i = 0; i < 3; i++) {
>+ unsigned char *output;
>
> switch (i) {
> case 0:
>@@ -115,23 +113,23 @@ static int _get_more_prng_bytes(struct
>prng_context *ctx, int cont_test) hexdump("tmp stage 0: ", tmp,
>DEFAULT_BLK_SZ);
> break;
> case 1:
>-
> /*
>- * Next xor I with our secret vector V
>- * encrypt that result to obtain our
>- * pseudo random data which we output
>+ * Next xor I with our secret vector V.
>+ * Encrypt that result to obtain our pseudo
random
>+ * data which we output. It is kept temporarily
>+ * in (no longer used) V until we have done the
>+ * anti-repetition compare.
> */
> xor_vectors(ctx->I, ctx->V, tmp,
DEFAULT_BLK_SZ);
> hexdump("tmp stage 1: ", tmp, DEFAULT_BLK_SZ);
>- output = ctx->rand_data;
>+ output = ctx->V;
> break;
> case 2:
> /*
> * First check that we didn't produce the same
>- * random data that we did last time around
through this
>+ * random data that we did last time around.
> */
>- if (!memcmp(ctx->rand_data, ctx->last_rand_data,
>- DEFAULT_BLK_SZ)) {
>+ if (!memcmp(ctx->V, ctx->rand_data,
DEFAULT_BLK_SZ)) {
> if (cont_test) {
> panic("cprng %p Failed
repetition check!\n",
> ctx);
>@@ -144,15 +142,13 @@ static int _get_more_prng_bytes(struct
>prng_context *ctx, int cont_test) ctx->flags |= PRNG_NEED_RESET;
> return -EINVAL;
> }
>- memcpy(ctx->last_rand_data, ctx->rand_data,
>- DEFAULT_BLK_SZ);
>+ memcpy(ctx->rand_data, ctx->V, DEFAULT_BLK_SZ);
>
> /*
> * Lastly xor the random data with I
> * and encrypt that to obtain a new secret
vector V
> */
>- xor_vectors(ctx->rand_data, ctx->I, tmp,
>- DEFAULT_BLK_SZ);
>+ xor_vectors(ctx->I, ctx->V, tmp,
DEFAULT_BLK_SZ);
> output = ctx->V;
> hexdump("tmp stage 2: ", tmp, DEFAULT_BLK_SZ);
> break;
>@@ -161,7 +157,6 @@ static int _get_more_prng_bytes(struct prng_context
>*ctx, int cont_test)
>
> /* do the encryption */
> crypto_cipher_encrypt_one(ctx->tfm, output, tmp);
>-
> }
>
> /*
>@@ -299,7 +294,6 @@ static int reset_prng_context(struct prng_context
>*ctx, memset(ctx->DT, 0, DEFAULT_BLK_SZ);
>
> memset(ctx->rand_data, 0, DEFAULT_BLK_SZ);
>- memset(ctx->last_rand_data, 0, DEFAULT_BLK_SZ);
>
> ctx->rand_read_pos = DEFAULT_BLK_SZ; /* Force immediate
refill */
Ciao
Stephan
This is a separate patch so it may be considered separately.
I think it's in the spirit of the original ANSI specs, but opinions
are solicited.
Signed-off-by: George Spelvin <[email protected]>
---
crypto/ansi_cprng.c | 9 +++++++++
1 file changed, 9 insertions(+)
I'm really not sure what people will think of this.
diff --git a/crypto/ansi_cprng.c b/crypto/ansi_cprng.c
index c2c285f3..4ed7c0cf 100644
--- a/crypto/ansi_cprng.c
+++ b/crypto/ansi_cprng.c
@@ -213,6 +213,15 @@ static int get_prng_bytes(unsigned char *buf, unsigned int nbytes,
len = nbytes - pos;
memcpy(buf + pos, ctx->rand_data.bytes + read_pos, len);
read_pos += len;
+ /*
+ * If not in deterministic mode, never buffer old entropy;
+ * re-seed on each read request. This is in the spirit of the
+ * specifications, which are themselves not clear on the subject
+ * of multiple requests for output over a period of time.
+ */
+ if (!(ctx->flags & PRNG_DETERMINISTIC))
+ read_pos = DEFAULT_BLK_SZ;
+
memzero_explicit(ctx->rand_data.bytes, read_pos);
ctx->rand_read_pos = read_pos;
err = nbytes;
--
2.1.3
crypto_rng_seedsize() isn't necessarily enough.
Also (while we're at it), dynamically allocate the result (in the
same buffer) as well.
Signed-off-by: George Spelvin <[email protected]>
---
crypto/testmgr.c | 27 +++++++++++++++++++++------
1 file changed, 21 insertions(+), 6 deletions(-)
Much of this gets undone in the next patch, but I wanted to show the idea.
diff --git a/crypto/testmgr.c b/crypto/testmgr.c
index 29a0cbdd..b81e593d 100644
--- a/crypto/testmgr.c
+++ b/crypto/testmgr.c
@@ -1449,9 +1449,19 @@ static int test_cprng(struct crypto_rng *tfm, struct cprng_testvec *template,
const char *algo = crypto_tfm_alg_driver_name(crypto_rng_tfm(tfm));
int err = 0, i, j, seedsize;
u8 *seed;
- char result[32];
- seedsize = crypto_rng_seedsize(tfm);
+ /*
+ * How big a seed + result buffer do we need? Note that some
+ * tests use a non-default seed size, so crypto_rng_seedsize(tfm)
+ * isn't necessarily enough.
+ */
+ seedsize = 0;
+ for (i = 0; i < tcount; i++) {
+ j = template[i].vlen + template[i].klen +
+ template[i].dtlen + template[i].rlen;
+ if (j > seedsize)
+ seedsize = j;
+ }
seed = kmalloc(seedsize, GFP_KERNEL);
if (!seed) {
@@ -1461,13 +1471,16 @@ static int test_cprng(struct crypto_rng *tfm, struct cprng_testvec *template,
}
for (i = 0; i < tcount; i++) {
- memset(result, 0, 32);
memcpy(seed, template[i].v, template[i].vlen);
memcpy(seed + template[i].vlen, template[i].key,
template[i].klen);
memcpy(seed + template[i].vlen + template[i].klen,
template[i].dt, template[i].dtlen);
+ seedsize = template[i].vlen + template[i].klen +
+ template[i].dtlen +
+
+ memset(seed+seedsize, 0, template[i].rlen);
err = crypto_rng_reset(tfm, seed, seedsize);
if (err) {
@@ -1477,7 +1490,7 @@ static int test_cprng(struct crypto_rng *tfm, struct cprng_testvec *template,
}
for (j = 0; j < template[i].loops; j++) {
- err = crypto_rng_get_bytes(tfm, result,
+ err = crypto_rng_get_bytes(tfm, seed + seedsize,
template[i].rlen);
if (err != template[i].rlen) {
printk(KERN_ERR "alg: cprng: Failed to obtain "
@@ -1488,12 +1501,12 @@ static int test_cprng(struct crypto_rng *tfm, struct cprng_testvec *template,
}
}
- err = memcmp(result, template[i].result,
+ err = memcmp(seed + seedsize, template[i].result,
template[i].rlen);
if (err) {
printk(KERN_ERR "alg: cprng: Test %d failed for %s\n",
i, algo);
- hexdump(result, template[i].rlen);
+ hexdump(seed + seedsize, template[i].rlen);
err = -EINVAL;
goto out;
}
@@ -1722,6 +1735,8 @@ static int alg_test_cprng(const struct alg_test_desc *desc, const char *driver,
crypto_free_rng(rng);
+printk("alg_test_cprng: testing %s: err %d\n", driver, err);
+
return err;
}
--
2.1.3
The current code stores three pointers to three arrays, and three lengths,
and the only thing it does with them is concatenate them.
This seems ridiculous. Just store one combined array and combined
length, and don't do any reformatting at run-time.
One questionable decision is the addition of consts. Really, a lot
larger cleanup is called for, but I just did the area I touched.
Hopefully that doesn't give someone seeing the rest the idea that
it's missing elsewhere for a significant reason.
Signed-off-by: George Spelvin <[email protected]>
---
crypto/testmgr.c | 58 ++++++++-------------------------
crypto/testmgr.h | 98 ++++++++++++++++++++++++--------------------------------
2 files changed, 54 insertions(+), 102 deletions(-)
This should get a reaction...
diff --git a/crypto/testmgr.c b/crypto/testmgr.c
index b81e593d..55faabed 100644
--- a/crypto/testmgr.c
+++ b/crypto/testmgr.c
@@ -105,7 +105,7 @@ struct hash_test_suite {
};
struct cprng_test_suite {
- struct cprng_testvec *vecs;
+ const struct cprng_testvec *vecs;
unsigned int count;
};
@@ -1443,77 +1443,45 @@ static int test_pcomp(struct crypto_pcomp *tfm,
}
-static int test_cprng(struct crypto_rng *tfm, struct cprng_testvec *template,
- unsigned int tcount)
+static int test_cprng(struct crypto_rng *tfm,
+ const struct cprng_testvec *template, unsigned int tcount)
{
const char *algo = crypto_tfm_alg_driver_name(crypto_rng_tfm(tfm));
- int err = 0, i, j, seedsize;
- u8 *seed;
+ int err = 0, i, j;
+ u8 result[32];
- /*
- * How big a seed + result buffer do we need? Note that some
- * tests use a non-default seed size, so crypto_rng_seedsize(tfm)
- * isn't necessarily enough.
- */
- seedsize = 0;
for (i = 0; i < tcount; i++) {
- j = template[i].vlen + template[i].klen +
- template[i].dtlen + template[i].rlen;
- if (j > seedsize)
- seedsize = j;
- }
+ memset(result, 0, sizeof result);
- seed = kmalloc(seedsize, GFP_KERNEL);
- if (!seed) {
- printk(KERN_ERR "alg: cprng: Failed to allocate seed space "
- "for %s\n", algo);
- return -ENOMEM;
- }
-
- for (i = 0; i < tcount; i++) {
-
- memcpy(seed, template[i].v, template[i].vlen);
- memcpy(seed + template[i].vlen, template[i].key,
- template[i].klen);
- memcpy(seed + template[i].vlen + template[i].klen,
- template[i].dt, template[i].dtlen);
- seedsize = template[i].vlen + template[i].klen +
- template[i].dtlen +
-
- memset(seed+seedsize, 0, template[i].rlen);
-
- err = crypto_rng_reset(tfm, seed, seedsize);
+ err = crypto_rng_reset(tfm, template[i].seed, template[i].slen);
if (err) {
printk(KERN_ERR "alg: cprng: Failed to reset rng "
"for %s\n", algo);
- goto out;
+ break;
}
for (j = 0; j < template[i].loops; j++) {
- err = crypto_rng_get_bytes(tfm, seed + seedsize,
+ err = crypto_rng_get_bytes(tfm, result,
template[i].rlen);
if (err != template[i].rlen) {
printk(KERN_ERR "alg: cprng: Failed to obtain "
"the correct amount of random data for "
"%s (requested %d, got %d)\n", algo,
template[i].rlen, err);
- goto out;
+ break;
}
}
- err = memcmp(seed + seedsize, template[i].result,
- template[i].rlen);
+ err = memcmp(result, template[i].result, template[i].rlen);
if (err) {
printk(KERN_ERR "alg: cprng: Test %d failed for %s\n",
i, algo);
- hexdump(seed + seedsize, template[i].rlen);
+ hexdump(result, template[i].rlen);
err = -EINVAL;
- goto out;
+ break;
}
}
-out:
- kfree(seed);
return err;
}
diff --git a/crypto/testmgr.h b/crypto/testmgr.h
index 62e2485b..5a789d87 100644
--- a/crypto/testmgr.h
+++ b/crypto/testmgr.h
@@ -81,14 +81,10 @@ struct aead_testvec {
};
struct cprng_testvec {
- char *key;
- char *dt;
- char *v;
- char *result;
- unsigned char klen;
- unsigned short dtlen;
- unsigned short vlen;
- unsigned short rlen;
+ const char *seed;
+ const char *result;
+ unsigned char slen;
+ unsigned char rlen;
unsigned short loops;
};
@@ -20711,87 +20707,75 @@ static struct aead_testvec aes_ccm_rfc4309_dec_tv_template[] = {
*/
#define ANSI_CPRNG_AES_TEST_VECTORS 6
-static struct cprng_testvec ansi_cprng_aes_tv_template[] = {
+static const struct cprng_testvec ansi_cprng_aes_tv_template[] = {
{
- .key = "\xf3\xb1\x66\x6d\x13\x60\x72\x42"
- "\xed\x06\x1c\xab\xb8\xd4\x62\x02",
- .klen = 16,
- .dt = "\xe6\xb3\xbe\x78\x2a\x23\xfa\x62"
+ .seed = "\x80\x00\x00\x00\x00\x00\x00\x00" /* V[16] */
+ "\x00\x00\x00\x00\x00\x00\x00\x00"
+ "\xf3\xb1\x66\x6d\x13\x60\x72\x42" /* Key[16] */
+ "\xed\x06\x1c\xab\xb8\xd4\x62\x02"
+ "\xe6\xb3\xbe\x78\x2a\x23\xfa\x62" /* DT[16] */
"\xd7\x1d\x4a\xfb\xb0\xe9\x22\xf9",
- .dtlen = 16,
- .v = "\x80\x00\x00\x00\x00\x00\x00\x00"
- "\x00\x00\x00\x00\x00\x00\x00\x00",
- .vlen = 16,
+ .slen = 48,
.result = "\x59\x53\x1e\xd1\x3b\xb0\xc0\x55"
"\x84\x79\x66\x85\xc1\x2f\x76\x41",
.rlen = 16,
.loops = 1,
}, {
- .key = "\xf3\xb1\x66\x6d\x13\x60\x72\x42"
- "\xed\x06\x1c\xab\xb8\xd4\x62\x02",
- .klen = 16,
- .dt = "\xe6\xb3\xbe\x78\x2a\x23\xfa\x62"
+ .seed = "\xc0\x00\x00\x00\x00\x00\x00\x00" /* V */
+ "\x00\x00\x00\x00\x00\x00\x00\x00"
+ "\xf3\xb1\x66\x6d\x13\x60\x72\x42" /* Key */
+ "\xed\x06\x1c\xab\xb8\xd4\x62\x02"
+ "\xe6\xb3\xbe\x78\x2a\x23\xfa\x62" /* DT */
"\xd7\x1d\x4a\xfb\xb0\xe9\x22\xfa",
- .dtlen = 16,
- .v = "\xc0\x00\x00\x00\x00\x00\x00\x00"
- "\x00\x00\x00\x00\x00\x00\x00\x00",
- .vlen = 16,
+ .slen = 48,
.result = "\x7c\x22\x2c\xf4\xca\x8f\xa2\x4c"
"\x1c\x9c\xb6\x41\xa9\xf3\x22\x0d",
.rlen = 16,
.loops = 1,
}, {
- .key = "\xf3\xb1\x66\x6d\x13\x60\x72\x42"
- "\xed\x06\x1c\xab\xb8\xd4\x62\x02",
- .klen = 16,
- .dt = "\xe6\xb3\xbe\x78\x2a\x23\xfa\x62"
+ .seed = "\xe0\x00\x00\x00\x00\x00\x00\x00" /* V */
+ "\x00\x00\x00\x00\x00\x00\x00\x00"
+ "\xf3\xb1\x66\x6d\x13\x60\x72\x42" /* Key */
+ "\xed\x06\x1c\xab\xb8\xd4\x62\x02"
+ "\xe6\xb3\xbe\x78\x2a\x23\xfa\x62" /* DT */
"\xd7\x1d\x4a\xfb\xb0\xe9\x22\xfb",
- .dtlen = 16,
- .v = "\xe0\x00\x00\x00\x00\x00\x00\x00"
- "\x00\x00\x00\x00\x00\x00\x00\x00",
- .vlen = 16,
+ .slen = 48,
.result = "\x8a\xaa\x00\x39\x66\x67\x5b\xe5"
"\x29\x14\x28\x81\xa9\x4d\x4e\xc7",
.rlen = 16,
.loops = 1,
}, {
- .key = "\xf3\xb1\x66\x6d\x13\x60\x72\x42"
- "\xed\x06\x1c\xab\xb8\xd4\x62\x02",
- .klen = 16,
- .dt = "\xe6\xb3\xbe\x78\x2a\x23\xfa\x62"
+ .seed = "\xf0\x00\x00\x00\x00\x00\x00\x00" /* V */
+ "\x00\x00\x00\x00\x00\x00\x00\x00"
+ "\xf3\xb1\x66\x6d\x13\x60\x72\x42" /* Key */
+ "\xed\x06\x1c\xab\xb8\xd4\x62\x02"
+ "\xe6\xb3\xbe\x78\x2a\x23\xfa\x62" /* DT */
"\xd7\x1d\x4a\xfb\xb0\xe9\x22\xfc",
- .dtlen = 16,
- .v = "\xf0\x00\x00\x00\x00\x00\x00\x00"
- "\x00\x00\x00\x00\x00\x00\x00\x00",
- .vlen = 16,
+ .slen = 48,
.result = "\x88\xdd\xa4\x56\x30\x24\x23\xe5"
"\xf6\x9d\xa5\x7e\x7b\x95\xc7\x3a",
.rlen = 16,
.loops = 1,
}, {
- .key = "\xf3\xb1\x66\x6d\x13\x60\x72\x42"
- "\xed\x06\x1c\xab\xb8\xd4\x62\x02",
- .klen = 16,
- .dt = "\xe6\xb3\xbe\x78\x2a\x23\xfa\x62"
+ .seed = "\xf8\x00\x00\x00\x00\x00\x00\x00" /* V */
+ "\x00\x00\x00\x00\x00\x00\x00\x00"
+ "\xf3\xb1\x66\x6d\x13\x60\x72\x42" /* Key */
+ "\xed\x06\x1c\xab\xb8\xd4\x62\x02"
+ "\xe6\xb3\xbe\x78\x2a\x23\xfa\x62" /* DT */
"\xd7\x1d\x4a\xfb\xb0\xe9\x22\xfd",
- .dtlen = 16,
- .v = "\xf8\x00\x00\x00\x00\x00\x00\x00"
- "\x00\x00\x00\x00\x00\x00\x00\x00",
- .vlen = 16,
+ .slen = 48,
.result = "\x05\x25\x92\x46\x61\x79\xd2\xcb"
"\x78\xc4\x0b\x14\x0a\x5a\x9a\xc8",
.rlen = 16,
.loops = 1,
}, { /* Monte Carlo Test */
- .key = "\x9f\x5b\x51\x20\x0b\xf3\x34\xb5"
- "\xd8\x2b\xe8\xc3\x72\x55\xc8\x48",
- .klen = 16,
- .dt = "\x63\x76\xbb\xe5\x29\x02\xba\x3b"
+ .seed = "\x57\x2c\x8e\x76\x87\x26\x47\x97" /* V */
+ "\x7e\x74\xfb\xdd\xc4\x95\x01\xd1"
+ "\x9f\x5b\x51\x20\x0b\xf3\x34\xb5" /* Key */
+ "\xd8\x2b\xe8\xc3\x72\x55\xc8\x48"
+ "\x63\x76\xbb\xe5\x29\x02\xba\x3b" /* DT */
"\x67\xc9\x25\xfa\x70\x1f\x11\xac",
- .dtlen = 16,
- .v = "\x57\x2c\x8e\x76\x87\x26\x47\x97"
- "\x7e\x74\xfb\xdd\xc4\x95\x01\xd1",
- .vlen = 16,
+ .slen = 48,
.result = "\x48\xe9\xbd\x0d\x06\xee\x18\xfb"
"\xe4\x57\x90\xd5\xc3\xfc\x9b\x73",
.rlen = 16,
--
2.1.3
The larger seed with deterministic DT is still supported, but make the
default non-deterministically random, with fresh DT.
This must come after the patch that makes testmgr.c not depend on
the default seedsize to allocate its buffers, since it tests the
non-default (larger seed) case.
Signed-off-by: George Spelvin <[email protected]>
---
crypto/ansi_cprng.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
Patch 15 is a prerequisite for this one.
diff --git a/crypto/ansi_cprng.c b/crypto/ansi_cprng.c
index 4ed7c0cf..875c4bf9 100644
--- a/crypto/ansi_cprng.c
+++ b/crypto/ansi_cprng.c
@@ -390,7 +390,7 @@ static struct crypto_alg rng_algs[] = { {
.rng = {
.rng_make_random = cprng_get_random,
.rng_reset = cprng_reset,
- .seedsize = DEFAULT_PRNG_KSZ + 2*DEFAULT_BLK_SZ,
+ .seedsize = DEFAULT_BLK_SZ + DEFAULT_PRNG_KSZ
}
}
#ifdef CONFIG_CRYPTO_FIPS
@@ -408,7 +408,7 @@ static struct crypto_alg rng_algs[] = { {
.rng = {
.rng_make_random = fips_cprng_get_random,
.rng_reset = fips_cprng_reset,
- .seedsize = DEFAULT_PRNG_KSZ + 2*DEFAULT_BLK_SZ,
+ .seedsize = DEFAULT_BLK_SZ + DEFAULT_PRNG_KSZ
}
}
#endif
--
2.1.3
>From [email protected] Tue Dec 02 08:57:23 2014
X-AuthUser: [email protected]
From: Stephan Mueller <[email protected]>
To: George Spelvin <[email protected]>
Cc: [email protected], [email protected], [email protected]
Subject: Re: [PATCH 02/17] crypto: ansi_cprng - Eliminate ctx->last_rand_data
Date: Tue, 02 Dec 2014 09:57:17 +0100
User-Agent: KMail/4.14.2 (Linux/3.17.2-200.fc20.x86_64; KDE/4.14.2; x86_64; ; )
In-Reply-To: <[email protected]>
References: <[email protected]>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"
Am Dienstag, 2. Dezember 2014, 03:35:50 schrieb George Spelvin:
Hi George,
>> It's simply not necessary.
> Can you please be a bit more verbose on why you think this is not
> necessary?
Sorry, I thought the code made that obvious. The two buffers have to
exist simultaneously very briefly in order to be compared, but the
old data can be overwritten immediately thereafter.
So what the revised code does is:
I := E(DT) (The buffer is called "tmp")
V ^= I
V := E(V) (This can be stored in V without problems)
compare V with read_data
read_data := V
V ^= I
V := E(V)
> Have you tested that change with reference test vectors -- what do
> testmgr test vectors say?
As I explained in part 00, yes. The behaviour is identical.
I should mention, however, that I did not exactly use testmgr; I cut &
pasted the relevant test vectors & code into ansi_cprng.c, then verified
that the tests passed with both old and modified code. I have so far been
unable to figure out how to make the tcrypt module do anything useful.
I just realized that the memzero_explicit of ctx->rand_data_bytes[]
(a late addition, done just a few minutes before posting), while
it prevents backtracking, also totally breaks FIPS anti-repetition
checking.
So ignore that line (171 of the modified file). Sorry.
On Tue, Dec 02, 2014 at 03:35:50AM -0500, George Spelvin wrote:
> It's simply not necessary.
>
> Signed-off-by: George Spelvin <[email protected]>
NACK
The assumption that its not needed is incorrect. In fips mode its explicitly
needed to validate that the rng isn't reproducing identical random data.
Neil
On Tue, Dec 02, 2014 at 03:37:07AM -0500, George Spelvin wrote:
> It's also not necessary. We do have to change some debugging
> output.
>
> Signed-off-by: George Spelvin <[email protected]>
> ---
> crypto/ansi_cprng.c | 39 ++++++++++++++++++++-------------------
> 1 file changed, 20 insertions(+), 19 deletions(-)
>
I'm only ok with removing I if you can continue to be able to output it. given
that I is listed as part of the test sequences that NIST provides, I'd like to
be able to compare the values.
Neil
On Tue, Dec 02, 2014 at 03:43:13AM -0500, George Spelvin wrote:
> rand_read_pos is never more than 16, while there's only 1 flag
> bit allocated, so we can shrink the context a little.
>
> Signed-off-by: George Spelvin <[email protected]>
> ---
> crypto/ansi_cprng.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> They're also reordered to avoid alignment holes.
>
> diff --git a/crypto/ansi_cprng.c b/crypto/ansi_cprng.c
> index 93ed00f6..f40f54cd 100644
> --- a/crypto/ansi_cprng.c
> +++ b/crypto/ansi_cprng.c
> @@ -51,9 +51,9 @@ struct prng_context {
> unsigned char rand_data[DEFAULT_BLK_SZ];
> unsigned char DT[DEFAULT_BLK_SZ];
> unsigned char V[DEFAULT_BLK_SZ];
> - u32 rand_read_pos; /* Offset into rand_data[] */
> + unsigned char rand_read_pos; /* Offset into rand_data[] */
u8 please. Also, not sure if this helps much, as I think the padding will just
get you back to word alignment on each of these.
> + unsigned char flags;
> struct crypto_cipher *tfm;
> - u32 flags;
> };
>
> static int dbg;
> --
> 2.1.3
>
>
>> if (byte_count < DEFAULT_BLK_SZ) {
>> empty_rbuf:
>>- while (ctx->rand_data_valid < DEFAULT_BLK_SZ) {
>>- *ptr = ctx->rand_data[ctx->rand_data_valid];
>>- ptr++;
>>- byte_count--;
>>- ctx->rand_data_valid++;
>>- if (byte_count == 0)
>>+ while (ctx->rand_read_pos < DEFAULT_BLK_SZ) {
>>+ *ptr++ = ctx->rand_data[ctx->rand_read_pos++];
>>+ if (--byte_count == 0)
>> goto done;
> I am against such collapsing of constructs into one-liners. It is not
> obvious at first sight, which value gets incremented in what order. Such
> collapsing was the cause for CVE-2013-4345 as it caused an off-by one.
Given that this turns out to the the exact code where that happened, I can
see the sensitivity of the matter! But I disagree in this case, and
it took me a while to figure out how to phrase it.
It's a code style issue, which means that it doesn't have a clear
technical answer. It's more of a "voting on what people think is
clearer" thing.
In the case of "if (--byte_count)" issue, that's not something I feel very
strongly about.
But in the case of "*ptr++ = ctx->rand_data[ctx->rand_read_pos++]", it's
the opposite. While I'm not going to pick a fight over it, my opinion
is that this form is clearly better.
There are two advantages to the code in this form:
1. "*dst++ = *src++" is a C idiom, like "for (i = 0; i < N; i++)".
It's very easy to recognize and understand. A more broken-up form
less obviously does all the necessary accounting.
2. The original bug was NOT caused by using side effects. Consider the
bug fix:
--- a/crypto/ansi_cprng.c
+++ b/crypto/ansi_cprng.c
@@ -230,11 +230,11 @@ remainder:
*/
if (byte_count < DEFAULT_BLK_SZ) {
empty_rbuf:
- for (; ctx->rand_data_valid < DEFAULT_BLK_SZ;
- ctx->rand_data_valid++) {
+ while (ctx->rand_data_valid < DEFAULT_BLK_SZ) {
*ptr = ctx->rand_data[ctx->rand_data_valid];
ptr++;
byte_count--;
+ ctx->rand_data_valid++;
if (byte_count == 0)
goto done;
}
The problem was the *separation* of the copy and the associated increment.
In some situations, one was done and not the other, leading to data
duplication. The fix was to move the increment of ctx->rand_data_valid
to before the "if (byte_count == 0)" loop exit test.
However, putting the increment in the same line as the copy reduces the
separation that caused the original bug, and makes it *more* clear that
the parts always happen together.
This, fundamentally, is the reason that I actually find it preferable:
it's conceptually "one operation" that should always have all of its parts
done together, and putting it on one line makes that easier for
the reader to recognize.
The problem with the original was putting it in the for(), not
using side effect expressions.
The above logic doesn't apply to "if (--byte_count)", which is why I
don't care about it nearly as much.
All that said, there are two significant remaining points:
3. this patch claims it's just a variable rename; perhaps it should stick
to that, and
4. patch 10/15 totally deletes this code and replaces it with something
even simpler, so if that is acceptable this entire discussion may be moot.
> NACK
>
> The assumption that its not needed is incorrect. In fips mode its explicitly
> needed to validate that the rng isn't reproducing identical random data.
Please take a second look. The validation is still there; I fully
understand that and preserved that. (Well, I broke it later getting
over-eager looking for places to put memzero_explicit, but already
sent a follow-on message about that.)
Only the *buffer* is unnecessary and was deleted.
> I'm only ok with removing I if you can continue to be able to output it.
> given that I is listed as part of the test sequences that NIST provides,
> I'd like to be able to compare the values.
I can do that easily, but I can't print the *input* I, which
is the result of encrypting the previous DT, as it's thrown
away earlier.
You'd have to look further back in the debug messages to find it.
Is changing the format of the debug messages okay? I'd like the debug
messages to describe the code, but I don't know if you have something
that parses the current output.
The test output I see on p. 33 of
http://csrc.nist.gov/groups/STM/cavp/documents/rng/RNGVS.pdf
doesn't include I. Can you point me to a sample that includes I?
It might be best to more significantly rework the debug messages to
resemble the NIST test vectors.
>> --- a/crypto/ansi_cprng.c
>> +++ b/crypto/ansi_cprng.c
>> @@ -51,9 +51,9 @@ struct prng_context {
>> unsigned char rand_data[DEFAULT_BLK_SZ];
>> unsigned char DT[DEFAULT_BLK_SZ];
>> unsigned char V[DEFAULT_BLK_SZ];
>> - u32 rand_read_pos; /* Offset into rand_data[] */
>> + unsigned char rand_read_pos; /* Offset into rand_data[] */
> u8 please. Also, not sure if this helps much, as I think the padding
> will just get you back to word alignment on each of these.
I noticed the "unsigned char" vs "u8" issue, but didn't make the change
as I didn't think the readailility improvement was worth the code churn.
But I'd be happy to clean that up, too!
Should I convert all the buffers and function prototypes, or is there
some criterion for distinguishing which gets which? (E.g. buffers are
"unsigned char" but control variables are "u8".)
And actually, you do win. spinlock_t is 16 bits on x86,
and the buffers are all 16 bytes. (80 bytes before my earlier
patches, 48 bytes after.)
So the the structure goes from:
32-bit 64-bit Variable
Offset Size Offset Size
0 2 0 2 spinlock_t prng_lock
2 16 2 16 unsigned char rand_data[16]
18 16 18 16 unsigned char DT[16]
34 16 34 16 unsigned char V[16]
50 2 50 2 (alignemnt)
52 4 52 4 u32 rand_read_pos
56 4 56 8 struct crypto_cipher *tfm
60 4 64 4 u32 flags
68 4 (alignment)
64 72 (structure size)
to
32-bit 64-bit Variable
Offset Size Offset Size
34 16 34 16 unsigned char V[16]
50 1 50 1 u8 rand_read_pos
51 1 51 1 u8 flags
52 4 (alignment)
52 4 56 8 struct crypto_cipher *tfm
56 64 (structure size)
You still get 4 bytes of alignment padding on x86-64, but given that
the structure has 60 bytes of payload, that's the minimum possible.
You save 6 bytes of variables and 2 bytes of padding on both
32- and 64-bit systems, for a total of 8 bytes, and that's enough
to knock you into a smaller slab object bin on 64-bit.
I forget where I read the terminology, but the most efficient
wat to pack a structure is in an "organ pipe" configuraiton where
the biggest (in terms of *alignment*) members are on the outside
and the structre and the smaller elements are on the inside.
Putting a 32-bit "flags" after a 64-bit pointer violates that.
The order of the v1 patches is somewhat in order of "increasing scale",
reflecting my learning about the code. But if this unroll is okay,
it would probably make the most sense to reorder things so it's
first and then other changes can be made on top of the simpler code.
Given the unusual implementation choice of a loop and a switch, it's
obviously a deliberate one, so I assume there's a reason for it that
I'm just not seeing.
On Tue, Dec 02, 2014 at 03:03:38PM -0500, George Spelvin wrote:
> > I'm only ok with removing I if you can continue to be able to output it.
> > given that I is listed as part of the test sequences that NIST provides,
> > I'd like to be able to compare the values.
>
> I can do that easily, but I can't print the *input* I, which
> is the result of encrypting the previous DT, as it's thrown
> away earlier.
>
> You'd have to look further back in the debug messages to find it.
>
> Is changing the format of the debug messages okay? I'd like the debug
> messages to describe the code, but I don't know if you have something
> that parses the current output.
>
I'm fine with changing the output, as I don't think anything particularly relies
on the format, but I cant' speak for others
Neil
>
> The test output I see on p. 33 of
> http://csrc.nist.gov/groups/STM/cavp/documents/rng/RNGVS.pdf
> doesn't include I. Can you point me to a sample that includes I?
>
> It might be best to more significantly rework the debug messages to
> resemble the NIST test vectors.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
On Tue, Dec 02, 2014 at 03:28:17PM -0500, George Spelvin wrote:
> >> --- a/crypto/ansi_cprng.c
> >> +++ b/crypto/ansi_cprng.c
> >> @@ -51,9 +51,9 @@ struct prng_context {
> >> unsigned char rand_data[DEFAULT_BLK_SZ];
> >> unsigned char DT[DEFAULT_BLK_SZ];
> >> unsigned char V[DEFAULT_BLK_SZ];
> >> - u32 rand_read_pos; /* Offset into rand_data[] */
> >> + unsigned char rand_read_pos; /* Offset into rand_data[] */
>
> > u8 please. Also, not sure if this helps much, as I think the padding
> > will just get you back to word alignment on each of these.
>
> I noticed the "unsigned char" vs "u8" issue, but didn't make the change
> as I didn't think the readailility improvement was worth the code churn.
>
You just sent a 17 patch series of clean up and were worried about code churn
converting an unsigned char to a u8?
> But I'd be happy to clean that up, too!
>
> Should I convert all the buffers and function prototypes, or is there
> some criterion for distinguishing which gets which? (E.g. buffers are
> "unsigned char" but control variables are "u8".)
>
If you want to sure. u8 probably makes more sense for the buffers here as our
intent is to treat them as an array of byte values.
> And actually, you do win. spinlock_t is 16 bits on x86,
> and the buffers are all 16 bytes. (80 bytes before my earlier
> patches, 48 bytes after.)
>
> So the the structure goes from:
>
> 32-bit 64-bit Variable
> Offset Size Offset Size
> 0 2 0 2 spinlock_t prng_lock
> 2 16 2 16 unsigned char rand_data[16]
> 18 16 18 16 unsigned char DT[16]
> 34 16 34 16 unsigned char V[16]
> 50 2 50 2 (alignemnt)
> 52 4 52 4 u32 rand_read_pos
> 56 4 56 8 struct crypto_cipher *tfm
> 60 4 64 4 u32 flags
> 68 4 (alignment)
> 64 72 (structure size)
>
> to
>
> 32-bit 64-bit Variable
> Offset Size Offset Size
> 34 16 34 16 unsigned char V[16]
> 50 1 50 1 u8 rand_read_pos
> 51 1 51 1 u8 flags
> 52 4 (alignment)
> 52 4 56 8 struct crypto_cipher *tfm
> 56 64 (structure size)
>
> You still get 4 bytes of alignment padding on x86-64, but given that
> the structure has 60 bytes of payload, that's the minimum possible.
>
> You save 6 bytes of variables and 2 bytes of padding on both
> 32- and 64-bit systems, for a total of 8 bytes, and that's enough
> to knock you into a smaller slab object bin on 64-bit.
>
>
> I forget where I read the terminology, but the most efficient
> wat to pack a structure is in an "organ pipe" configuraiton where
> the biggest (in terms of *alignment*) members are on the outside
> and the structre and the smaller elements are on the inside.
> Putting a 32-bit "flags" after a 64-bit pointer violates that.
>
On Tue, Dec 02, 2014 at 03:33:14AM -0500, George Spelvin wrote:
> Thank you all for your hospitality to my amateurish efforts.
>
> Given that this all started with me reading the code in an attempt to
> understand it, it is likely that some of the "problems" I address are
> actually misunderstandings on my part. If all I get from this patch series
> is some enlightenment, that's okay.
>
> It's even more likely that some parts contain the germ of a good idea,
> but will need considerable rework to be acceptable. I look forward
> to that too.
>
> Expecting that much more work will be needed, I've run the testmgr.h
> test vectors on this code, but haven't desk-checked it as thoroughly as
> security-sensitive code should be before final acceptance. If the ideas
> pass muster, I'll double-check the implementatoin details.
>
> Really, I'm just trying to understand the code. Patches are just
> a very specific way of talking about it.
>
> Here's an overview of the series. It's a lot of code cleanup, and
> a bit of semantic change.
>
> [01/17] crypto: ansi_cprng - Rename rand_data_valid more sensibly
> I find it confusing, so I rename it to rand_data_pos
> [02/17] crypto: ansi_cprng - Eliminate ctx->last_rand_data
> Shrink the context structure.
> [03/17] crypto: ansi_cprng - Eliminate ctx->I
> Shrink it some more.
> [04/17] crypto: ansi_cprng - simplify xor_vectors() to xor_block()
> We don't need a size parameter.
> [05/17] crypto: ansi_cprng - Add const annotations to hexdump()
> I like const.
> [06/17] crypto: ansi_cprng - Eliminate unused PRNG_FIXED_SIZE flag
> It's dead code ACAICT.
> [07/17] crypto: ansi_cprng - Shrink rand_read_pos & flags
> A little more context shrinking.
> [08/17] crypto: ansi_cprng - Require non-null key & V in reset_prng_context
> Sue to the PRNG_NEED_RESET flag, cprng_init() doesn't need to call
> reset_prng_context) directly.
> [09/17] crypto: ansi_cprng - Clean up some variable types
> Try to be more consistent about signed vs. unsigned.
> [10/17] crypto: ansi_cprng - simplify get_prng_bytes
> Code shrink & simplification.
> [11/17] crypto: ansi_cprng - unroll _get_more_prng_bytes
> Slight object code size savings, and definite readability improvement.
> [12/17] crypto: ansi_cprng - Create a "block buffer" data type
> union { u8 bytes[16]; u32 ints[4]; u64 longs[2]; }, sort of.
> [13/17] crypto: ansi_cprng - If DT is not provided, use a fresh timestamp
> This is the big semantic change. I'm rather proud of the use
> of get_random_int() as a timestamp, but feedback is definitely
> solicited!
> [14/17] crypto: ansi_cprng - If DT is omitted, don't buffer old output
> Not sure if this is desirable or not.
> [15/17] crypto: testmgr - Teach test_cprng to handle non-default seed sizes
> I consider this a latent bug that patch 17 sould expose.
> [16/17] crypto: testmgr - Merge seed arrays in struct cprng_testvec
> I think this is a better way of solving the preceding problem,
> but it's more intrusive. All the consts I add are not a critical
> part of the patch, but an example of what I'd like to do to the
> rest of the code.
> [17/17] crypto: ansi_cprng - Shrink default seed size
> This makes (some amount of) true entropy the default.
>
So, generally speaking I'm ok with most of this I think, but before it goes
anywhere, it really needs to pass the NIST and FIPS test vectors. Can you do
that please and post the results?
Neil
> So, generally speaking I'm ok with most of this I think,
One open issue... I thought you had said that the non-determinsitic
version was not in the current X9.31 and I had the impression that it
wasn't wanted. I've got reorganized patch series that gets rid of that
and just tweaks the comments.
I'm happy to put it back, of course. Or just hold off until Herbert
chimes in with an opinion.
As an example of the reorganization, now the comment for patch 4 in the
series is a bit clearer:
crypto: ansi_cprng - Eliminate ctx->I and ctx->last_rand_data
Careful use of the other available buffers avoids the need for
these, shrinking the context by 32 bytes.
Neither the debug output nor the FIPS-required anti-repetition check
are changed in the slightest.
(That's because I change the debug output in patches 2 and 3, to make
this more-sensitive patch easier to review.)
> but before it goes anywhere, it really needs to pass the NIST and FIPS
> test vectors. Can you do that please and post the results?
It of course passes the ones in testmgr.h, and I can add the others from
http://csrc.nist.gov/groups/STM/cavp/documents/rng/RNGVS.pdf
and
http://csrc.nist.gov/groups/STM/cavp/documents/rng/rngtestvectors.zip
I didn't know there were separate FIPS test vectors for this PRNG;
can you give me a clue where to find them?
I'm also not sure what "the results" look like. As I mentioned previously,
I have been unable to figure out how to make the tcrypt code print anything
in the event of a successful test, so its output is the empty string.
I am using my own version which prints "cprng: Test %d passed", and
I can turn debugging on, but the 10000-round MCT produces an annoying
amount of output in that case.
(Note to self: teach test_cprng to test odd-sized reads, since that was
a previous bug source and I'm touching that code some more.)
> I'm fine with changing the output, as I don't think anything
> particularly relies on the format, but I can't speak for others.
The current debug output for the first 5 testmgr.h tests (the 6th is
omitted) is follows, but obviously things get reconfirmed right at
the end.
getting 16 random bytes for context ffff88042ce41b18
Calling _get_more_prng_bytes for context ffff88042ce41b18
DT = e6b3be782a23fa62d71d4afbb0e922f9
V = 80000000000000000000000000000000
I = 92640cf08d302f2550ea3d73d1985385
V^I = 12640cf08d302f2550ea3d73d1985385
R = 59531ed13bb0c05584796685c12f7641
R^I = cb371221b680ef70d4935bf610b725c4
V' = 2ac489ad47640b6d86380229e769adc3
DT' = e6b3be782a23fa62d71d4afbb0e922fa
Returning new block for context ffff88042ce41b18
returning 16 from get_prng_bytes in context ffff88042ce41b18
cprng: Test 0 passed
getting 16 random bytes for context ffff88042ce41b18
Calling _get_more_prng_bytes for context ffff88042ce41b18
DT = e6b3be782a23fa62d71d4afbb0e922fa
V = c0000000000000000000000000000000
I = b30a8cba1c4cd3a14af7d9db9488f5f0
V^I = 730a8cba1c4cd3a14af7d9db9488f5f0
R = 7c222cf4ca8fa24c1c9cb641a9f3220d
R^I = cf28a04ed6c371ed566b6f9a3d7bd7fd
V' = fcbd61e7c51dd167624c56cb97b4c66d
DT' = e6b3be782a23fa62d71d4afbb0e922fb
Returning new block for context ffff88042ce41b18
returning 16 from get_prng_bytes in context ffff88042ce41b18
cprng: Test 1 passed
getting 16 random bytes for context ffff88042ce41b18
Calling _get_more_prng_bytes for context ffff88042ce41b18
DT = e6b3be782a23fa62d71d4afbb0e922fb
V = e0000000000000000000000000000000
I = d761699cc3de4a90234c62eec2479cd9
V^I = 3761699cc3de4a90234c62eec2479cd9
R = 8aaa003966675be529142881a94d4ec7
R^I = 5dcb69a5a5b911750a584a6f6b0ad21e
V' = ef2c1fbf609a68f8fe5110636bf4bf6a
DT' = e6b3be782a23fa62d71d4afbb0e922fc
Returning new block for context ffff88042ce41b18
returning 16 from get_prng_bytes in context ffff88042ce41b18
cprng: Test 2 passed
getting 16 random bytes for context ffff88042ce41b18
Calling _get_more_prng_bytes for context ffff88042ce41b18
DT = e6b3be782a23fa62d71d4afbb0e922fc
V = f0000000000000000000000000000000
I = 048ecb25943e8c31d614cd8a23f13f84
V^I = f48ecb25943e8c31d614cd8a23f13f84
R = 88dda456302423e5f69da57e7b95c73a
R^I = 8c536f73a41aafd4208968f45864f8be
V' = 48893b71bce400b65e21ba378a0ad570
DT' = e6b3be782a23fa62d71d4afbb0e922fd
Returning new block for context ffff88042ce41b18
returning 16 from get_prng_bytes in context ffff88042ce41b18
cprng: Test 3 passed
getting 16 random bytes for context ffff88042ce41b18
Calling _get_more_prng_bytes for context ffff88042ce41b18
DT = e6b3be782a23fa62d71d4afbb0e922fd
V = f8000000000000000000000000000000
I = 3a6a1754bdf6f844e53662990cadc492
V^I = c26a1754bdf6f844e53662990cadc492
R = 052592466179d2cb78c40b140a5a9ac8
R^I = 3f4f8512dc8f2a8f9df2698d06f75e5a
V' = ac4b23c4fb1e85098d9927afa617ad88
DT' = e6b3be782a23fa62d71d4afbb0e922fe
Returning new block for context ffff88042ce41b18
returning 16 from get_prng_bytes in context ffff88042ce41b18
cprng: Test 4 passed
self_test: err 0
prng_mod_init: err 0
>> I noticed the "unsigned char" vs "u8" issue, but didn't make the change
>> as I didn't think the readailility improvement was worth the code churn.
> You just sent a 17 patch series of clean up and were worried about code churn
> converting an unsigned char to a u8?
Yes, it does sound funny like that, but I meant that I have no trouble
reading the types as synonyms, so the improvement is very small, and it's
the ratio.
More simply, "I noticed, but it didn't make me itch enough."
Once I got deep enough into it, perhaps I should have re-evaluated.
>> Should I convert all the buffers and function prototypes,
> If you want to sure. u8 probably makes more sense for the buffers here as our
> intent is to treat them as an array of byte values.
I had a look at <linux/crypto.h>, which uses u8 for everything, so I've
already changed everything to that.
Thank you for your time and effort looking at this!
Am Mittwoch, 3. Dezember 2014, 15:27:53 schrieb George Spelvin:
Hi George,
> > but before it goes anywhere, it really needs to pass the NIST and FIPS
> > test vectors. Can you do that please and post the results?
>
> It of course passes the ones in testmgr.h, and I can add the others from
> http://csrc.nist.gov/groups/STM/cavp/documents/rng/RNGVS.pdf
> and
> http://csrc.nist.gov/groups/STM/cavp/documents/rng/rngtestvectors.zip
>
> I didn't know there were separate FIPS test vectors for this PRNG;
> can you give me a clue where to find them?
I can send you such vectors that would be used in FIPS validations. I will
send them off-list as they are large.
--
Ciao
Stephan
On Wed, Dec 03, 2014 at 03:27:53PM -0500, George Spelvin wrote:
> > So, generally speaking I'm ok with most of this I think,
>
> One open issue... I thought you had said that the non-determinsitic
> version was not in the current X9.31 and I had the impression that it
> wasn't wanted. I've got reorganized patch series that gets rid of that
> and just tweaks the comments.
>
I presume you're referring to the deterministic DT vector here? It is currently
in the implementation, and I personally have no need for a non-deterministic
variant. I'm fine if you add it as long as the default remains the same as it
currently is.
> I'm happy to put it back, of course. Or just hold off until Herbert
> chimes in with an opinion.
>
> As an example of the reorganization, now the comment for patch 4 in the
> series is a bit clearer:
>
> crypto: ansi_cprng - Eliminate ctx->I and ctx->last_rand_data
>
> Careful use of the other available buffers avoids the need for
> these, shrinking the context by 32 bytes.
>
> Neither the debug output nor the FIPS-required anti-repetition check
> are changed in the slightest.
>
> (That's because I change the debug output in patches 2 and 3, to make
> this more-sensitive patch easier to review.)
>
> > but before it goes anywhere, it really needs to pass the NIST and FIPS
> > test vectors. Can you do that please and post the results?
>
> It of course passes the ones in testmgr.h, and I can add the others from
> http://csrc.nist.gov/groups/STM/cavp/documents/rng/RNGVS.pdf
> and
> http://csrc.nist.gov/groups/STM/cavp/documents/rng/rngtestvectors.zip
>
> I didn't know there were separate FIPS test vectors for this PRNG;
> can you give me a clue where to find them?
>
>
> I'm also not sure what "the results" look like. As I mentioned previously,
> I have been unable to figure out how to make the tcrypt code print anything
> in the event of a successful test, so its output is the empty string.
>
> I am using my own version which prints "cprng: Test %d passed", and
> I can turn debugging on, but the 10000-round MCT produces an annoying
> amount of output in that case.
>
> (Note to self: teach test_cprng to test odd-sized reads, since that was
> a previous bug source and I'm touching that code some more.)
>
> > I'm fine with changing the output, as I don't think anything
> > particularly relies on the format, but I can't speak for others.
>
> The current debug output for the first 5 testmgr.h tests (the 6th is
> omitted) is follows, but obviously things get reconfirmed right at
> the end.
>
> getting 16 random bytes for context ffff88042ce41b18
> Calling _get_more_prng_bytes for context ffff88042ce41b18
> DT = e6b3be782a23fa62d71d4afbb0e922f9
> V = 80000000000000000000000000000000
> I = 92640cf08d302f2550ea3d73d1985385
> V^I = 12640cf08d302f2550ea3d73d1985385
> R = 59531ed13bb0c05584796685c12f7641
> R^I = cb371221b680ef70d4935bf610b725c4
> V' = 2ac489ad47640b6d86380229e769adc3
> DT' = e6b3be782a23fa62d71d4afbb0e922fa
> Returning new block for context ffff88042ce41b18
> returning 16 from get_prng_bytes in context ffff88042ce41b18
> cprng: Test 0 passed
> getting 16 random bytes for context ffff88042ce41b18
> Calling _get_more_prng_bytes for context ffff88042ce41b18
> DT = e6b3be782a23fa62d71d4afbb0e922fa
> V = c0000000000000000000000000000000
> I = b30a8cba1c4cd3a14af7d9db9488f5f0
> V^I = 730a8cba1c4cd3a14af7d9db9488f5f0
> R = 7c222cf4ca8fa24c1c9cb641a9f3220d
> R^I = cf28a04ed6c371ed566b6f9a3d7bd7fd
> V' = fcbd61e7c51dd167624c56cb97b4c66d
> DT' = e6b3be782a23fa62d71d4afbb0e922fb
> Returning new block for context ffff88042ce41b18
> returning 16 from get_prng_bytes in context ffff88042ce41b18
> cprng: Test 1 passed
> getting 16 random bytes for context ffff88042ce41b18
> Calling _get_more_prng_bytes for context ffff88042ce41b18
> DT = e6b3be782a23fa62d71d4afbb0e922fb
> V = e0000000000000000000000000000000
> I = d761699cc3de4a90234c62eec2479cd9
> V^I = 3761699cc3de4a90234c62eec2479cd9
> R = 8aaa003966675be529142881a94d4ec7
> R^I = 5dcb69a5a5b911750a584a6f6b0ad21e
> V' = ef2c1fbf609a68f8fe5110636bf4bf6a
> DT' = e6b3be782a23fa62d71d4afbb0e922fc
> Returning new block for context ffff88042ce41b18
> returning 16 from get_prng_bytes in context ffff88042ce41b18
> cprng: Test 2 passed
> getting 16 random bytes for context ffff88042ce41b18
> Calling _get_more_prng_bytes for context ffff88042ce41b18
> DT = e6b3be782a23fa62d71d4afbb0e922fc
> V = f0000000000000000000000000000000
> I = 048ecb25943e8c31d614cd8a23f13f84
> V^I = f48ecb25943e8c31d614cd8a23f13f84
> R = 88dda456302423e5f69da57e7b95c73a
> R^I = 8c536f73a41aafd4208968f45864f8be
> V' = 48893b71bce400b65e21ba378a0ad570
> DT' = e6b3be782a23fa62d71d4afbb0e922fd
> Returning new block for context ffff88042ce41b18
> returning 16 from get_prng_bytes in context ffff88042ce41b18
> cprng: Test 3 passed
> getting 16 random bytes for context ffff88042ce41b18
> Calling _get_more_prng_bytes for context ffff88042ce41b18
> DT = e6b3be782a23fa62d71d4afbb0e922fd
> V = f8000000000000000000000000000000
> I = 3a6a1754bdf6f844e53662990cadc492
> V^I = c26a1754bdf6f844e53662990cadc492
> R = 052592466179d2cb78c40b140a5a9ac8
> R^I = 3f4f8512dc8f2a8f9df2698d06f75e5a
> V' = ac4b23c4fb1e85098d9927afa617ad88
> DT' = e6b3be782a23fa62d71d4afbb0e922fe
> Returning new block for context ffff88042ce41b18
> returning 16 from get_prng_bytes in context ffff88042ce41b18
> cprng: Test 4 passed
> self_test: err 0
> prng_mod_init: err 0
>
>
> >> I noticed the "unsigned char" vs "u8" issue, but didn't make the change
> >> as I didn't think the readailility improvement was worth the code churn.
>
> > You just sent a 17 patch series of clean up and were worried about code churn
> > converting an unsigned char to a u8?
>
> Yes, it does sound funny like that, but I meant that I have no trouble
> reading the types as synonyms, so the improvement is very small, and it's
> the ratio.
>
> More simply, "I noticed, but it didn't make me itch enough."
>
> Once I got deep enough into it, perhaps I should have re-evaluated.
>
> >> Should I convert all the buffers and function prototypes,
>
> > If you want to sure. u8 probably makes more sense for the buffers here as our
> > intent is to treat them as an array of byte values.
>
> I had a look at <linux/crypto.h>, which uses u8 for everything, so I've
> already changed everything to that.
>
>
> Thank you for your time and effort looking at this!
>