[PATCH 0/4] DRBG: Fixes for code review comments

[PATCH 0/4] DRBG: Fixes for code review comments

[Stephan Mueller]

Hi,

the following fixes cover comments from code reviewers. The patch set
is an addition to the patch set offered several days ago [1].

[1] https://lkml.org/lkml/2014/6/28/497

Stephan Mueller (4):
  DRBG: Select correct DRBG core for stdrng
  DRBG: Mix a time stamp into DRBG state
  DRBG: fix memory corruption for AES192
  DRBG: HMAC-SHA1 DRBG has crypto strength of 128 bits

 crypto/drbg.c         | 77 ++++++++++++++++++++++++++++++++-------------------
 include/crypto/drbg.h |  2 +-
 2 files changed, 49 insertions(+), 30 deletions(-)

-- 
1.9.3

[PATCH 1/4] DRBG: Select correct DRBG core for stdrng

[Stephan Mueller]

When the DRBG is initialized, the core is looked up using the DRBG name.
The name that can be used for the lookup is registered in
cra_driver_name. The cra_name value contains stdrng.

Thus, the lookup code must use crypto_tfm_alg_driver_name to obtain the
precise DRBG name and select the correct DRBG.

Signed-off-by: Stephan Mueller <smueller@chronox.de>
---
 crypto/drbg.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/crypto/drbg.c b/crypto/drbg.c
index 53ff20d..14495df 100644
--- a/crypto/drbg.c
+++ b/crypto/drbg.c
@@ -1764,7 +1764,7 @@ static int drbg_kcapi_init(struct crypto_tfm *tfm)
 	bool pr = false;
 	int coreref = 0;
 
-	drbg_convert_tfm_core(crypto_tfm_alg_name(tfm), &coreref, &pr);
+	drbg_convert_tfm_core(crypto_tfm_alg_driver_name(tfm), &coreref, &pr);
 	/*
 	 * when personalization string is needed, the caller must call reset
 	 * and provide the personalization string as seed information
-- 
1.9.3

[PATCH 2/4] DRBG: Mix a time stamp into DRBG state

[Stephan Mueller]

The current locking approach of the DRBG tries to keep the protected
code paths very minimal. It is therefore possible that two threads query
one DRBG instance at the same time. When thread A requests random
numbers, a shadow copy of the DRBG state is created upon which the
request for A is processed. After finishing the state for A's request is
merged back into the DRBG state. If now thread B requests random numbers
from the same DRBG after the request for thread A is received, but
before A's shadow state is merged back, the random numbers for B will be
identical to the ones for A. Please note that the time window is very
small for this scenario.

To prevent that there is even a theoretical chance for thread A and B
having the same DRBG state, the current time stamp is provided as
additional information string for each new request.

The addition of the time stamp as additional information string implies
that now all generate functions must be capable to process a linked
list with additional information strings instead of a scalar.

CC: Rafael Aquini <aquini@redhat.com>
Signed-off-by: Stephan Mueller <smueller@chronox.de>
---
 crypto/drbg.c         | 59 
++++++++++++++++++++++++++++++---------------------
 include/crypto/drbg.h |  2 +-
 2 files changed, 36 insertions(+), 25 deletions(-)

diff --git a/crypto/drbg.c b/crypto/drbg.c
index 14495df..86d483f 100644
--- a/crypto/drbg.c
+++ b/crypto/drbg.c
@@ -649,7 +649,7 @@ out:
 /* Generate function of CTR DRBG as defined in 10.2.1.5.2 */
 static int drbg_ctr_generate(struct drbg_state *drbg,
 			     unsigned char *buf, unsigned int buflen,
-			     struct drbg_string *addtl)
+			     struct list_head *addtl)
 {
 	int len = 0;
 	int ret = 0;
@@ -659,11 +659,8 @@ static int drbg_ctr_generate(struct drbg_state *drbg,
 	memset(drbg->scratchpad, 0, drbg_blocklen(drbg));
 
 	/* 10.2.1.5.2 step 2 */
-	if (addtl && 0 < addtl->len) {
-		LIST_HEAD(addtllist);
-
-		list_add_tail(&addtl->list, &addtllist);
-		ret = drbg_ctr_update(drbg, &addtllist, 2);
+	if (addtl && !list_empty(addtl)) {
+		ret = drbg_ctr_update(drbg, addtl, 2);
 		if (ret)
 			return 0;
 	}
@@ -780,7 +777,7 @@ static int drbg_hmac_update(struct drbg_state *drbg, 
struct list_head *seed,
 static int drbg_hmac_generate(struct drbg_state *drbg,
 			      unsigned char *buf,
 			      unsigned int buflen,
-			      struct drbg_string *addtl)
+			      struct list_head *addtl)
 {
 	int len = 0;
 	int ret = 0;
@@ -788,11 +785,8 @@ static int drbg_hmac_generate(struct drbg_state *drbg,
 	LIST_HEAD(datalist);
 
 	/* 10.1.2.5 step 2 */
-	if (addtl && 0 < addtl->len) {
-		LIST_HEAD(addtllist);
-
-		list_add_tail(&addtl->list, &addtllist);
-		ret = drbg_hmac_update(drbg, &addtllist, 1);
+	if (addtl && !list_empty(addtl)) {
+		ret = drbg_hmac_update(drbg, addtl, 1);
 		if (ret)
 			return ret;
 	}
@@ -816,14 +810,10 @@ static int drbg_hmac_generate(struct drbg_state *drbg,
 	}
 
 	/* 10.1.2.5 step 6 */
-	if (addtl && 0 < addtl->len) {
-		LIST_HEAD(addtllist);
-
-		list_add_tail(&addtl->list, &addtllist);
-		ret = drbg_hmac_update(drbg, &addtllist, 1);
-	} else {
+	if (addtl && !list_empty(addtl))
+		ret = drbg_hmac_update(drbg, addtl, 1);
+	else
 		ret = drbg_hmac_update(drbg, NULL, 1);
-	}
 	if (ret)
 		return ret;
 
@@ -947,7 +937,7 @@ out:
 
 /* processing of additional information string for Hash DRBG */
 static int drbg_hash_process_addtl(struct drbg_state *drbg,
-				   struct drbg_string *addtl)
+				   struct list_head *addtl)
 {
 	int ret = 0;
 	struct drbg_string data1, data2;
@@ -958,7 +948,7 @@ static int drbg_hash_process_addtl(struct drbg_state 
*drbg,
 	memset(drbg->scratchpad, 0, drbg_blocklen(drbg));
 
 	/* 10.1.1.4 step 2 */
-	if (!addtl || 0 == addtl->len)
+	if (!addtl || list_empty(addtl))
 		return 0;
 
 	/* 10.1.1.4 step 2a */
@@ -966,7 +956,7 @@ static int drbg_hash_process_addtl(struct drbg_state 
*drbg,
 	drbg_string_fill(&data2, drbg->V, drbg_statelen(drbg));
 	list_add_tail(&data1.list, &datalist);
 	list_add_tail(&data2.list, &datalist);
-	list_add_tail(&addtl->list, &datalist);
+	list_splice_tail(addtl, &datalist);
 	ret = drbg_kcapi_hash(drbg, NULL, drbg->scratchpad, &datalist);
 	if (ret)
 		goto out;
@@ -1032,7 +1022,7 @@ out:
 /* generate function for Hash DRBG as defined in  10.1.1.4 */
 static int drbg_hash_generate(struct drbg_state *drbg,
 			      unsigned char *buf, unsigned int buflen,
-			      struct drbg_string *addtl)
+			      struct list_head *addtl)
 {
 	int len = 0;
 	int ret = 0;
@@ -1350,6 +1340,12 @@ static int drbg_generate(struct drbg_state *drbg,
 {
 	int len = 0;
 	struct drbg_state *shadow = NULL;
+	LIST_HEAD(addtllist);
+	struct drbg_string timestamp;
+	union {
+		cycles_t cycles;
+		unsigned char char_cycles[sizeof(cycles_t)];
+	} now;
 
 	if (0 == buflen || !buf) {
 		pr_devel("DRBG: no output buffer provided\n");
@@ -1410,8 +1406,23 @@ static int drbg_generate(struct drbg_state *drbg,
 		/* 9.3.1 step 7.4 */
 		addtl = NULL;
 	}
+
+	/*
+	 * Mix the time stamp into the DRBG state if the DRBG is not in
+	 * test mode. If there are two callers invoking the DRBG at the same
+	 * time, i.e. before the first caller merges its shadow state back,
+	 * both callers would obtain the same random number stream without
+	 * changing the state here.
+	 */
+	if (!drbg->test_data) {
+		now.cycles = random_get_entropy();
+		drbg_string_fill(&timestamp, now.char_cycles, 
sizeof(cycles_t));
+		list_add_tail(&timestamp.list, &addtllist);
+	}
+	if (addtl && 0 < addtl->len)
+		list_add_tail(&addtl->list, &addtllist);
 	/* 9.3.1 step 8 and 10 */
-	len = shadow->d_ops->generate(shadow, buf, buflen, addtl);
+	len = shadow->d_ops->generate(shadow, buf, buflen, &addtllist);
 
 	/* 10.1.1.4 step 6, 10.1.2.5 step 7, 10.2.1.5.2 step 7 */
 	shadow->reseed_ctr++;
diff --git a/include/crypto/drbg.h b/include/crypto/drbg.h
index 4065dfc..831d786 100644
--- a/include/crypto/drbg.h
+++ b/include/crypto/drbg.h
@@ -102,7 +102,7 @@ struct drbg_state_ops {
 		      int reseed);
 	int (*generate)(struct drbg_state *drbg,
 			unsigned char *buf, unsigned int buflen,
-			struct drbg_string *addtl);
+			struct list_head *addtl);
 	int (*crypto_init)(struct drbg_state *drbg);
 	int (*crypto_fini)(struct drbg_state *drbg);
 
-- 
1.9.3

[PATCH 3/4] DRBG: fix memory corruption for AES192

[Stephan Mueller]

For the CTR DRBG, the drbg_state->scratchpad temp buffer (i.e. the
memory location immediately before the drbg_state->tfm variable
is the buffer that the BCC function operates on. BCC operates
blockwise. Making the temp buffer drbg_statelen(drbg) in size is
sufficient when the DRBG state length is a multiple of the block
size. For AES192 this is not the case and the length for temp is
insufficient (yes, that also means for such ciphers, the final
output of all BCC rounds are truncated before used to update the
state of the DRBG!!).

The patch enlarges the temp buffer from drbg_statelen to
drbg_statelen + drbg_blocklen to have sufficient space.

Reported-by: Fengguang Wu <fengguang.wu@intel.com>
Signed-off-by: Stephan Mueller <smueller@chronox.de>
---
 crypto/drbg.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/crypto/drbg.c b/crypto/drbg.c
index 86d483f..c2f02b7 100644
--- a/crypto/drbg.c
+++ b/crypto/drbg.c
@@ -441,8 +441,16 @@ static int drbg_ctr_bcc(struct drbg_state *drbg,
  *		length: drbg_blocklen(drbg)
  *	temp
  *		start: iv + drbg_blocklen(drbg)
- *		length: (drbg_keylen(drbg) + drbg_blocklen(drbg) ==
- *				drbg_statelen(drbg))
+ *		length: drbg_satelen(drbg) + drbg_blocklen(drbg)
+ *			note: temp is the buffer that the BCC function 
operates
+ *			on. BCC operates blockwise. drbg_statelen(drbg)
+ *			is sufficient when the DRBG state length is a multiple
+ *			of the block size. For AES192 (and maybe other 
ciphers)
+ *			this is not correct and the length for temp is
+ *			insufficient (yes, that also means for such ciphers,
+ *			the final output of all BCC rounds are truncated).
+ *			Therefore, add drbg_blocklen(drbg) to cover all
+ *			possibilities.
  */
 
 /* Derivation Function for CTR DRBG as defined in 10.4.2 */
@@ -1214,7 +1222,7 @@ static inline int drbg_alloc_state(struct drbg_state 
*drbg)
 			  drbg_statelen(drbg) +	/* df_data */
 			  drbg_blocklen(drbg) +	/* pad */
 			  drbg_blocklen(drbg) +	/* iv */
-			  drbg_statelen(drbg);	/* temp */
+			  drbg_statelen(drbg) + drbg_blocklen(drbg); /* temp 
*/
 	else
 		sb_size = drbg_statelen(drbg) + drbg_blocklen(drbg);
 
-- 
1.9.3

Re: [PATCH 3/4] DRBG: fix memory corruption for AES192

[Herbert Xu]

On Tue, Jul 01, 2014 at 05:08:48PM +0200, Stephan Mueller wrote:
> For the CTR DRBG, the drbg_state->scratchpad temp buffer (i.e. the
> memory location immediately before the drbg_state->tfm variable
> is the buffer that the BCC function operates on. BCC operates
> blockwise. Making the temp buffer drbg_statelen(drbg) in size is
> sufficient when the DRBG state length is a multiple of the block
> size. For AES192 this is not the case and the length for temp is
> insufficient (yes, that also means for such ciphers, the final
> output of all BCC rounds are truncated before used to update the
> state of the DRBG!!).
> 
> The patch enlarges the temp buffer from drbg_statelen to
> drbg_statelen + drbg_blocklen to have sufficient space.
> 
> Reported-by: Fengguang Wu <fengguang.wu@intel.com>
> Signed-off-by: Stephan Mueller <smueller@chronox.de>

I have applied just this patch out of your series.  You patches
depend on the previous four patches which I have not yet applied
since there are still outstanding issues with two of them.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 3/4] DRBG: fix memory corruption for AES192

[Stephan Mueller]

Am Freitag, 4. Juli 2014, 11:08:10 schrieb Herbert Xu:

Hi Herbert,

> On Tue, Jul 01, 2014 at 05:08:48PM +0200, Stephan Mueller wrote:
> > For the CTR DRBG, the drbg_state->scratchpad temp buffer (i.e. the
> > memory location immediately before the drbg_state->tfm variable
> > is the buffer that the BCC function operates on. BCC operates
> > blockwise. Making the temp buffer drbg_statelen(drbg) in size is
> > sufficient when the DRBG state length is a multiple of the block
> > size. For AES192 this is not the case and the length for temp is
> > insufficient (yes, that also means for such ciphers, the final
> > output of all BCC rounds are truncated before used to update the
> > state of the DRBG!!).
> > 
> > The patch enlarges the temp buffer from drbg_statelen to
> > drbg_statelen + drbg_blocklen to have sufficient space.
> > 
> > Reported-by: Fengguang Wu <fengguang.wu@intel.com>
> > Signed-off-by: Stephan Mueller <smueller@chronox.de>
> 
> I have applied just this patch out of your series.  You patches
> depend on the previous four patches which I have not yet applied
> since there are still outstanding issues with two of them.

Thank you.

The raised issues are for patches [1] and [2].

The issue around [1] is whether there is Kconfig solution for the problem of 
selecting at least one or more than one configure option from a set of 
options. This should avoid having a module init function with returns an 
error. The selection of DRBG cores shall allow the user to compile the DRBG to 
suit his needs and reduce the footprint of the DRBG. Since at least one DRBG 
core must be selected as otherwise the DRBG is meaningless. Unfortunately I 
have not found a solution with Kconfig and all given suggestions also did not 
lead to a solution. So, if anybody has a suggestion on how to solve the issue, 
I will try it out.

Regarding [2], the issue is that ARRAY_SIZE returns different variable types 
on different platforms (e.g. unsigned long on x86_64 vs unsigned int on i386). 
Thus I have to use a type cast. This issue may be fixed by forcing a 
particular type to ARRAY_SIZE. Such change, however, may have quite some 
ripple effects throughout the kernel. Therefore, IMHO such a patch should not 
be made in the realm of the DRBG patchset and the type cast should be allowed.

[1] https://lkml.org/lkml/2014/6/28/489

[2] https://lkml.org/lkml/2014/6/28/492

-- 
Ciao
Stephan

Re: [PATCH 3/4] DRBG: fix memory corruption for AES192

[Herbert Xu]

On Tue, Jul 01, 2014 at 05:08:48PM +0200, Stephan Mueller wrote:
> For the CTR DRBG, the drbg_state->scratchpad temp buffer (i.e. the
> memory location immediately before the drbg_state->tfm variable
> is the buffer that the BCC function operates on. BCC operates
> blockwise. Making the temp buffer drbg_statelen(drbg) in size is
> sufficient when the DRBG state length is a multiple of the block
> size. For AES192 this is not the case and the length for temp is
> insufficient (yes, that also means for such ciphers, the final
> output of all BCC rounds are truncated before used to update the
> state of the DRBG!!).
> 
> The patch enlarges the temp buffer from drbg_statelen to
> drbg_statelen + drbg_blocklen to have sufficient space.
> 
> Reported-by: Fengguang Wu <fengguang.wu@intel.com>
> Signed-off-by: Stephan Mueller <smueller@chronox.de>

BTW your patches were all corrupted by your mailer so I had to
fix them by hand.  Please check the cryptodev tree to ensure
that my fixes are correct.

In future please test your patches by applying your own patches
returned via the list.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 3/4] DRBG: fix memory corruption for AES192

[Stephan Mueller]

Am Freitag, 4. Juli 2014, 11:12:35 schrieb Herbert Xu:

Hi Herbert,

> On Tue, Jul 01, 2014 at 05:08:48PM +0200, Stephan Mueller wrote:
> > For the CTR DRBG, the drbg_state->scratchpad temp buffer (i.e. the
> > memory location immediately before the drbg_state->tfm variable
> > is the buffer that the BCC function operates on. BCC operates
> > blockwise. Making the temp buffer drbg_statelen(drbg) in size is
> > sufficient when the DRBG state length is a multiple of the block
> > size. For AES192 this is not the case and the length for temp is
> > insufficient (yes, that also means for such ciphers, the final
> > output of all BCC rounds are truncated before used to update the
> > state of the DRBG!!).
> > 
> > The patch enlarges the temp buffer from drbg_statelen to
> > drbg_statelen + drbg_blocklen to have sufficient space.
> > 
> > Reported-by: Fengguang Wu <fengguang.wu@intel.com>
> > Signed-off-by: Stephan Mueller <smueller@chronox.de>
> 
> BTW your patches were all corrupted by your mailer so I had to
> fix them by hand.  Please check the cryptodev tree to ensure
> that my fixes are correct.

The patch fixing the memory corruption is completely and correctly applied.

Apologies for sending broken patches. If I shall resend them corrected, please 
let me know.
> 
> In future please test your patches by applying your own patches
> returned via the list.

I will do that.

-- 
Ciao
Stephan

[PATCH 4/4] DRBG: HMAC-SHA1 DRBG has crypto strength of 128 bits

[Stephan Mueller]

The patch corrects the security strength of the HMAC-SHA1 DRBG to 128
bits. This strength defines the size of the seed required for the DRBG.
Thus, the patch lowers the seeding requirement from 256 bits to 128 bits
for HMAC-SHA1.

Signed-off-by: Stephan Mueller <smueller@chronox.de>
---
 crypto/drbg.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/crypto/drbg.c b/crypto/drbg.c
index c2f02b7..e6c52e0 100644
--- a/crypto/drbg.c
+++ b/crypto/drbg.c
@@ -195,7 +195,7 @@ static const struct drbg_core drbg_cores[] = {
 #endif /* CONFIG_CRYPTO_DRBG_HASH */
 #ifdef CONFIG_CRYPTO_DRBG_HMAC
 	{
-		.flags = DRBG_HMAC | DRBG_STRENGTH256,
+		.flags = DRBG_HMAC | DRBG_STRENGTH128,
 		.statelen = 20, /* block length of cipher */
 		.max_addtllen = 35,
 		.max_bits = 19,
-- 
1.9.3