Re: [PATCH 4/6] crypto: DRBG - make reseeding from get_random_bytes() synchronous

[Stephan Müller]

Am Mittwoch, 27. Oktober 2021, 11:19:50 CEST schrieb Nicolai Stange:

Hi Nicolai,

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

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