| Message ID | 20141202083550.17918.qmail@ns.horizon.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Herbert Xu |
| Headers | show |
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 <linux@horizon.com> >--- > 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From smueller@chronox.de Tue Dec 02 08:57:23 2014 X-AuthUser: sm@eperm.de From: Stephan Mueller <smueller@chronox.de> To: George Spelvin <linux@horizon.com> Cc: herbert@gondor.apana.org.au, nhorman@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.qmail@ns.horizon.com> References: <20141202083550.17918.qmail@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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Dec 02, 2014 at 03:35:50AM -0500, George Spelvin wrote: > It's simply not necessary. > > Signed-off-by: George Spelvin <linux@horizon.com> 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 */
It's simply not necessary. Signed-off-by: George Spelvin <linux@horizon.com> --- crypto/ansi_cprng.c | 28 +++++++++++----------------- 1 file changed, 11 insertions(+), 17 deletions(-)