| Message ID | 2415536.7nqArbGB13@myon.chronox.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Herbert Xu |
| Headers | show |
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 -- 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
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.
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,
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,
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,
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 -- 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 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,
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 -- 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/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;
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(-)