[PATCH 00/17] Multiple changes to crypto/ansi_cprng.c
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 01/17] crypto: ansi_cprng - Rename rand_data_valid more sensibly
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
---
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
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 02/17] crypto: ansi_cprng - Eliminate ctx->last_rand_data
It's simply not necessary.
Signed-off-by: George Spelvin
---
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
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 03/17] crypto: ansi_cprng - Eliminate ctx->I
It's also not necessary. We do have to change some debugging
output.
Signed-off-by: George Spelvin
---
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
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH 04/17] crypto: ansi_cprng - simplify xor_vectors() to xor_block()
It doesn't need a second input or a length parameter.
Signed-off-by: George Spelvin
---
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
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 05/17] crypto: ansi_cprng - Add const annotations to hexdump()
So I can pass the "input" variable to it.
Signed-off-by: George Spelvin
---
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
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 06/17] crypto: ansi_cprng - Eliminate unused PRNG_FIXED_SIZE flag
There's no way to set it, so it's dead code.
Signed-off-by: George Spelvin
---
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
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/17] crypto: ansi_cprng - Rename rand_data_valid more sensibly
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
>---
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 07/17] crypto: ansi_cprng - Shrink rand_read_pos & flags
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
---
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
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 08/17] crypto: ansi_cprng - Require non-null key & V in reset_prng_context
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
---
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
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 09/17] crypto: ansi_cprng - Clean up some variable types
Add a few const annotations, use unsigned bytes consistently, and
make do_cont_test a bool.
Signed-off-by: George Spelvin
---
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
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 10/17] crypto: ansi_cprng - simplify get_prng_bytes
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
---
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
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 11/17] crypto: ansi_cprng - unroll _get_more_prng_bytes
It's more legible, and the code is 15 bytes smaller (i386).
Signed-off-by: George Spelvin
---
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
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.ke
[PATCH 13/17] crypto: ansi_cprng - If DT is not provided, use a fresh timestamp
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
---
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
*
@@ -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
@@ -41,8 +43,8 @@ union block {
/*
* Flags for the prng_context flags field
*/
-
-#define PRNG_NEED_RESET 0x1
+#define PRNG_NEED_RESET0x1
+#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();
[PATCH 12/17] crypto: ansi_cprng - Create a "block buffer" data type
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
---
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, c
Re: [PATCH 02/17] crypto: ansi_cprng - Eliminate ctx->last_rand_data
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
>---
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 14/17] crypto: ansi_cprng - If DT is omitted, don't buffer old output
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 --- 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 -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 15/17] crypto: testmgr - Teach test_cprng to handle non-default seed sizes
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
---
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
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 16/17] crypto: testmgr - Merge seed arrays in struct cprng_testvec
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
---
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;
- unsig
[PATCH 17/17] crypto: ansi_cprng - Shrink default seed size
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
---
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
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 02/17] crypto: ansi_cprng - Eliminate ctx->last_rand_data
>From smuel...@chronox.de Tue Dec 02 08:57:23 2014 X-AuthUser: s...@eperm.de From: Stephan Mueller To: George Spelvin Cc: herb...@gondor.apana.org.au, nhor...@tuxdriver.com, linux-crypto@vger.kernel.org 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: <20141202083550.17918.qm...@ns.horizon.com> References: <20141202083550.17918.qm...@ns.horizon.com> 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. -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 13/17] crypto: ansi_cprng - If DT is not provided, use a fresh timestamp
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. -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
re: crypto: qat - Intel(R) QAT transport code
Hello Tadeusz Struk, The patch a672a9dc872e: "crypto: qat - Intel(R) QAT transport code" from Jun 5, 2014, leads to the following static checker warning: drivers/crypto/qat/qat_common/adf_transport.c:412 adf_init_bank() error: potentially using uninitialized 'coalesc_enabled'. drivers/crypto/qat/qat_common/adf_transport.c 407 /* Enable IRQ coalescing always. This will allow to use 408 * the optimised flag and coalesc register. 409 * If it is disabled in the config file just use min time value */ 410 if (adf_get_cfg_int(accel_dev, "Accelerator0", 411 ADF_ETRMGR_COALESCING_ENABLED_FORMAT, 412 bank_num, &coalesc_enabled) && coalesc_enabled) ^^^ This condition is reversed, so it only enables coalescing on error. Probably this feature has not been tested yet. 413 adf_enable_coalesc(bank, "Accelerator0", bank_num); 414 else 415 bank->irq_coalesc_timer = ADF_COALESCING_MIN_TIME; 416 regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Is ansi_cprng.c supposed to be an implmentation of X9.31?
On Mon, Dec 01, 2014 at 11:55:18PM -0500, George Spelvin wrote: > >> The other one, which I have to think *very* hard about, is whether > >> using the same seed material as /dev/random in this much weaker > >> cryptographic construction will introduce a flaw in *it*. > > > I have no idea what you are referring to here. The caller is requred to > > seed the DRNG. Typically that is done with a call to get_random_bytes. > > As Neil mentioned, the X9.31 DRNG implementation does not seed itself. > > Er, yes it does. Not the key, but the IV vector V[]. > > Here's the closest thing I can find on-line to a direct quote > from the ANSI Spec: > > http://www.untruth.org/~josh/security/ansi931rng.PDF > > "Let DT be a date/time vector which is updated on each iteration." > > This timestamp is a source of continuous reseed material. The spec > doesn't impose specific resolution requirements, but it's hopefully > obvious that the finer, the better. The goal is a vector which changes > per iteration. > That is an old version. The updated version (published in 2005), and specified in the ansi_cprng.c file removes that language. > There are limitations due to its finite entropy and lack of catastrophic > reseeding, as Kelsey & Schneier pointed out: > > https://www.schneier.com/paper-prngs.html > > but it is intended as an entropy source. > Again, that language is removed in the more recent version. Using DT as an entropy source, and updating it on each iteration also destroys the predictive nature of the cprng when two peers use it to communicate. That is to say, if a DT vector is updated every iteration, and the resultant R value is used as a key to encrypt data, the validitiy of that key at a peer host using an identically seeded cprng to decrypt is limited by the granularity of the DT value. That is to say, if you update DT every second in cprng, an entity that knows the secret key can only decrypt that data up to a second after its encryption, unless the decrypting entity also knows at exactly what time the data was encrypted. If you share the DT value, any entropy it provides becomes worthless, as it becomes widely known. The long and the short of it is that, if you want a cprng who's output can be predicted by any entity with the IV and KEY values, then DT has to be known initially and updated in a predictable fashion that is independent of the data being transmitted. Using a real date/time vector can't do that. > Hopefully very soon I'll have a patch series to post. (Unfortunately, > I just managed to oops my kernel and need to reboot to continue testing.) > > If I don't do much more, it'll be patch 13/17 in the series. I think > the use of get_random_int() is a good compromise between the raw > random_get_entropy() timestamp and the (too heavy) get_random_bytes(). > -- > To unsubscribe from this list: send the line "unsubscribe linux-crypto" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Is ansi_cprng.c supposed to be an implmentation of X9.31?
On Tue, Dec 02, 2014 at 12:39:30AM -0500, George Spelvin wrote:
> > See Documentation/DocBook/crypto-API.tmpl in the cryptodev-2.6 tree.
> > There you will find tons of documentation (which will be merged during
> > 3.19-rc1)
>
> Yes, I've been reading that. It certainly helps a great deal, but
> still leaves me with some significant questions.
>
> I started researching the crypto layer when I proposed using Dan
> Bernstein's SipHash elsewhere in the kernel and someone asked for a
> crypto API wrapper for it. That seemed a simple enough request to me,
> but it's been a deeper rabbit hole than I expected.
>
> I started reading the code to another keyed hash, michael_mic, as a model,
> but I'm stil trying to understand the intended difference between "struct
> crypto_shash" and "struct shash_desc", and in particular why both have
> a copy of the key. The SHASH API documentation at
>
> https://git.kernel.org/cgit/linux/kernel/git/herbert/cryptodev-2.6.git/tree/include/crypto/hash.h
>
> isn't particularly enlightening. If the crypto_shash were entirely
> read-only and the shash_desc were the entire volatile state, that would
> make sense, but as it is I'm confused about the design intent.
>
Not sure what you're concerned about, or what you're even referencing. A
struct crypto_shash is the object retured by crypto_alloc_shash, and represents
an instance of a synchronous hash algorithm:
struct crypto_shash {
unsigned int descsize;
struct crypto_tfm base;
};
The key is stored in the base.__crt_ctx part of the structure in a algorithm
specific manner.
The shash_desc structure represents a discrete block of data that is being
hashed. It can be updated with new data, reset, finalized, etc. It only points
to the crypto_hash structure that it is associated with (as it must, given that
the crypto layer needs to know what algorithm to use to hash the data and what
key to use). But it doesn't store a second copy of the key on its own.
> (On a related point, the general lack of const declarations throughout the
> crypto layer has been a source of frustration.)
>
So fix it. Making large claims about what frustrates you about the code without
producing any fixes doesn't make people listen to you.
>
> The other big issue I'm struggling with is how to get the tcrypt.ko module
> to print "ansi_cprng has passed its tests." All it has produced for me
> so far is a few kilobytes of dmesg spam about ciphers which aren't even
> configured in my kernel.
>
The tests only print out a pass fail notification in fips mode, that seems
pretty clear if you look at the alg_test function.
> After a few hours of trying to figure out what the alg and type parameters
> do, I gave up and cut and pasted the tests into prng_mod_init().
They're used to differentiate algorithms that can be used for multiple purposes.
See the CRYPTO_ALG_TYPE_* defines in crypto.h. For the CPRNG, they do nothing
since an RNG is only an RNG.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] crypto: algif - Mark sgl end at the end of data
On Mon, Dec 01, 2014 at 07:03:00AM -0800, Tadeusz Struk wrote: > > Ok, I can look at the data, but do you think the idea with marking the > end of data in TX sgl is worthwhile or should I just forget about it. > Another question is - is an sgl with lots of empty buffers a valid input > from an algorithm implementation point of view? I think marking the end is useful. How about doing the marking and unmarking whenever sgl->cur is updated? Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 02/17] crypto: ansi_cprng - Eliminate ctx->last_rand_data
On Tue, Dec 02, 2014 at 03:35:50AM -0500, George Spelvin wrote: > It's simply not necessary. > > Signed-off-by: George Spelvin 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 -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/17] crypto: ansi_cprng - Eliminate ctx->I
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 > --- > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] crypto: algif_skcipher - initialize upon init request
On Sun, Nov 30, 2014 at 10:55:26AM +0100, Stephan Mueller wrote: > When using the algif_skcipher, the following call sequence causess a > re-initialization: > > 1. sendmsg with ALG_SET_OP and iov == NULL, iovlen == 0 (i.e > initializing the cipher, but not sending data) > > 2. sendmsg with msg->msg-controllen == 0 and iov != NULL (using the initalized > cipher handle by sending data) > > In step 2, the cipher operation type (encryption or decryption) is reset > to always decryption, because the local variable of enc is put into > ctx->enc as ctx->user is still zero. > > The same applies when all send data is processed and ctx->used falls to > zero followed by user space to send new data. > > This patch changes the behavior to only reset the cipher operation type > (and the IV) if such configuration request is received. > > Signed-off-by: Stephan Mueller Patch applied. -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/8 v2] sparc64: replace memset by memzero_explicit
On Sun, Nov 30, 2014 at 06:03:43PM +0100, Julia Lawall wrote:
> From: Julia Lawall
>
> Memset on a local variable may be removed when it is called just before the
> variable goes out of scope. Using memzero_explicit defeats this
> optimization. A simplified version of the semantic patch that makes this
> change is as follows: (http://coccinelle.lip6.fr/)
>
> //
> @@
> identifier x;
> type T;
> @@
>
> {
> ... when any
> T x[...];
> ... when any
> when exists
> - memset
> + memzero_explicit
> (x,
> -0,
> ...)
> ... when != x
> when strict
> }
> //
>
> This change was suggested by Daniel Borkmann
>
> Signed-off-by: Julia Lawall
Applied patches 2, 3, 6, 7.
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 07/17] crypto: ansi_cprng - Shrink rand_read_pos & flags
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
> ---
> 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
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] crypto: algif - Mark sgl end at the end of data
Hi, On 12/02/2014 06:33 AM, Herbert Xu wrote: > I think marking the end is useful. How about doing the marking > and unmarking whenever sgl->cur is updated? I have a v2 ready where I mark it based on the actual data. Thanks -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] crypto: algif - Mark sgl end at the end of data
Hi,
algif_skcipher sends 127 sgl buffers for encryption regardless of how
many buffers acctually have data to process, where the few first with
valid len and the rest with zero len. This is not very eficient and may cause
problems when algs do something like this without checking the buff
lenght:
for_each_sg(sgl, sg, sg_nents, i)
sg_virt(sg)
This patch marks the last one with data as the last one to process.
Changes:
v2 - use data len to find the last buffer instead of nents in RX list.
Signed-off-by: Tadeusz Struk
---
crypto/algif_skcipher.c | 22 ++
1 file changed, 22 insertions(+)
diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
index f2a88a7..cde507f 100644
--- a/crypto/algif_skcipher.c
+++ b/crypto/algif_skcipher.c
@@ -414,6 +414,23 @@ unlock:
return err ?: size;
}
+static int skcipher_get_nents_with_data(struct scatterlist *sg, int len)
+{
+ int nents;
+
+ for (nents = 0; sg; sg = sg_next(sg)) {
+ if (!sg)
+ return nents;
+
+ len -= sg->length;
+ if (!(len > 0))
+ return nents;
+ nents++;
+ }
+
+ return 0;
+}
+
static int skcipher_recvmsg(struct kiocb *unused, struct socket *sock,
struct msghdr *msg, size_t ignored, int flags)
{
@@ -437,6 +454,8 @@ static int skcipher_recvmsg(struct kiocb *unused, struct
socket *sock,
char __user *from = iov->iov_base;
while (seglen) {
+ int nents;
+
sgl = list_first_entry(&ctx->tsgl,
struct skcipher_sg_list, list);
sg = sgl->sg;
@@ -464,6 +483,8 @@ static int skcipher_recvmsg(struct kiocb *unused, struct
socket *sock,
if (!used)
goto free;
+ nents = skcipher_get_nents_with_data(sg, used);
+ sg_mark_end(&sg[nents]);
ablkcipher_request_set_crypt(&ctx->req, sg,
ctx->rsgl.sg, used,
ctx->iv);
@@ -473,6 +494,7 @@ static int skcipher_recvmsg(struct kiocb *unused, struct
socket *sock,
crypto_ablkcipher_encrypt(&ctx->req) :
crypto_ablkcipher_decrypt(&ctx->req),
&ctx->completion);
+ sg_unmark_end(&sg[nents]);
free:
af_alg_free_sg(&ctx->rsgl);
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: crypto: qat - Intel(R) QAT transport code
On 12/02/2014 04:21 AM, Dan Carpenter wrote: > drivers/crypto/qat/qat_common/adf_transport.c >407 /* Enable IRQ coalescing always. This will allow to use >408 * the optimised flag and coalesc register. >409 * If it is disabled in the config file just use min time > value */ >410 if (adf_get_cfg_int(accel_dev, "Accelerator0", >411 ADF_ETRMGR_COALESCING_ENABLED_FORMAT, >412 bank_num, &coalesc_enabled) && > coalesc_enabled) > ^^^ > This condition is reversed, so it only enables coalescing on error. Hello Dan, Yes, you are correct. Looks like we never enable interrupt coalescing. Thanks for reporting the issue. Patch fixing this will be out soon. Regards, Tadeusz -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/17] crypto: ansi_cprng - Rename rand_data_valid more sensibly
>> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Is ansi_cprng.c supposed to be an implmentation of X9.31?
> That is an old version. The updated version (published in 2005), and > specified in the ansi_cprng.c file removes that language. Oh! Thank you! I'm pretty sure I read the 1998 version. In fact, apparently there's a 2010 version: http://www.codesdownload.org/3761-ANSI-X9-TR-31-2010.html I need to figure out how to get my hands on that. Presumably this is the 2005 version with the 2009 supplement incorporated. If I could read Chinese, I might be able to find it here: http://www.docin.com/p-524511188.html > The long and the short of it is that, if you want a cprng who's output can be > predicted by any entity with the IV and KEY values, then DT has to be known > initially and updated in a predictable fashion that is independent of the data > being transmitted. Using a real date/time vector can't do that. Er, yes, this is all extremely obvious; I'm not quite sure why we're belabouring it. Fully deterministic generators have their uses, which is why I had to ask in the beginning what the design intent was. If this is *intended* to be purely deterministic, there's nothing to fix. I'd like to propose a small comment clarification because a quick reading confused me. But when I talked about making it random, you said "send a patch", so I did. If you don't want the semantic change, I'm not upset. The other code cleanups are hopefully (after I've finished polishing them) useful; just stop before the whole "union block" business. -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: crypto: qat - Intel(R) QAT transport code
On 12/02/2014 08:49 AM, Tadeusz Struk wrote: > On 12/02/2014 04:21 AM, Dan Carpenter wrote: >> drivers/crypto/qat/qat_common/adf_transport.c >>407 /* Enable IRQ coalescing always. This will allow to use >>408 * the optimised flag and coalesc register. >>409 * If it is disabled in the config file just use min time >> value */ >>410 if (adf_get_cfg_int(accel_dev, "Accelerator0", >>411 ADF_ETRMGR_COALESCING_ENABLED_FORMAT, >>412 bank_num, &coalesc_enabled) && >> coalesc_enabled) >> ^^^ >> This condition is reversed, so it only enables coalescing on error. > > Hello Dan, > Yes, you are correct. Looks like we never enable interrupt coalescing. > Thanks for reporting the issue. Patch fixing this will be out soon. > Regards, > Tadeusz > Hi, I had to take a closer look to see what's going on there. We do enable coalescing always, just as the comment states (in function adf_enable_ring_irq(), line 105). The adf_enable_coalesc() function only reads the coalescing timer from the configuration and stores it in the bank->irq_coalesc_timer. Your point is still valid - the condition is reversed so we have always used ADF_COALESCING_MIN_TIME. What's also missing is initialization of coalesc_enabled and the adf_enable_coalesc() function should really be called adf_get_coalesc_timer() I'll send a patch to fix it. Thanks again. Regards, Tadeusz -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Is ansi_cprng.c supposed to be an implmentation of X9.31?
> Not sure what you're concerned about, or what you're even referencing.
Sorry for the confusion, but it's genuine. See below for what I think is
the solution.
> The shash_desc structure represents a discrete block of data that is
> being hashed. It can be updated with new data, reset, finalized, etc.
> It only points to the crypto_hash structure that it is associated with
> (as it must, given that the crypto layer needs to know what algorithm
> to use to hash the data and what key to use). But it doesn't store a
> second copy of the key on its own.
Er, I saw the following code:
static int michael_setkey(struct crypto_shash *tfm, const u8 *key,
unsigned int keylen)
{
struct michael_mic_ctx *mctx = crypto_shash_ctx(tfm);
const __le32 *data = (const __le32 *)key;
if (keylen != 8) {
crypto_shash_set_flags(tfm, CRYPTO_TFM_RES_BAD_KEY_LEN);
return -EINVAL;
}
mctx->l = le32_to_cpu(data[0]);
mctx->r = le32_to_cpu(data[1]);
return 0;
}
and thought "okay, the key is in mctx->l and mctx->r."
Then I saw
static int michael_init(struct shash_desc *desc)
{
struct michael_mic_desc_ctx *mctx = shash_desc_ctx(desc);
struct michael_mic_ctx *ctx = crypto_shash_ctx(desc->tfm);
mctx->pending_len = 0;
mctx->l = ctx->l;
mctx->r = ctx->r;
return 0;
}
and thought "the key is being copied from the ctx to the desc. Why
the heck are they doing it in two steps?"
I spent a long time trying to figure out why there were two separate
layers, as it looked like
struct michael_mic_ctx {
u32 l, r;
};
was a strict subset of
struct michael_mic_desc_ctx {
u8 pending[4];
size_t pending_len;
u32 l, r;
};
and thus the former was unnecessary.
(Another code optimization that jumps out at me: given that pending_len
is strictly between 0 and 3, using a 64-bit size_t unnecessarily bloats
the context structure to 24 bytes. Shrinking it to 32 bits would reduce
the context to 16 bytes, and given that at most three bytes of pending[]
are ever used, except transiently in the update() function, pending_len
could be stored in pending[3] and and the context shrunk to 12 bytes.)
I'm thinking that the confusion comes from my unfortunate choice of
which file to start reading and the peculiar way that michael_mic's key
is more like an IV, so there is neither key scheduling nor persistent
access to it.
== My guess as to how this works ==
(If I say anything wrong, please correct me!)
A ctx is an abstraction of a (scheduled) key. With symmetric ciphers,
it's very common to have a complex key set up (I'm looking at you, twofish)
which can be re-used for multiple messages/packets/whatever.
It has to be an opaque object because it might correspond to some
state in a hardware acclerator, with the software swapping keys in
and out like virtual memory.
A desc is the abstraction of an IV. It handles chaining and block
boundaries, to encrypt/hash/transform a stream/bvec/sk_buff of data.
The reason for the separation is that multiple descs may simultaneously
access the same ctx.
This is okay because the ctx reference is conceptually read-only.
(If there's swapping of hardware contexts going on behind the curtain,
the ctx may not be entirely immutable, but it's like a cache: you're
not supposed to notice the difference.)
There are some cases, like ECB, where a desc is a ridiculously thin
wrapper and seems like overkill, but it's there to provide a consistent
interface.
(Design choice: a single ctx can handle both encryption and decryption.
For algorithms that have different key schedules for the two directions,
it has to waste the space even if the cipher is only being used forward.)
>> (On a related point, the general lack of const declarations throughout the
>> crypto layer has been a source of frustration.)
> So fix it. Making large claims about what frustrates you about the
> code without producing any fixes doesn't make people listen to you.
I'm happy to! It's just a large, invasive, change and I wonder how welcome
it is. Maybe there's a reason for the omission that I'm not seeing?
You know the aphorism "never attribute to malice that which can be
adequately explained by stupidity"? Well, there's a follow-on, which
says be careful attributing to someone else's stupidity that which
can be adequately explained by your own.
It's a pattern I go through frequently, and I think others do too:
my initial reaction is "what the @#$@# is this $#!+?", but I try to
consider whether the fault is actually mine before speaking aloud (or
hitting "send", as the case may be).
If it's soemthing that annoys you too and you just haven't gotten around
to fixing it, I'd be delighted!
I just wanted to start with something smaller in scope, confined to
a single source file, where I felt like I understood the implications
better.
>> The other big issue I'm s
Re: [PATCH 02/17] crypto: ansi_cprng - Eliminate ctx->last_rand_data
> 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. -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/17] crypto: ansi_cprng - Eliminate ctx->I
> 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. -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 07/17] crypto: ansi_cprng - Shrink rand_read_pos & flags
>> --- 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 SizeOffset Size
0 20 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 SizeOffset 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 11/17] crypto: ansi_cprng - unroll _get_more_prng_bytes
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. -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] crypto: qat - fix problem with coalescing enable logic
Fixed issue reported by Dan Carpenter
410 if (adf_get_cfg_int(accel_dev, "Accelerator0",
411 ADF_ETRMGR_COALESCING_ENABLED_FORMAT,
412 bank_num, &coalesc_enabled) && coalesc_enabled)
This condition is reversed, so it only enables coalescing on error.
Reported-by: Dan Carpenter
Signed-off-by: Tadeusz Struk
---
drivers/crypto/qat/qat_common/adf_transport.c | 15 ---
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/drivers/crypto/qat/qat_common/adf_transport.c
b/drivers/crypto/qat/qat_common/adf_transport.c
index 5f3fa45..5890992 100644
--- a/drivers/crypto/qat/qat_common/adf_transport.c
+++ b/drivers/crypto/qat/qat_common/adf_transport.c
@@ -376,8 +376,9 @@ static inline int adf_get_cfg_int(struct adf_accel_dev
*accel_dev,
return 0;
}
-static void adf_enable_coalesc(struct adf_etr_bank_data *bank,
- const char *section, uint32_t bank_num_in_accel)
+static void adf_get_coalesc_timer(struct adf_etr_bank_data *bank,
+ const char *section,
+ uint32_t bank_num_in_accel)
{
if (adf_get_cfg_int(bank->accel_dev, section,
ADF_ETRMGR_COALESCE_TIMER_FORMAT,
@@ -396,7 +397,7 @@ static int adf_init_bank(struct adf_accel_dev *accel_dev,
struct adf_hw_device_data *hw_data = accel_dev->hw_device;
struct adf_etr_ring_data *ring;
struct adf_etr_ring_data *tx_ring;
- uint32_t i, coalesc_enabled;
+ uint32_t i, coalesc_enabled = 0;
memset(bank, 0, sizeof(*bank));
bank->bank_number = bank_num;
@@ -407,10 +408,10 @@ static int adf_init_bank(struct adf_accel_dev *accel_dev,
/* Enable IRQ coalescing always. This will allow to use
* the optimised flag and coalesc register.
* If it is disabled in the config file just use min time value */
- if (adf_get_cfg_int(accel_dev, "Accelerator0",
- ADF_ETRMGR_COALESCING_ENABLED_FORMAT,
- bank_num, &coalesc_enabled) && coalesc_enabled)
- adf_enable_coalesc(bank, "Accelerator0", bank_num);
+ if ((adf_get_cfg_int(accel_dev, "Accelerator0",
+ADF_ETRMGR_COALESCING_ENABLED_FORMAT, bank_num,
+&coalesc_enabled) == 0) && coalesc_enabled)
+ adf_get_coalesc_timer(bank, "Accelerator0", bank_num);
else
bank->irq_coalesc_timer = ADF_COALESCING_MIN_TIME;
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
