| Message ID | 20211025092525.12805-1-nstange@suse.de (mailing list archive) |
|---|---|
| Headers | show |
| Series | crypto: DRBG - improve 'nopr' reseeding | expand |
Am Montag, 25. Oktober 2021, 11:25:19 CEST schrieb Nicolai Stange: Hi Nicolai, > Hi all, > > this patchset aims at (hopefully) improving the DRBG code related to > reseeding from get_random_bytes() a bit: Thanks for sharing your patches. > - Replace the asynchronous random_ready_callback based DRBG reseeding > logic with a synchronous solution leveraging rng_is_initialized(). Could you please help me why replacing an async method with a sync method is helpful? Which problems do you see with the async method that are alleviated with the swtich to the sync method? In general, an async method is more powerful, though it requires a bit more code. > This > move simplifies the code IMO and, as a side-effect, would enable DRBG > users to rely on wait_for_random_bytes() to sync properly with > drbg_generate(), if desired. Implemented by patches 1-5/6. > - Make the 'nopr' DRBGs to reseed themselves every 5min from > get_random_bytes(). This achieves at least kind of a partial prediction > resistance over the time domain at almost no extra cost. Implemented > by patch 6/6, the preceding patches in this series are a prerequisite > for this. Just as a side note not against your ideas and patches, but in general: IMHO it is a failure of all of us that the quite sensitive (re)seeding of RNGs and entropy management is handled in multiple places in the kernel - and each case only handles a subset of considerations around that topic. Note, (re)seeding may be needed in other occasions than the elapse of a timer or the reaching of maximum number of generate operations. Seeding belongs to a central place where it is done right once and usable for differnent RNGs as proposed with my LRNG patch set and the published todo list to get rid of the entire seeding logic in the DRBG code base. That said, your patch of adding the timer-based reseeding seems appropriate and thus should be considered for the current code base. Ciao Stephan
Hi Stephan, first of all, many thanks for your prompt review! Stephan Müller <smueller@chronox.de> writes: > Am Montag, 25. Oktober 2021, 11:25:19 CEST schrieb Nicolai Stange: > > >> - Replace the asynchronous random_ready_callback based DRBG reseeding >> logic with a synchronous solution leveraging rng_is_initialized(). > > Could you please help me why replacing an async method with a sync method is > helpful? Which problems do you see with the async method that are alleviated > with the swtich to the sync method? In general, an async method is more > powerful, though it requires a bit more code. There is no problem with the async method (*), I just don't see any advantage over the less complex approach of doing all reseeding work synchronously from drbg_generate(). Before the change, there had been two sites taking care of reseeding: the drbg_async_seed() work handler scheduled from the random_ready_callback and drbg_generate(). After the change, all reseeding is handled at a single place only, namely drbg_generate(), which, in my opinion, makes it easier to reason about. In particular, in preparation for patch 6/6 from this series introducing yet another condition for triggering a reseed... Thanks, Nicolai (*) Except for that a wait_for_random_bytes() issued by DRBG users won't give any guarantees with respect to a subsequent drbg_generate() operation, c.f. my other mail in reply to your review on 3/6 I'm about to write in a second. As of now, there aren't any DRBG users invoking wait_for_random_bytes(), but one might perhaps consider changing that in the future. >> This >> move simplifies the code IMO and, as a side-effect, would enable DRBG >> users to rely on wait_for_random_bytes() to sync properly with >> drbg_generate(), if desired. Implemented by patches 1-5/6. >> - Make the 'nopr' DRBGs to reseed themselves every 5min from >> get_random_bytes(). This achieves at least kind of a partial prediction >> resistance over the time domain at almost no extra cost. Implemented >> by patch 6/6, the preceding patches in this series are a prerequisite >> for this.
Am Mittwoch, 27. Oktober 2021, 10:40:12 CEST schrieb Nicolai Stange: Hi Nicolai, > Hi Stephan, > > first of all, many thanks for your prompt review! > > Stephan Müller <smueller@chronox.de> writes: > > Am Montag, 25. Oktober 2021, 11:25:19 CEST schrieb Nicolai Stange: > >> - Replace the asynchronous random_ready_callback based DRBG reseeding > >> > >> logic with a synchronous solution leveraging rng_is_initialized(). > > > > Could you please help me why replacing an async method with a sync method > > is helpful? Which problems do you see with the async method that are > > alleviated with the swtich to the sync method? In general, an async > > method is more powerful, though it requires a bit more code. > > There is no problem with the async method (*), I just don't see any > advantage over the less complex approach of doing all reseeding > work synchronously from drbg_generate(). > > Before the change, there had been two sites taking care of reseeding: > the drbg_async_seed() work handler scheduled from the > random_ready_callback and drbg_generate(). > > After the change, all reseeding is handled at a single place only, namely > drbg_generate(), which, in my opinion, makes it easier to reason about. > In particular, in preparation for patch 6/6 from this series introducing > yet another condition for triggering a reseed... That makes sense. Thanks for clarifying. Ciao Stephan > > Thanks, > > Nicolai > > (*) Except for that a wait_for_random_bytes() issued by DRBG users won't > give any guarantees with respect to a subsequent drbg_generate() > operation, c.f. my other mail in reply to your review on 3/6 I'm > about to write in a second. As of now, there aren't any DRBG users > invoking wait_for_random_bytes(), but one might perhaps consider > changing that in the future. > > >> This > >> move simplifies the code IMO and, as a side-effect, would enable DRBG > >> users to rely on wait_for_random_bytes() to sync properly with > >> drbg_generate(), if desired. Implemented by patches 1-5/6. > >> > >> - Make the 'nopr' DRBGs to reseed themselves every 5min from > >> > >> get_random_bytes(). This achieves at least kind of a partial prediction > >> resistance over the time domain at almost no extra cost. Implemented > >> by patch 6/6, the preceding patches in this series are a prerequisite > >> for this. Ciao Stephan
Hi all, this patchset aims at (hopefully) improving the DRBG code related to reseeding from get_random_bytes() a bit: - Replace the asynchronous random_ready_callback based DRBG reseeding logic with a synchronous solution leveraging rng_is_initialized(). This move simplifies the code IMO and, as a side-effect, would enable DRBG users to rely on wait_for_random_bytes() to sync properly with drbg_generate(), if desired. Implemented by patches 1-5/6. - Make the 'nopr' DRBGs to reseed themselves every 5min from get_random_bytes(). This achieves at least kind of a partial prediction resistance over the time domain at almost no extra cost. Implemented by patch 6/6, the preceding patches in this series are a prerequisite for this. Tested with and without fips_enabled in a x86_64 VM, both with random.trust_cpu=on and off. As confirmed with a couple of debugging printks() (added for testing only, not included in this series), DRBGs have been instantiated with and without rng_is_initialized() evaluating to true each during my tests and the patched DRBG reseeding code worked as intended in either case. Applies to current herbert/cryptodev-2.6.git master. Many thanks for your comments and remarks! Nicolai Nicolai Stange (6): crypto: DRBG - prepare for more fine-grained tracking of seeding state crypto: DRBG - track whether DRBG was seeded with !rng_is_initialized() crypto: DRBG - move dynamic ->reseed_threshold adjustments to __drbg_seed() crypto: DRBG - make reseeding from get_random_bytes() synchronous crypto: DRBG - make drbg_prepare_hrng() handle jent instantiation errors crypto: DRBG - reseed 'nopr' drbgs periodically from get_random_bytes() crypto/drbg.c | 145 +++++++++++++++++++++--------------------- include/crypto/drbg.h | 11 +++- 2 files changed, 82 insertions(+), 74 deletions(-)