Hi Stephan, Stephan Müller <smueller@xxxxxxxxxx> writes: > Am Montag, 25. Oktober 2021, 11:25:23 CEST schrieb Nicolai Stange: > >> get_random_bytes() usually hasn't full entropy available by the time DRBG >> instances are first getting seeded from it during boot. Thus, the DRBG >> implementation registers random_ready_callbacks which would in turn >> schedule some work for reseeding the DRBGs once get_random_bytes() has >> sufficient entropy available. >> >> For reference, the relevant history around handling DRBG (re)seeding in >> the context of a not yet fully seeded get_random_bytes() is: >> >> commit 16b369a91d0d ("random: Blocking API for accessing >> nonblocking_pool") >> commit 4c7879907edd ("crypto: drbg - add async seeding operation") >> >> commit 205a525c3342 ("random: Add callback API for random pool >> readiness") >> commit 57225e679788 ("crypto: drbg - Use callback API for random >> readiness") >> commit c2719503f5e1 ("random: Remove kernel blocking API") >> >> However, some time later, the initialization state of get_random_bytes() >> has been made queryable via rng_is_initialized() introduced with commit >> 9a47249d444d ("random: Make crng state queryable"). This primitive now >> allows for streamlining the DRBG reseeding from get_random_bytes() by >> replacing that aforementioned asynchronous work scheduling from >> random_ready_callbacks with some simpler, synchronous code in >> drbg_generate() next to the related logic already present therein. Apart >> from improving overall code readability, this change will also enable DRBG >> users to rely on wait_for_random_bytes() for ensuring that the initial >> seeding has completed, if desired. >> >> The previous patches already laid the grounds by making drbg_seed() to >> record at each DRBG instance whether it was being seeded at a time when >> rng_is_initialized() still had been false as indicated by >> ->seeded == DRBG_SEED_STATE_PARTIAL. >> >> All that remains to be done now is to make drbg_generate() check for this >> condition, determine whether rng_is_initialized() has flipped to true in >> the meanwhile and invoke a reseed from get_random_bytes() if so. >> >> Make this move: >> - rename the former drbg_async_seed() work handler, i.e. the one in charge >> of reseeding a DRBG instance from get_random_bytes(), to >> "drbg_seed_from_random()", >> - change its signature as appropriate, i.e. make it take a struct >> drbg_state rather than a work_struct and change its return type from >> "void" to "int" in order to allow for passing error information from >> e.g. its __drbg_seed() invocation onwards to callers, >> - make drbg_generate() invoke this drbg_seed_from_random() once it >> encounters a DRBG instance with ->seeded == DRBG_SEED_STATE_PARTIAL by >> the time rng_is_initialized() has flipped to true and >> - prune everything related to the former, random_ready_callback based >> mechanism. >> >> As drbg_seed_from_random() is now getting invoked from drbg_generate() with >> the ->drbg_mutex being held, it must not attempt to recursively grab it >> once again. Remove the corresponding mutex operations from what is now >> drbg_seed_from_random(). Furthermore, as drbg_seed_from_random() can now >> report errors directly to its caller, there's no need for it to temporarily >> switch the DRBG's ->seeded state to DRBG_SEED_STATE_UNSEEDED so that a >> failure of the subsequently invoked __drbg_seed() will get signaled to >> drbg_generate(). Don't do it then. > > The code change in general looks good. But the code change seems to now imply > that the DRBG only gets fully seeded when its internal reseed operation is > invoked again - during boot time this is after the elapse of 50 generate > operations (or in your later patch after the elapse of 5 minutes). I.e. it is > not immediately reseeded when random.c turns to fully seeded and > rng_is_initialized() would turn true. I would argue that the DRBGs don't get immediately reseeded before this patch, because there's no guarantee on when the drbg_async_seed() work eventually gets to run. I.e. something like the following would be possible: wait_for_random_bytes() { crng_reseed() { crng_init = 2; process_random_ready_list() { drbg_schedule_async_seed(); } wake_up_interruptible(&crng_init_wait); } } crypto_rng_generate() { drbg_generate(); } drbg_async_seed(); /* <-- too late */ The wait_for_random_bytes() has been added only for demonstration purposes here, right now there aren't any DRBG users invoking it, AFAICT. Note that even in the presence of a hypothetical wait_for_random_bytes() barrier, it would still be possible for a subsequent drbg_generate() to run on a not yet reseeded DRBG. After this patch OTOH, the drbg_generate() would invoke a reseed by itself once it observes the crng_init == 2, i.e. once rng_is_initialized() flips to true. So yes, you're right, the DRBGs don't get reseeded immediately once get_random_bytes() becomes ready, but more in a "lazy fashion" when accessed the next time. However, it's now guaranteed that drbg_generate() won't operate on a not yet updated DRBG state when rng_is_initialized() is true (at function entry). > So, the DRBG seems to run still > partially seeded even though it could already be fully seeded, no? Assuming that by "run" you mean drbg_generate(), then no, I don't think so. Or at least that has not been my intention and would be a bug in the patch. For reference, I'll mark the spot in the quoted code below which is supposed to make drbg_generate() reseed the DRGB once rng_is_initialized() has flipped to true. >> >> Signed-off-by: Nicolai Stange <nstange@xxxxxxx> >> --- >> crypto/drbg.c | 64 +++++++++---------------------------------- >> include/crypto/drbg.h | 2 -- >> 2 files changed, 13 insertions(+), 53 deletions(-) >> >> diff --git a/crypto/drbg.c b/crypto/drbg.c >> index 6aad210f101a..d9f7dddfd683 100644 >> --- a/crypto/drbg.c >> +++ b/crypto/drbg.c >> @@ -1087,12 +1087,10 @@ static inline int drbg_get_random_bytes(struct >> drbg_state *drbg, return 0; >> } >> >> -static void drbg_async_seed(struct work_struct *work) >> +static int drbg_seed_from_random(struct drbg_state *drbg) >> { >> struct drbg_string data; >> LIST_HEAD(seedlist); >> - struct drbg_state *drbg = container_of(work, struct drbg_state, >> - seed_work); >> unsigned int entropylen = drbg_sec_strength(drbg->core->flags); >> unsigned char entropy[32]; >> int ret; >> @@ -1103,23 +1101,17 @@ static void drbg_async_seed(struct work_struct >> *work) drbg_string_fill(&data, entropy, entropylen); >> list_add_tail(&data.list, &seedlist); >> >> - mutex_lock(&drbg->drbg_mutex); >> - >> ret = drbg_get_random_bytes(drbg, entropy, entropylen); >> if (ret) >> - goto unlock; >> - >> - /* Reset ->seeded so that if __drbg_seed fails the next >> - * generate call will trigger a reseed. >> - */ >> - drbg->seeded = DRBG_SEED_STATE_UNSEEDED; >> - >> - __drbg_seed(drbg, &seedlist, true, DRBG_SEED_STATE_FULL); >> + goto out; >> >> -unlock: >> - mutex_unlock(&drbg->drbg_mutex); >> + ret = __drbg_seed(drbg, &seedlist, true, DRBG_SEED_STATE_FULL); >> + if (ret) >> + goto out; > > Is this last check for ret truly needed considering the goto target is the > next line? Darn, no. I'll wait a few more days for further review and send a v2 with this fixed up. >> +out: >> memzero_explicit(entropy, entropylen); >> + return ret; >> } >> >> /* >> @@ -1422,6 +1414,11 @@ static int drbg_generate(struct drbg_state *drbg, >> goto err; >> /* 9.3.1 step 7.4 */ >> addtl = NULL; >> + } else if (rng_is_initialized() && >> + drbg->seeded == DRBG_SEED_STATE_PARTIAL) { >> + len = drbg_seed_from_random(drbg); >> + if (len) >> + goto err; >> } As mentioned above, this here is the change that is supposed to make drbg_generate() reseed itself once rng_is_initialized() has flipped to true. Thanks, Nicolai >> >> if (addtl && 0 < addtl->len) >> @@ -1514,45 +1511,15 @@ static int drbg_generate_long(struct drbg_state >> *drbg, return 0; >> } >> >> -static void drbg_schedule_async_seed(struct random_ready_callback *rdy) >> -{ >> - struct drbg_state *drbg = container_of(rdy, struct drbg_state, >> - random_ready); >> - >> - schedule_work(&drbg->seed_work); >> -} >> - >> static int drbg_prepare_hrng(struct drbg_state *drbg) >> { >> - int err; >> - >> /* We do not need an HRNG in test mode. */ >> if (list_empty(&drbg->test_data.list)) >> return 0; >> >> drbg->jent = crypto_alloc_rng("jitterentropy_rng", 0, 0); >> >> - INIT_WORK(&drbg->seed_work, drbg_async_seed); >> - >> - drbg->random_ready.owner = THIS_MODULE; >> - drbg->random_ready.func = drbg_schedule_async_seed; >> - >> - err = add_random_ready_callback(&drbg->random_ready); >> - >> - switch (err) { >> - case 0: >> - break; >> - >> - case -EALREADY: >> - err = 0; >> - fallthrough; >> - >> - default: >> - drbg->random_ready.func = NULL; >> - return err; >> - } >> - >> - return err; >> + return 0; >> } >> >> /* >> @@ -1646,11 +1613,6 @@ static int drbg_instantiate(struct drbg_state *drbg, >> struct drbg_string *pers, */ >> static int drbg_uninstantiate(struct drbg_state *drbg) >> { >> - if (drbg->random_ready.func) { >> - del_random_ready_callback(&drbg->random_ready); >> - cancel_work_sync(&drbg->seed_work); >> - } >> - >> if (!IS_ERR_OR_NULL(drbg->jent)) >> crypto_free_rng(drbg->jent); >> drbg->jent = NULL; >> diff --git a/include/crypto/drbg.h b/include/crypto/drbg.h >> index 3ebdb1effe74..a6c3b8e7deb6 100644 >> --- a/include/crypto/drbg.h >> +++ b/include/crypto/drbg.h >> @@ -137,12 +137,10 @@ struct drbg_state { >> bool pr; /* Prediction resistance enabled? */ >> bool fips_primed; /* Continuous test primed? */ >> unsigned char *prev; /* FIPS 140-2 continuous test value */ >> - struct work_struct seed_work; /* asynchronous seeding support */ >> struct crypto_rng *jent; >> const struct drbg_state_ops *d_ops; >> const struct drbg_core *core; >> struct drbg_string test_data; >> - struct random_ready_callback random_ready; >> }; >> >> static inline __u8 drbg_statelen(struct drbg_state *drbg) -- SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg), GF: Felix Imendörffer