[PATCH 0/4] crypto: drbg - revamp locking

[PATCH 0/4] crypto: drbg - revamp locking

[Stephan Mueller]

Hi,

the current implementation of the DRBG generates a shadow copy of its
DRBG state for each incoming request. The idea is that only a short
term lock is needed to spawn the shadow copy. The drawback is that
if multiple parallel requests come in, the generated DRBG shadow
states only differ by a high-resolution timer that was mixed in during
the shadow state generation.

This patch now removes this shadow state and introduces a mutex
to serialize all requests to one DRBG instance.

The patch was fully CAVS tested and demonstrates that the DRBG still
complies with the standard.

Stephan Mueller (4):
  cryoto: drbg - clear all temporary memory
  crypto: drbg - do not create shadow copy
  crypto: drbg - replace spinlock with mutex
  crypto: drbg - leave cipher handles operational

 crypto/drbg.c         | 154 +++++++++-----------------------------------------
 include/crypto/drbg.h |   4 +-
 2 files changed, 30 insertions(+), 128 deletions(-)

-- 
2.1.0

[PATCH 1/4] cryoto: drbg - clear all temporary memory

[Stephan Mueller]

The buffer uses for temporary data must be cleared entirely. In AES192
the used buffer is drbg_statelen(drbg) + drbg_blocklen(drbg) as
documented in the comment above drbg_ctr_df.

This patch ensures that the temp buffer is completely wiped.

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 b69409c..8d2944f 100644
--- a/crypto/drbg.c
+++ b/crypto/drbg.c
@@ -487,7 +487,7 @@ static int drbg_ctr_df(struct drbg_state *drbg,
 
 out:
 	memset(iv, 0, drbg_blocklen(drbg));
-	memset(temp, 0, drbg_statelen(drbg));
+	memset(temp, 0, drbg_statelen(drbg) + drbg_blocklen(drbg));
 	memset(pad, 0, drbg_blocklen(drbg));
 	return ret;
 }
-- 
2.1.0

Re: [PATCH 1/4] cryoto: drbg - clear all temporary memory

[Herbert Xu]

On Fri, Apr 17, 2015 at 02:54:08PM +0200, Stephan Mueller wrote:
> The buffer uses for temporary data must be cleared entirely. In AES192
> the used buffer is drbg_statelen(drbg) + drbg_blocklen(drbg) as
> documented in the comment above drbg_ctr_df.
> 
> This patch ensures that the temp buffer is completely wiped.
> 
> Signed-off-by: Stephan Mueller <smueller@chronox.de>

Patch applied.
-- 
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

[PATCH 2/4] crypto: drbg - do not create shadow copy

[Stephan Mueller]

The creation of a shadow copy is intended to only hold a short term
lock. But the drawback is that parallel users have a very similar DRBG
state which only differs by a high-resolution time stamp.

As the locking is changed to use a long-term lock to avoid such similar
DRBG states, the entire creation and maintenance of a shadow copy can be
removed.

Signed-off-by: Stephan Mueller <smueller@chronox.de>
---
 crypto/drbg.c | 122 ++++++----------------------------------------------------
 1 file changed, 11 insertions(+), 111 deletions(-)

diff --git a/crypto/drbg.c b/crypto/drbg.c
index 8d2944f..c8a083c 100644
--- a/crypto/drbg.c
+++ b/crypto/drbg.c
@@ -1189,79 +1189,6 @@ err:
 	return ret;
 }
 
-/*
- * Strategy to avoid holding long term locks: generate a shadow copy of DRBG
- * and perform all operations on this shadow copy. After finishing, restore
- * the updated state of the shadow copy into original drbg state. This way,
- * only the read and write operations of the original drbg state must be
- * locked
- */
-static inline void drbg_copy_drbg(struct drbg_state *src,
-				  struct drbg_state *dst)
-{
-	if (!src || !dst)
-		return;
-	memcpy(dst->V, src->V, drbg_statelen(src));
-	memcpy(dst->C, src->C, drbg_statelen(src));
-	dst->reseed_ctr = src->reseed_ctr;
-	dst->seeded = src->seeded;
-	dst->pr = src->pr;
-#ifdef CONFIG_CRYPTO_FIPS
-	dst->fips_primed = src->fips_primed;
-	memcpy(dst->prev, src->prev, drbg_blocklen(src));
-#endif
-	/*
-	 * Not copied:
-	 * scratchpad is initialized drbg_alloc_state;
-	 * priv_data is initialized with call to crypto_init;
-	 * d_ops and core are set outside, as these parameters are const;
-	 * test_data is set outside to prevent it being copied back.
-	 */
-}
-
-static int drbg_make_shadow(struct drbg_state *drbg, struct drbg_state **shadow)
-{
-	int ret = -ENOMEM;
-	struct drbg_state *tmp = NULL;
-
-	tmp = kzalloc(sizeof(struct drbg_state), GFP_KERNEL);
-	if (!tmp)
-		return -ENOMEM;
-
-	/* read-only data as they are defined as const, no lock needed */
-	tmp->core = drbg->core;
-	tmp->d_ops = drbg->d_ops;
-
-	ret = drbg_alloc_state(tmp);
-	if (ret)
-		goto err;
-
-	spin_lock_bh(&drbg->drbg_lock);
-	drbg_copy_drbg(drbg, tmp);
-	/* only make a link to the test buffer, as we only read that data */
-	tmp->test_data = drbg->test_data;
-	spin_unlock_bh(&drbg->drbg_lock);
-	*shadow = tmp;
-	return 0;
-
-err:
-	kzfree(tmp);
-	return ret;
-}
-
-static void drbg_restore_shadow(struct drbg_state *drbg,
-				struct drbg_state **shadow)
-{
-	struct drbg_state *tmp = *shadow;
-
-	spin_lock_bh(&drbg->drbg_lock);
-	drbg_copy_drbg(tmp, drbg);
-	spin_unlock_bh(&drbg->drbg_lock);
-	drbg_dealloc_state(tmp);
-	kzfree(tmp);
-	*shadow = NULL;
-}
-
 /*************************************************************************
  * DRBG interface functions
  *************************************************************************/
@@ -1287,13 +1214,7 @@ static int drbg_generate(struct drbg_state *drbg,
 			 struct drbg_string *addtl)
 {
 	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");
@@ -1304,15 +1225,9 @@ static int drbg_generate(struct drbg_state *drbg,
 		return -EINVAL;
 	}
 
-	len = drbg_make_shadow(drbg, &shadow);
-	if (len) {
-		pr_devel("DRBG: shadow copy cannot be generated\n");
-		return len;
-	}
-
 	/* 9.3.1 step 2 */
 	len = -EINVAL;
-	if (buflen > (drbg_max_request_bytes(shadow))) {
+	if (buflen > (drbg_max_request_bytes(drbg))) {
 		pr_devel("DRBG: requested random numbers too large %u\n",
 			 buflen);
 		goto err;
@@ -1321,7 +1236,7 @@ static int drbg_generate(struct drbg_state *drbg,
 	/* 9.3.1 step 3 is implicit with the chosen DRBG */
 
 	/* 9.3.1 step 4 */
-	if (addtl && addtl->len > (drbg_max_addtl(shadow))) {
+	if (addtl && addtl->len > (drbg_max_addtl(drbg))) {
 		pr_devel("DRBG: additional information string too long %zu\n",
 			 addtl->len);
 		goto err;
@@ -1332,46 +1247,34 @@ static int drbg_generate(struct drbg_state *drbg,
 	 * 9.3.1 step 6 and 9 supplemented by 9.3.2 step c is implemented
 	 * here. The spec is a bit convoluted here, we make it simpler.
 	 */
-	if ((drbg_max_requests(shadow)) < shadow->reseed_ctr)
-		shadow->seeded = false;
+	if ((drbg_max_requests(drbg)) < drbg->reseed_ctr)
+		drbg->seeded = false;
 
 	/* allocate cipher handle */
-	len = shadow->d_ops->crypto_init(shadow);
+	len = drbg->d_ops->crypto_init(drbg);
 	if (len)
 		goto err;
 
-	if (shadow->pr || !shadow->seeded) {
+	if (drbg->pr || !drbg->seeded) {
 		pr_devel("DRBG: reseeding before generation (prediction "
 			 "resistance: %s, state %s)\n",
 			 drbg->pr ? "true" : "false",
 			 drbg->seeded ? "seeded" : "unseeded");
 		/* 9.3.1 steps 7.1 through 7.3 */
-		len = drbg_seed(shadow, addtl, true);
+		len = drbg_seed(drbg, addtl, true);
 		if (len)
 			goto err;
 		/* 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, &addtllist);
+	len = drbg->d_ops->generate(drbg, 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++;
+	drbg->reseed_ctr++;
 	if (0 >= len)
 		goto err;
 
@@ -1391,7 +1294,7 @@ static int drbg_generate(struct drbg_state *drbg,
 	 * case somebody has a need to implement the test of 11.3.3.
 	 */
 #if 0
-	if (shadow->reseed_ctr && !(shadow->reseed_ctr % 4096)) {
+	if (drbg->reseed_ctr && !(drbg->reseed_ctr % 4096)) {
 		int err = 0;
 		pr_devel("DRBG: start to perform self test\n");
 		if (drbg->core->flags & DRBG_HMAC)
@@ -1410,8 +1313,6 @@ static int drbg_generate(struct drbg_state *drbg,
 			 * are returned when reusing this DRBG cipher handle
 			 */
 			drbg_uninstantiate(drbg);
-			drbg_dealloc_state(shadow);
-			kzfree(shadow);
 			return 0;
 		} else {
 			pr_devel("DRBG: self test successful\n");
@@ -1425,8 +1326,7 @@ static int drbg_generate(struct drbg_state *drbg,
 	 */
 	len = 0;
 err:
-	shadow->d_ops->crypto_fini(shadow);
-	drbg_restore_shadow(drbg, &shadow);
+	drbg->d_ops->crypto_fini(drbg);
 	return len;
 }
 
-- 
2.1.0

Re: [PATCH 2/4] crypto: drbg - do not create shadow copy

[Herbert Xu]

On Fri, Apr 17, 2015 at 02:54:58PM +0200, Stephan Mueller wrote:
> The creation of a shadow copy is intended to only hold a short term
> lock. But the drawback is that parallel users have a very similar DRBG
> state which only differs by a high-resolution time stamp.
> 
> As the locking is changed to use a long-term lock to avoid such similar
> DRBG states, the entire creation and maintenance of a shadow copy can be
> removed.
> 
> Signed-off-by: Stephan Mueller <smueller@chronox.de>

This patch needs to come after the lock/mutex conversion.  Otherwise
you will break bisections.

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 2/4] crypto: drbg - do not create shadow copy

[Stephan Mueller]

Am Samstag, 18. April 2015, 18:49:43 schrieb Herbert Xu:

Hi Herbert,

> On Fri, Apr 17, 2015 at 02:54:58PM +0200, Stephan Mueller wrote:
> > The creation of a shadow copy is intended to only hold a short term
> > lock. But the drawback is that parallel users have a very similar DRBG
> > state which only differs by a high-resolution time stamp.
> > 
> > As the locking is changed to use a long-term lock to avoid such similar
> > DRBG states, the entire creation and maintenance of a shadow copy can be
> > removed.
> > 
> > Signed-off-by: Stephan Mueller <smueller@chronox.de>
> 
> This patch needs to come after the lock/mutex conversion.  Otherwise
> you will break bisections.

For the next patch installment, I would like to merge both patches as they 
cannot stand alone by themselves.

The separation into two patches is just done to aid code review.
> 
> Cheers,


-- 
Ciao
Stephan

[PATCH 3/4] crypto: drbg - replace spinlock with mutex

[Stephan Mueller]

The DRBG shall hold a long term lock. Therefore, the lock is changed to
a mutex which implies that the DRBG can only be used in process context.

The lock now guards the instantiation as well as the entire DRBG
generation operation. Therefore, multiple callers are fully serialized
when generating a random number.

Signed-off-by: Stephan Mueller <smueller@chronox.de>
---
 crypto/drbg.c         | 22 ++++++++++++++--------
 include/crypto/drbg.h |  4 ++--
 2 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/crypto/drbg.c b/crypto/drbg.c
index c8a083c..19916ea 100644
--- a/crypto/drbg.c
+++ b/crypto/drbg.c
@@ -1181,7 +1181,6 @@ static inline int drbg_alloc_state(struct drbg_state *drbg)
 		if (!drbg->scratchpad)
 			goto err;
 	}
-	spin_lock_init(&drbg->drbg_lock);
 	return 0;
 
 err:
@@ -1349,7 +1348,9 @@ static int drbg_generate_long(struct drbg_state *drbg,
 		unsigned int chunk = 0;
 		slice = ((buflen - len) / drbg_max_request_bytes(drbg));
 		chunk = slice ? drbg_max_request_bytes(drbg) : (buflen - len);
+		mutex_lock(&drbg->drbg_mutex);
 		tmplen = drbg_generate(drbg, buf + len, chunk, addtl);
+		mutex_unlock(&drbg->drbg_mutex);
 		if (0 >= tmplen)
 			return tmplen;
 		len += tmplen;
@@ -1377,10 +1378,12 @@ static int drbg_generate_long(struct drbg_state *drbg,
 static int drbg_instantiate(struct drbg_state *drbg, struct drbg_string *pers,
 			    int coreref, bool pr)
 {
-	int ret = -ENOMEM;
+	int ret = -EOPNOTSUPP;
 
 	pr_devel("DRBG: Initializing DRBG core %d with prediction resistance "
 		 "%s\n", coreref, pr ? "enabled" : "disabled");
+	mutex_init(&drbg->drbg_mutex);
+	mutex_lock(&drbg->drbg_mutex);
 	drbg->core = &drbg_cores[coreref];
 	drbg->pr = pr;
 	drbg->seeded = false;
@@ -1401,7 +1404,7 @@ static int drbg_instantiate(struct drbg_state *drbg, struct drbg_string *pers,
 		break;
 #endif /* CONFIG_CRYPTO_DRBG_CTR */
 	default:
-		return -EOPNOTSUPP;
+		goto unlock;
 	}
 
 	/* 9.1 step 1 is implicit with the selected DRBG type */
@@ -1416,7 +1419,7 @@ static int drbg_instantiate(struct drbg_state *drbg, struct drbg_string *pers,
 
 	ret = drbg_alloc_state(drbg);
 	if (ret)
-		return ret;
+		goto unlock;
 
 	ret = -EFAULT;
 	if (drbg->d_ops->crypto_init(drbg))
@@ -1426,10 +1429,13 @@ static int drbg_instantiate(struct drbg_state *drbg, struct drbg_string *pers,
 	if (ret)
 		goto err;
 
+	mutex_unlock(&drbg->drbg_mutex);
 	return 0;
 
 err:
 	drbg_dealloc_state(drbg);
+unlock:
+	mutex_unlock(&drbg->drbg_mutex);
 	return ret;
 }
 
@@ -1444,10 +1450,10 @@ err:
  */
 static int drbg_uninstantiate(struct drbg_state *drbg)
 {
-	spin_lock_bh(&drbg->drbg_lock);
+	mutex_lock(&drbg->drbg_mutex);
 	drbg_dealloc_state(drbg);
 	/* no scrubbing of test_data -- this shall survive an uninstantiate */
-	spin_unlock_bh(&drbg->drbg_lock);
+	mutex_unlock(&drbg->drbg_mutex);
 	return 0;
 }
 
@@ -1462,9 +1468,9 @@ static inline void drbg_set_testdata(struct drbg_state *drbg,
 {
 	if (!test_data || !test_data->testentropy)
 		return;
-	spin_lock_bh(&drbg->drbg_lock);
+	mutex_lock(&drbg->drbg_mutex);;
 	drbg->test_data = test_data;
-	spin_unlock_bh(&drbg->drbg_lock);
+	mutex_unlock(&drbg->drbg_mutex);
 }
 
 /***************************************************************
diff --git a/include/crypto/drbg.h b/include/crypto/drbg.h
index 5186f75..a43a7ed 100644
--- a/include/crypto/drbg.h
+++ b/include/crypto/drbg.h
@@ -49,7 +49,7 @@
 #include <crypto/internal/rng.h>
 #include <crypto/rng.h>
 #include <linux/fips.h>
-#include <linux/spinlock.h>
+#include <linux/mutex.h>
 #include <linux/list.h>
 
 /*
@@ -104,7 +104,7 @@ struct drbg_test_data {
 };
 
 struct drbg_state {
-	spinlock_t drbg_lock;	/* lock around DRBG */
+	struct mutex drbg_mutex;	/* lock around DRBG */
 	unsigned char *V;	/* internal state 10.1.1.1 1a) */
 	/* hash: static value 10.1.1.1 1b) hmac / ctr: key */
 	unsigned char *C;
-- 
2.1.0

Re: [PATCH 3/4] crypto: drbg - replace spinlock with mutex

[Herbert Xu]

On Fri, Apr 17, 2015 at 02:55:28PM +0200, Stephan Mueller wrote:
>
> @@ -1349,7 +1348,9 @@ static int drbg_generate_long(struct drbg_state *drbg,
>  		unsigned int chunk = 0;
>  		slice = ((buflen - len) / drbg_max_request_bytes(drbg));
>  		chunk = slice ? drbg_max_request_bytes(drbg) : (buflen - len);
> +		mutex_lock(&drbg->drbg_mutex);
>  		tmplen = drbg_generate(drbg, buf + len, chunk, addtl);
> +		mutex_unlock(&drbg->drbg_mutex);
>  		if (0 >= tmplen)
>  			return tmplen;

BTW I just noticed that we always return 0 now so this is broken.

> @@ -1377,10 +1378,12 @@ static int drbg_generate_long(struct drbg_state *drbg,
>  static int drbg_instantiate(struct drbg_state *drbg, struct drbg_string *pers,
>  			    int coreref, bool pr)
>  {
> -	int ret = -ENOMEM;
> +	int ret = -EOPNOTSUPP;
>  
>  	pr_devel("DRBG: Initializing DRBG core %d with prediction resistance "
>  		 "%s\n", coreref, pr ? "enabled" : "disabled");
> +	mutex_init(&drbg->drbg_mutex);

This shouldn't be here because the reset function will call this
and break.  This needs to be moved to cra_init.

Incidentally the whole reset concept seems redundant as you could
always free and allocate a new RNG instead.  So I'm planning on
replacing it with a seed/reseed function instead.  IOW it will
keep the existing state and just add in new data if it's already
seeded.

This will change our user-space API semantically but as you're the
only user I hope we won't need to add any backwards compatibility
cruft.

This also means that cra_init will no longer seed this and the RNG
won't be usable until seed is called.

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] crypto: drbg - replace spinlock with mutex

[Stephan Mueller]

Am Samstag, 18. April 2015, 18:55:23 schrieb Herbert Xu:

Hi Herbert,

> 
> Incidentally the whole reset concept seems redundant as you could
> always free and allocate a new RNG instead.  So I'm planning on
> replacing it with a seed/reseed function instead.  IOW it will
> keep the existing state and just add in new data if it's already
> seeded.

Very good.

When you do that, may I ask you to also update the crypto_alloc_rng. This 
function has one major drawback at the moment: we are wasting precious 
entropy. The testmgr must allocate the RNG for performing its testing using 
crypto_alloc_rng. As the RNG implementation has no knowledge at allocation 
time that it will be used for testing, it will seed itself for the real work. 
Then comes testing: the seeded RNG is now reset with the test "entropy" and 
thereby wasting the initial seed.

IMHO such change in the allocation function must happen when you remove the 
reset function, because the reset function currently is the way to perform 
testing of the RNGs.

-- 
Ciao
Stephan

Re: [PATCH 3/4] crypto: drbg - replace spinlock with mutex

[Herbert Xu]

On Sat, Apr 18, 2015 at 01:35:40PM +0200, Stephan Mueller wrote:
>
> When you do that, may I ask you to also update the crypto_alloc_rng. This 
> function has one major drawback at the moment: we are wasting precious 
> entropy. The testmgr must allocate the RNG for performing its testing using 
> crypto_alloc_rng. As the RNG implementation has no knowledge at allocation 
> time that it will be used for testing, it will seed itself for the real work. 
> Then comes testing: the seeded RNG is now reset with the test "entropy" and 
> thereby wasting the initial seed.

Yes the DRBG will be unusable when first allocated and you must
seed it to actually draw entropy and be able to use it.

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] crypto: drbg - replace spinlock with mutex

[Stephan Mueller]

Am Sonntag, 19. April 2015, 13:48:03 schrieb Herbert Xu:

Hi Herbert,

> On Sat, Apr 18, 2015 at 01:35:40PM +0200, Stephan Mueller wrote:
> > When you do that, may I ask you to also update the crypto_alloc_rng. This
> > function has one major drawback at the moment: we are wasting precious
> > entropy. The testmgr must allocate the RNG for performing its testing
> > using
> > crypto_alloc_rng. As the RNG implementation has no knowledge at allocation
> > time that it will be used for testing, it will seed itself for the real
> > work. Then comes testing: the seeded RNG is now reset with the test
> > "entropy" and thereby wasting the initial seed.
> 
> Yes the DRBG will be unusable when first allocated and you must
> seed it to actually draw entropy and be able to use it.

I am not sure I understand you correctly: shall the DRBG have these 
precautions? If so, wouldn't we break the requirements in SP800-90A where the 
DRBG is intended to seed itself?

Or would you want to update the crypto_alloc_rng routine?
> 
> Cheers,


-- 
Ciao
Stephan

Re: [PATCH 3/4] crypto: drbg - replace spinlock with mutex

[Herbert Xu]

On Sun, Apr 19, 2015 at 05:37:21PM +0200, Stephan Mueller wrote:
>
> I am not sure I understand you correctly: shall the DRBG have these 
> precautions? If so, wouldn't we break the requirements in SP800-90A where the 
> DRBG is intended to seed itself?
> 
> Or would you want to update the crypto_alloc_rng routine?

No.  Our API doesn't provide the Instantiate_function obviously.
If you really want to have an explicit instantiate function then
you can provide a wrapper:

crypto_instantiate_drbg(...)
{
	struct crypto_rng *drbg;

	drbg = crypto_alloc_rng(...);
	crypto_rng_reset(drbg, ...);
	return drbg;
}

The fact that crypto_alloc_rng currently instantiates the RNG
is wrong because there is no provision for the personalisation
string.

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] crypto: drbg - replace spinlock with mutex

[Stephan Mueller]

Am Montag, 20. April 2015, 08:27:09 schrieb Herbert Xu:

Hi Herbert,

>On Sun, Apr 19, 2015 at 05:37:21PM +0200, Stephan Mueller wrote:
>> I am not sure I understand you correctly: shall the DRBG have these
>> precautions? If so, wouldn't we break the requirements in SP800-90A where
>> the DRBG is intended to seed itself?
>> 
>> Or would you want to update the crypto_alloc_rng routine?
>
>No.  Our API doesn't provide the Instantiate_function obviously.
>If you really want to have an explicit instantiate function then
>you can provide a wrapper:
>
>crypto_instantiate_drbg(...)
>{
>	struct crypto_rng *drbg;
>
>	drbg = crypto_alloc_rng(...);
>	crypto_rng_reset(drbg, ...);
>	return drbg;
>}
>
>The fact that crypto_alloc_rng currently instantiates the RNG
>is wrong because there is no provision for the personalisation
>string.

I do not want to deviate from the kernel crypto API by adding some additional 
wrapper. But what we can do is to leave the DRBG unseeded during alloc time. 
As long as the DRBG is unseeded, it will return EAGAIN to any request for 
random numbers, forcing the caller to use crypto_rng_reset to activate the 
DRBG.

When the DRBG receives a reset, it will always obtain the seed and treat any 
user-provided data as personalization string / additional data.

Such change is straight forward. I would like to roll that one into the 
patchset for the discussed seeding revamp as this issue is definitely 
noticeable there. That patch set is already complete, I am just doing the 
final testing before airing it.

Ciao
Stephan

Re: [PATCH 3/4] crypto: drbg - replace spinlock with mutex

[Herbert Xu]

On Mon, Apr 20, 2015 at 02:45:02AM +0200, Stephan Mueller wrote:
> 
> I do not want to deviate from the kernel crypto API by adding some additional 
> wrapper. But what we can do is to leave the DRBG unseeded during alloc time. 
> As long as the DRBG is unseeded, it will return EAGAIN to any request for 
> random numbers, forcing the caller to use crypto_rng_reset to activate the 
> DRBG.
> 
> When the DRBG receives a reset, it will always obtain the seed and treat any 
> user-provided data as personalization string / additional data.

That's exactly what I was suggesting.  I already have two patches
that I will post once I finish testing.

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] crypto: drbg - replace spinlock with mutex

[Stephan Mueller]

Am Montag, 20. April 2015, 08:48:55 schrieb Herbert Xu:

Hi Herbert,

>On Mon, Apr 20, 2015 at 02:45:02AM +0200, Stephan Mueller wrote:
>> I do not want to deviate from the kernel crypto API by adding some
>> additional wrapper. But what we can do is to leave the DRBG unseeded
>> during alloc time. As long as the DRBG is unseeded, it will return EAGAIN
>> to any request for random numbers, forcing the caller to use
>> crypto_rng_reset to activate the DRBG.
>> 
>> When the DRBG receives a reset, it will always obtain the seed and treat
>> any
>> user-provided data as personalization string / additional data.
>
>That's exactly what I was suggesting.  I already have two patches
>that I will post once I finish testing.

Ok, I will wait then for your patches before I send out my patch set for the 
seeding revamp.
>
>Cheers,


Ciao
Stephan

[PATCH 4/4] crypto: drbg - leave cipher handles operational

[Stephan Mueller]

As the DRBG does not operate on shadow copies of the DRBG instance
any more, the cipher handles only need to be allocated once during
initalization time and deallocated during uninstantiate time.

Signed-off-by: Stephan Mueller <smueller@chronox.de>
---
 crypto/drbg.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/crypto/drbg.c b/crypto/drbg.c
index 19916ea..4289624 100644
--- a/crypto/drbg.c
+++ b/crypto/drbg.c
@@ -1249,11 +1249,6 @@ static int drbg_generate(struct drbg_state *drbg,
 	if ((drbg_max_requests(drbg)) < drbg->reseed_ctr)
 		drbg->seeded = false;
 
-	/* allocate cipher handle */
-	len = drbg->d_ops->crypto_init(drbg);
-	if (len)
-		goto err;
-
 	if (drbg->pr || !drbg->seeded) {
 		pr_devel("DRBG: reseeding before generation (prediction "
 			 "resistance: %s, state %s)\n",
@@ -1325,7 +1320,6 @@ static int drbg_generate(struct drbg_state *drbg,
 	 */
 	len = 0;
 err:
-	drbg->d_ops->crypto_fini(drbg);
 	return len;
 }
 
@@ -1425,9 +1419,10 @@ static int drbg_instantiate(struct drbg_state *drbg, struct drbg_string *pers,
 	if (drbg->d_ops->crypto_init(drbg))
 		goto err;
 	ret = drbg_seed(drbg, pers, false);
-	drbg->d_ops->crypto_fini(drbg);
-	if (ret)
+	if (ret) {
+		drbg->d_ops->crypto_fini(drbg);
 		goto err;
+	}
 
 	mutex_unlock(&drbg->drbg_mutex);
 	return 0;
@@ -1451,6 +1446,7 @@ unlock:
 static int drbg_uninstantiate(struct drbg_state *drbg)
 {
 	mutex_lock(&drbg->drbg_mutex);
+	drbg->d_ops->crypto_fini(drbg);
 	drbg_dealloc_state(drbg);
 	/* no scrubbing of test_data -- this shall survive an uninstantiate */
 	mutex_unlock(&drbg->drbg_mutex);
-- 
2.1.0