| Message ID | 20211025092525.12805-5-nstange@suse.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | crypto: DRBG - improve 'nopr' reseeding | expand |
Am Montag, 25. Oktober 2021, 11:25:23 CEST schrieb Nicolai Stange: Hi Nicolai, > 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. So, the DRBG seems to run still partially seeded even though it could already be fully seeded, no? > > Signed-off-by: Nicolai Stange <nstange@suse.de> > --- > 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? > > +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; > } > > 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) Ciao Stephan
Hi Stephan, Stephan Müller <smueller@chronox.de> 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@suse.de> >> --- >> 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)
Am Mittwoch, 27. Oktober 2021, 11:19:50 CEST schrieb Nicolai Stange: Hi Nicolai, > Hi Stephan, > > Stephan Müller <smueller@chronox.de> 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@suse.de> > >> --- > >> > >> 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. I agree with your explanation above and the description here. I have no objections. Thanks Stephan > > 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) Ciao Stephan
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; +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; } 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)
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. Signed-off-by: Nicolai Stange <nstange@suse.de> --- crypto/drbg.c | 64 +++++++++---------------------------------- include/crypto/drbg.h | 2 -- 2 files changed, 13 insertions(+), 53 deletions(-)