[PATCH 0/6] crypto: DRBG - improve 'nopr' reseeding

[PATCH 0/6] crypto: DRBG - improve 'nopr' reseeding

[Nicolai Stange]

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(-)

-- 
2.26.2

[PATCH 1/6] crypto: DRBG - prepare for more fine-grained tracking of seeding state

[Nicolai Stange]

There are two different randomness sources the DRBGs are getting seeded
from, namely the jitterentropy source (if enabled) and get_random_bytes().
At initial DRBG seeding time during boot, the latter might not have
collected sufficient entropy for seeding itself yet and thus, the DRBG
implementation schedules a reseed work from a random_ready_callback once
that has happened. This is particularly important for the !->pr DRBG
instances, for which (almost) no further reseeds are getting triggered
during their lifetime.

Because collecting data from the jitterentropy source is a rather expensive
operation, the aforementioned asynchronously scheduled reseed work
restricts itself to get_random_bytes() only. That is, it in some sense
amends the initial DRBG seed derived from jitterentropy output at full
(estimated) entropy with fresh randomness obtained from get_random_bytes()
once that has been seeded with sufficient entropy itself.

With the advent of rng_is_initialized(), there is no real need for doing
the reseed operation from an asynchronously scheduled work anymore and a
subsequent patch will make it synchronous by moving it next to related
logic already present in drbg_generate().

However, for tracking whether a full reseed including the jitterentropy
source is required or a "partial" reseed involving only get_random_bytes()
would be sufficient already, the boolean struct drbg_state's ->seeded
member must become a tristate value.

Prepare for this by introducing the new enum drbg_seed_state and change
struct drbg_state's ->seeded member's type from bool to that type.

For facilitating review, enum drbg_seed_state is made to only contain
two members corresponding to the former ->seeded values of false and true
resp. at this point: DRBG_SEED_STATE_UNSEEDED and DRBG_SEED_STATE_FULL. A
third one for tracking the intermediate state of "seeded from jitterentropy
only" will be introduced with a subsequent patch.

There is no change in behaviour at this point.

Signed-off-by: Nicolai Stange <nstange@suse.de>
---
 crypto/drbg.c         | 19 ++++++++++---------
 include/crypto/drbg.h |  7 ++++++-
 2 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/crypto/drbg.c b/crypto/drbg.c
index ea85d4a0fe9e..8651bd2c5c82 100644
--- a/crypto/drbg.c
+++ b/crypto/drbg.c
@@ -1043,7 +1043,7 @@ static inline int __drbg_seed(struct drbg_state *drbg, struct list_head *seed,
 	if (ret)
 		return ret;
 
-	drbg->seeded = true;
+	drbg->seeded = DRBG_SEED_STATE_FULL;
 	/* 10.1.1.2 / 10.1.1.3 step 5 */
 	drbg->reseed_ctr = 1;
 
@@ -1088,14 +1088,14 @@ static void drbg_async_seed(struct work_struct *work)
 	if (ret)
 		goto unlock;
 
-	/* Set seeded to false so that if __drbg_seed fails the
-	 * next generate call will trigger a reseed.
+	/* Reset ->seeded so that if __drbg_seed fails the next
+	 * generate call will trigger a reseed.
 	 */
-	drbg->seeded = false;
+	drbg->seeded = DRBG_SEED_STATE_UNSEEDED;
 
 	__drbg_seed(drbg, &seedlist, true);
 
-	if (drbg->seeded)
+	if (drbg->seeded == DRBG_SEED_STATE_FULL)
 		drbg->reseed_threshold = drbg_max_requests(drbg);
 
 unlock:
@@ -1386,13 +1386,14 @@ static int drbg_generate(struct drbg_state *drbg,
 	 * here. The spec is a bit convoluted here, we make it simpler.
 	 */
 	if (drbg->reseed_threshold < drbg->reseed_ctr)
-		drbg->seeded = false;
+		drbg->seeded = DRBG_SEED_STATE_UNSEEDED;
 
-	if (drbg->pr || !drbg->seeded) {
+	if (drbg->pr || drbg->seeded == DRBG_SEED_STATE_UNSEEDED) {
 		pr_devel("DRBG: reseeding before generation (prediction "
 			 "resistance: %s, state %s)\n",
 			 drbg->pr ? "true" : "false",
-			 drbg->seeded ? "seeded" : "unseeded");
+			 (drbg->seeded ==  DRBG_SEED_STATE_FULL ?
+			  "seeded" : "unseeded"));
 		/* 9.3.1 steps 7.1 through 7.3 */
 		len = drbg_seed(drbg, addtl, true);
 		if (len)
@@ -1578,7 +1579,7 @@ static int drbg_instantiate(struct drbg_state *drbg, struct drbg_string *pers,
 	if (!drbg->core) {
 		drbg->core = &drbg_cores[coreref];
 		drbg->pr = pr;
-		drbg->seeded = false;
+		drbg->seeded = DRBG_SEED_STATE_UNSEEDED;
 		drbg->reseed_threshold = drbg_max_requests(drbg);
 
 		ret = drbg_alloc_state(drbg);
diff --git a/include/crypto/drbg.h b/include/crypto/drbg.h
index c4165126937e..92a87b23ad2f 100644
--- a/include/crypto/drbg.h
+++ b/include/crypto/drbg.h
@@ -105,6 +105,11 @@ struct drbg_test_data {
 	struct drbg_string *testentropy; /* TEST PARAMETER: test entropy */
 };
 
+enum drbg_seed_state {
+	DRBG_SEED_STATE_UNSEEDED,
+	DRBG_SEED_STATE_FULL,
+};
+
 struct drbg_state {
 	struct mutex drbg_mutex;	/* lock around DRBG */
 	unsigned char *V;	/* internal state 10.1.1.1 1a) */
@@ -127,7 +132,7 @@ struct drbg_state {
 	struct crypto_wait ctr_wait;		/* CTR mode async wait obj */
 	struct scatterlist sg_in, sg_out;	/* CTR mode SGLs */
 
-	bool seeded;		/* DRBG fully seeded? */
+	enum drbg_seed_state seeded;		/* DRBG fully seeded? */
 	bool pr;		/* Prediction resistance enabled? */
 	bool fips_primed;	/* Continuous test primed? */
 	unsigned char *prev;	/* FIPS 140-2 continuous test value */
-- 
2.26.2

[PATCH 2/6] crypto: DRBG - track whether DRBG was seeded with !rng_is_initialized()

[Nicolai Stange]

Currently, the DRBG implementation schedules asynchronous works from
random_ready_callbacks for reseeding the DRBG instances with output from
get_random_bytes() once the latter has sufficient entropy available.

However, as the get_random_bytes() initialization state can get queried by
means of rng_is_initialized() now, there is no real need for this
asynchronous reseeding logic anymore and it's better to keep things simple
by doing it synchronously when needed instead, i.e. from drbg_generate()
once rng_is_initialized() has flipped to true.

Of course, for this to work, drbg_generate() would need some means by which
it can tell whether or not rng_is_initialized() has flipped to true since
the last seeding from get_random_bytes(). Or equivalently, whether or not
the last seed from get_random_bytes() has happened when
rng_is_initialized() was still evaluating to false.

As it currently stands, enum drbg_seed_state allows for the representation
of two different DRBG seeding states: DRBG_SEED_STATE_UNSEEDED and
DRBG_SEED_STATE_FULL. The former makes drbg_generate() to invoke a full
reseeding operation involving both, the rather expensive jitterentropy as
well as the get_random_bytes() randomness sources. The DRBG_SEED_STATE_FULL
state on the other hand implies that no reseeding at all is required for a
!->pr DRBG variant.

Introduce the new DRBG_SEED_STATE_PARTIAL state to enum drbg_seed_state for
representing the condition that a DRBG was being seeded when
rng_is_initialized() had still been false. In particular, this new state
implies that
- the given DRBG instance has been fully seeded from the jitterentropy
  source (if enabled)
- and drbg_generate() is supposed to reseed from get_random_bytes()
  *only* once rng_is_initialized() turns to true.

Up to now, the __drbg_seed() helper used to set the given DRBG instance's
->seeded state to constant DRBG_SEED_STATE_FULL. Introduce a new argument
allowing for the specification of the to be written ->seeded value instead.
Make the first of its two callers, drbg_seed(), determine the appropriate
value based on rng_is_initialized(). The remaining caller,
drbg_async_seed(), is known to get invoked only once rng_is_initialized()
is true, hence let it pass constant DRBG_SEED_STATE_FULL for the new
argument to __drbg_seed().

There is no change in behaviour, except for that the pr_devel() in
drbg_generate() would now report "unseeded" for ->pr DRBG instances which
had last been seeded when rng_is_initialized() was still evaluating to
false.

Signed-off-by: Nicolai Stange <nstange@suse.de>
---
 crypto/drbg.c         | 12 ++++++++----
 include/crypto/drbg.h |  1 +
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/crypto/drbg.c b/crypto/drbg.c
index 8651bd2c5c82..55fe8caf569a 100644
--- a/crypto/drbg.c
+++ b/crypto/drbg.c
@@ -1036,14 +1036,14 @@ static const struct drbg_state_ops drbg_hash_ops = {
  ******************************************************************/
 
 static inline int __drbg_seed(struct drbg_state *drbg, struct list_head *seed,
-			      int reseed)
+			      int reseed, enum drbg_seed_state new_seed_state)
 {
 	int ret = drbg->d_ops->update(drbg, seed, reseed);
 
 	if (ret)
 		return ret;
 
-	drbg->seeded = DRBG_SEED_STATE_FULL;
+	drbg->seeded = new_seed_state;
 	/* 10.1.1.2 / 10.1.1.3 step 5 */
 	drbg->reseed_ctr = 1;
 
@@ -1093,7 +1093,7 @@ static void drbg_async_seed(struct work_struct *work)
 	 */
 	drbg->seeded = DRBG_SEED_STATE_UNSEEDED;
 
-	__drbg_seed(drbg, &seedlist, true);
+	__drbg_seed(drbg, &seedlist, true, DRBG_SEED_STATE_FULL);
 
 	if (drbg->seeded == DRBG_SEED_STATE_FULL)
 		drbg->reseed_threshold = drbg_max_requests(drbg);
@@ -1123,6 +1123,7 @@ static int drbg_seed(struct drbg_state *drbg, struct drbg_string *pers,
 	unsigned int entropylen = drbg_sec_strength(drbg->core->flags);
 	struct drbg_string data1;
 	LIST_HEAD(seedlist);
+	enum drbg_seed_state new_seed_state = DRBG_SEED_STATE_FULL;
 
 	/* 9.1 / 9.2 / 9.3.1 step 3 */
 	if (pers && pers->len > (drbg_max_addtl(drbg))) {
@@ -1150,6 +1151,9 @@ static int drbg_seed(struct drbg_state *drbg, struct drbg_string *pers,
 		BUG_ON((entropylen * 2) > sizeof(entropy));
 
 		/* Get seed from in-kernel /dev/urandom */
+		if (!rng_is_initialized())
+			new_seed_state = DRBG_SEED_STATE_PARTIAL;
+
 		ret = drbg_get_random_bytes(drbg, entropy, entropylen);
 		if (ret)
 			goto out;
@@ -1206,7 +1210,7 @@ static int drbg_seed(struct drbg_state *drbg, struct drbg_string *pers,
 		memset(drbg->C, 0, drbg_statelen(drbg));
 	}
 
-	ret = __drbg_seed(drbg, &seedlist, reseed);
+	ret = __drbg_seed(drbg, &seedlist, reseed, new_seed_state);
 
 out:
 	memzero_explicit(entropy, entropylen * 2);
diff --git a/include/crypto/drbg.h b/include/crypto/drbg.h
index 92a87b23ad2f..3ebdb1effe74 100644
--- a/include/crypto/drbg.h
+++ b/include/crypto/drbg.h
@@ -107,6 +107,7 @@ struct drbg_test_data {
 
 enum drbg_seed_state {
 	DRBG_SEED_STATE_UNSEEDED,
+	DRBG_SEED_STATE_PARTIAL, /* Seeded with !rng_is_initialized() */
 	DRBG_SEED_STATE_FULL,
 };
 
-- 
2.26.2

[PATCH 3/6] crypto: DRBG - move dynamic ->reseed_threshold adjustments to __drbg_seed()

[Nicolai Stange]

Since commit 42ea507fae1a ("crypto: drbg - reseed often if seedsource is
degraded"), the maximum seed lifetime represented by ->reseed_threshold
gets temporarily lowered if the get_random_bytes() source cannot provide
sufficient entropy yet, as is common during boot, and restored back to
the original value again once that has changed.

More specifically, if the add_random_ready_callback() invoked from
drbg_prepare_hrng() in the course of DRBG instantiation does not return
-EALREADY, that is, if get_random_bytes() has not been fully initialized
at this point yet, drbg_prepare_hrng() will lower ->reseed_threshold
to a value of 50. The drbg_async_seed() scheduled from said
random_ready_callback will eventually restore the original value.

A future patch will replace the random_ready_callback based notification
mechanism and thus, there will be no add_random_ready_callback() return
value anymore which could get compared to -EALREADY.

However, there's __drbg_seed() which gets invoked in the course of both,
the DRBG instantiation as well as the eventual reseeding from
get_random_bytes() in aforementioned drbg_async_seed(), if any. Moreover,
it knows about the get_random_bytes() initialization state by the time the
seed data had been obtained from it: the new_seed_state argument introduced
with the previous patch would get set to DRBG_SEED_STATE_PARTIAL in case
get_random_bytes() had not been fully initialized yet and to
DRBG_SEED_STATE_FULL otherwise. Thus, __drbg_seed() provides a convenient
alternative for managing that ->reseed_threshold lowering and restoring at
a central place.

Move all ->reseed_threshold adjustment code from drbg_prepare_hrng() and
drbg_async_seed() respectively to __drbg_seed(). Make __drbg_seed()
lower the ->reseed_threshold to 50 in case its new_seed_state argument
equals DRBG_SEED_STATE_PARTIAL and let it restore the original value
otherwise.

There is no change in behaviour.

Signed-off-by: Nicolai Stange <nstange@suse.de>
---
 crypto/drbg.c | 30 +++++++++++++++++++++---------
 1 file changed, 21 insertions(+), 9 deletions(-)

diff --git a/crypto/drbg.c b/crypto/drbg.c
index 55fe8caf569a..6aad210f101a 100644
--- a/crypto/drbg.c
+++ b/crypto/drbg.c
@@ -1047,6 +1047,27 @@ static inline int __drbg_seed(struct drbg_state *drbg, struct list_head *seed,
 	/* 10.1.1.2 / 10.1.1.3 step 5 */
 	drbg->reseed_ctr = 1;
 
+	switch (drbg->seeded) {
+	case DRBG_SEED_STATE_UNSEEDED:
+		/* Impossible, but handle it to silence compiler warnings. */
+		fallthrough;
+	case DRBG_SEED_STATE_PARTIAL:
+		/*
+		 * Require frequent reseeds until the seed source is
+		 * fully initialized.
+		 */
+		drbg->reseed_threshold = 50;
+		break;
+
+	case DRBG_SEED_STATE_FULL:
+		/*
+		 * Seed source has become fully initialized, frequent
+		 * reseeds no longer required.
+		 */
+		drbg->reseed_threshold = drbg_max_requests(drbg);
+		break;
+	}
+
 	return ret;
 }
 
@@ -1095,9 +1116,6 @@ static void drbg_async_seed(struct work_struct *work)
 
 	__drbg_seed(drbg, &seedlist, true, DRBG_SEED_STATE_FULL);
 
-	if (drbg->seeded == DRBG_SEED_STATE_FULL)
-		drbg->reseed_threshold = drbg_max_requests(drbg);
-
 unlock:
 	mutex_unlock(&drbg->drbg_mutex);
 
@@ -1534,12 +1552,6 @@ static int drbg_prepare_hrng(struct drbg_state *drbg)
 		return err;
 	}
 
-	/*
-	 * Require frequent reseeds until the seed source is fully
-	 * initialized.
-	 */
-	drbg->reseed_threshold = 50;
-
 	return err;
 }
 
-- 
2.26.2

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

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

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;
 
+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)
-- 
2.26.2

[PATCH 5/6] crypto: DRBG - make drbg_prepare_hrng() handle jent instantiation errors

[Nicolai Stange]

Now that drbg_prepare_hrng() doesn't do anything but to instantiate a
jitterentropy crypto_rng instance, it looks a little odd to have the
related error handling at its only caller, drbg_instantiate().

Move the handling of jitterentropy allocation failures from
drbg_instantiate() close to the allocation itself in drbg_prepare_hrng().

There is no change in behaviour.

Signed-off-by: Nicolai Stange <nstange@suse.de>
---
 crypto/drbg.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/crypto/drbg.c b/crypto/drbg.c
index d9f7dddfd683..f5947211e0bf 100644
--- a/crypto/drbg.c
+++ b/crypto/drbg.c
@@ -1518,6 +1518,14 @@ static int drbg_prepare_hrng(struct drbg_state *drbg)
 		return 0;
 
 	drbg->jent = crypto_alloc_rng("jitterentropy_rng", 0, 0);
+	if (IS_ERR(drbg->jent)) {
+		const int err = PTR_ERR(drbg->jent);
+
+		drbg->jent = NULL;
+		if (fips_enabled || err != -ENOENT)
+			return err;
+		pr_info("DRBG: Continuing without Jitter RNG\n");
+	}
 
 	return 0;
 }
@@ -1573,14 +1581,6 @@ static int drbg_instantiate(struct drbg_state *drbg, struct drbg_string *pers,
 		if (ret)
 			goto free_everything;
 
-		if (IS_ERR(drbg->jent)) {
-			ret = PTR_ERR(drbg->jent);
-			drbg->jent = NULL;
-			if (fips_enabled || ret != -ENOENT)
-				goto free_everything;
-			pr_info("DRBG: Continuing without Jitter RNG\n");
-		}
-
 		reseed = false;
 	}
 
-- 
2.26.2

[PATCH 6/6] crypto: DRBG - reseed 'nopr' drbgs periodically from get_random_bytes()

[Nicolai Stange]

In contrast to the fully prediction resistant 'pr' DRBGs, the 'nopr'
variants get seeded once at boot and reseeded only rarely thereafter,
namely only after 2^20 requests have been served each. AFAICT, this
reseeding based on the number of requests served is primarily motivated
by information theoretic considerations, c.f. NIST SP800-90Ar1,
sec. 8.6.8 ("Reseeding").

However, given the relatively large seed lifetime of 2^20 requests, the
'nopr' DRBGs can hardly be considered to provide any prediction resistance
whatsoever, i.e. to protect against threats like side channel leaks of the
internal DRBG state (think e.g. leaked VM snapshots). This is expected and
completely in line with the 'nopr' naming, but as e.g. the
"drbg_nopr_hmac_sha512" implementation is potentially being used for
providing the "stdrng" and thus, the crypto_default_rng serving the
in-kernel crypto, it would certainly be desirable to achieve at least the
same level of prediction resistance as get_random_bytes() does.

Note that the chacha20 rngs underlying get_random_bytes() get reseeded
every CRNG_RESEED_INTERVAL == 5min: the secondary, per-NUMA node rngs from
the primary one and the primary rng in turn from the entropy pool, provided
sufficient entropy is available.

The 'nopr' DRBGs do draw randomness from get_random_bytes() for their
initial seed already, so making them to reseed themselves periodically from
get_random_bytes() in order to let them benefit from the latter's
prediction resistance is not such a big change conceptually.

In principle, it would have been also possible to make the 'nopr' DRBGs to
periodically invoke a full reseeding operation, i.e. to also consider the
jitterentropy source (if enabled) in addition to get_random_bytes() for the
seed value. However, get_random_bytes() is relatively lightweight as
compared to the jitterentropy generation process and thus, even though the
'nopr' reseeding is supposed to get invoked infrequently, it's IMO still
worthwhile to avoid occasional latency spikes for drbg_generate() and
stick to get_random_bytes() only. As an additional remark, note that
drawing randomness from the non-SP800-90B-conforming get_random_bytes()
only won't adversely affect SP800-90A conformance either: the very same is
being done during boot via drbg_seed_from_random() already once
rng_is_initialized() flips to true and it follows that if the DRBG
implementation does conform to SP800-90A now, it will continue to do so.

Make the 'nopr' DRBGs to reseed themselves periodically from
get_random_bytes() every CRNG_RESEED_INTERVAL == 5min.

More specifically, introduce a new member ->last_seed_time to struct
drbg_state for recording in units of jiffies when the last seeding
operation had taken place. Make __drbg_seed() maintain it and let
drbg_generate() invoke a reseed from get_random_bytes() via
drbg_seed_from_random() if more than 5min have passed by since the last
seeding operation. Be careful to not to reseed if in testing mode though,
or otherwise the drbg related tests in crypto/testmgr.c would fail to
reproduce the expected output.

In order to keep the formatting clean in drbg_generate() wrap the logic
for deciding whether or not a reseed is due in a new helper,
drbg_nopr_reseed_interval_elapsed().

Signed-off-by: Nicolai Stange <nstange@suse.de>
---
 crypto/drbg.c         | 26 +++++++++++++++++++++++++-
 include/crypto/drbg.h |  1 +
 2 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/crypto/drbg.c b/crypto/drbg.c
index f5947211e0bf..65e5cd2335c1 100644
--- a/crypto/drbg.c
+++ b/crypto/drbg.c
@@ -100,6 +100,7 @@
 #include <crypto/drbg.h>
 #include <crypto/internal/cipher.h>
 #include <linux/kernel.h>
+#include <linux/jiffies.h>
 
 /***************************************************************
  * Backend cipher definitions available to DRBG
@@ -1044,6 +1045,7 @@ static inline int __drbg_seed(struct drbg_state *drbg, struct list_head *seed,
 		return ret;
 
 	drbg->seeded = new_seed_state;
+	drbg->last_seed_time = jiffies;
 	/* 10.1.1.2 / 10.1.1.3 step 5 */
 	drbg->reseed_ctr = 1;
 
@@ -1114,6 +1116,26 @@ static int drbg_seed_from_random(struct drbg_state *drbg)
 	return ret;
 }
 
+static bool drbg_nopr_reseed_interval_elapsed(struct drbg_state *drbg)
+{
+	unsigned long next_reseed;
+
+	/* Don't ever reseed from get_random_bytes() in test mode. */
+	if (list_empty(&drbg->test_data.list))
+		return false;
+
+	/*
+	 * Obtain fresh entropy for the nopr DRBGs after 300s have
+	 * elapsed in order to still achieve sort of partial
+	 * prediction resistance over the time domain at least. Note
+	 * that the period of 300s has been chosen to match the
+	 * CRNG_RESEED_INTERVAL of the get_random_bytes()' chacha
+	 * rngs.
+	 */
+	next_reseed = drbg->last_seed_time + 300 * HZ;
+	return time_after(jiffies, next_reseed);
+}
+
 /*
  * Seeding or reseeding of the DRBG
  *
@@ -1415,7 +1437,8 @@ static int drbg_generate(struct drbg_state *drbg,
 		/* 9.3.1 step 7.4 */
 		addtl = NULL;
 	} else if (rng_is_initialized() &&
-		   drbg->seeded == DRBG_SEED_STATE_PARTIAL) {
+		   (drbg->seeded == DRBG_SEED_STATE_PARTIAL ||
+		    drbg_nopr_reseed_interval_elapsed(drbg))) {
 		len = drbg_seed_from_random(drbg);
 		if (len)
 			goto err;
@@ -1571,6 +1594,7 @@ static int drbg_instantiate(struct drbg_state *drbg, struct drbg_string *pers,
 		drbg->core = &drbg_cores[coreref];
 		drbg->pr = pr;
 		drbg->seeded = DRBG_SEED_STATE_UNSEEDED;
+		drbg->last_seed_time = 0;
 		drbg->reseed_threshold = drbg_max_requests(drbg);
 
 		ret = drbg_alloc_state(drbg);
diff --git a/include/crypto/drbg.h b/include/crypto/drbg.h
index a6c3b8e7deb6..af5ad51d3eef 100644
--- a/include/crypto/drbg.h
+++ b/include/crypto/drbg.h
@@ -134,6 +134,7 @@ struct drbg_state {
 	struct scatterlist sg_in, sg_out;	/* CTR mode SGLs */
 
 	enum drbg_seed_state seeded;		/* DRBG fully seeded? */
+	unsigned long last_seed_time;
 	bool pr;		/* Prediction resistance enabled? */
 	bool fips_primed;	/* Continuous test primed? */
 	unsigned char *prev;	/* FIPS 140-2 continuous test value */
-- 
2.26.2

Re: [PATCH 0/6] crypto: DRBG - improve 'nopr' reseeding

[Stephan Müller]

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

Re: [PATCH 1/6] crypto: DRBG - prepare for more fine-grained tracking of seeding state

[Stephan Müller]

Am Montag, 25. Oktober 2021, 11:25:20 CEST schrieb Nicolai Stange:

Hi Nicolai,

> There are two different randomness sources the DRBGs are getting seeded
> from, namely the jitterentropy source (if enabled) and get_random_bytes().
> At initial DRBG seeding time during boot, the latter might not have
> collected sufficient entropy for seeding itself yet and thus, the DRBG
> implementation schedules a reseed work from a random_ready_callback once
> that has happened. This is particularly important for the !->pr DRBG
> instances, for which (almost) no further reseeds are getting triggered
> during their lifetime.
> 
> Because collecting data from the jitterentropy source is a rather expensive
> operation, the aforementioned asynchronously scheduled reseed work
> restricts itself to get_random_bytes() only. That is, it in some sense
> amends the initial DRBG seed derived from jitterentropy output at full
> (estimated) entropy with fresh randomness obtained from get_random_bytes()
> once that has been seeded with sufficient entropy itself.
> 
> With the advent of rng_is_initialized(), there is no real need for doing
> the reseed operation from an asynchronously scheduled work anymore and a
> subsequent patch will make it synchronous by moving it next to related
> logic already present in drbg_generate().
> 
> However, for tracking whether a full reseed including the jitterentropy
> source is required or a "partial" reseed involving only get_random_bytes()
> would be sufficient already, the boolean struct drbg_state's ->seeded
> member must become a tristate value.
> 
> Prepare for this by introducing the new enum drbg_seed_state and change
> struct drbg_state's ->seeded member's type from bool to that type.
> 
> For facilitating review, enum drbg_seed_state is made to only contain
> two members corresponding to the former ->seeded values of false and true
> resp. at this point: DRBG_SEED_STATE_UNSEEDED and DRBG_SEED_STATE_FULL. A
> third one for tracking the intermediate state of "seeded from jitterentropy
> only" will be introduced with a subsequent patch.
> 
> There is no change in behaviour at this point.
> 
> Signed-off-by: Nicolai Stange <nstange@suse.de>

Reviewed-by: Stephan Müller <smueller@chronox.de>

Ciao
Stephan

Re: [PATCH 2/6] crypto: DRBG - track whether DRBG was seeded with !rng_is_initialized()

[Stephan Müller]

Am Montag, 25. Oktober 2021, 11:25:21 CEST schrieb Nicolai Stange:

Hi Nicolai,

> Currently, the DRBG implementation schedules asynchronous works from
> random_ready_callbacks for reseeding the DRBG instances with output from
> get_random_bytes() once the latter has sufficient entropy available.
> 
> However, as the get_random_bytes() initialization state can get queried by
> means of rng_is_initialized() now, there is no real need for this
> asynchronous reseeding logic anymore and it's better to keep things simple
> by doing it synchronously when needed instead, i.e. from drbg_generate()
> once rng_is_initialized() has flipped to true.
> 
> Of course, for this to work, drbg_generate() would need some means by which
> it can tell whether or not rng_is_initialized() has flipped to true since
> the last seeding from get_random_bytes(). Or equivalently, whether or not
> the last seed from get_random_bytes() has happened when
> rng_is_initialized() was still evaluating to false.
> 
> As it currently stands, enum drbg_seed_state allows for the representation
> of two different DRBG seeding states: DRBG_SEED_STATE_UNSEEDED and
> DRBG_SEED_STATE_FULL. The former makes drbg_generate() to invoke a full
> reseeding operation involving both, the rather expensive jitterentropy as
> well as the get_random_bytes() randomness sources. The DRBG_SEED_STATE_FULL
> state on the other hand implies that no reseeding at all is required for a
> !->pr DRBG variant.
> 
> Introduce the new DRBG_SEED_STATE_PARTIAL state to enum drbg_seed_state for
> representing the condition that a DRBG was being seeded when
> rng_is_initialized() had still been false. In particular, this new state
> implies that
> - the given DRBG instance has been fully seeded from the jitterentropy
>   source (if enabled)
> - and drbg_generate() is supposed to reseed from get_random_bytes()
>   *only* once rng_is_initialized() turns to true.
> 
> Up to now, the __drbg_seed() helper used to set the given DRBG instance's
> ->seeded state to constant DRBG_SEED_STATE_FULL. Introduce a new argument
> allowing for the specification of the to be written ->seeded value instead.
> Make the first of its two callers, drbg_seed(), determine the appropriate
> value based on rng_is_initialized(). The remaining caller,
> drbg_async_seed(), is known to get invoked only once rng_is_initialized()
> is true, hence let it pass constant DRBG_SEED_STATE_FULL for the new
> argument to __drbg_seed().
> 
> There is no change in behaviour, except for that the pr_devel() in
> drbg_generate() would now report "unseeded" for ->pr DRBG instances which
> had last been seeded when rng_is_initialized() was still evaluating to
> false.
> 
> Signed-off-by: Nicolai Stange <nstange@suse.de>

Reviewed-by: Stephan Müller <smueller@chronox.de>

Ciao
Stephan

Re: [PATCH 3/6] crypto: DRBG - move dynamic ->reseed_threshold adjustments to __drbg_seed()

[Stephan Müller]

Am Montag, 25. Oktober 2021, 11:25:22 CEST schrieb Nicolai Stange:

Hi Nicolai,

> Since commit 42ea507fae1a ("crypto: drbg - reseed often if seedsource is
> degraded"), the maximum seed lifetime represented by ->reseed_threshold
> gets temporarily lowered if the get_random_bytes() source cannot provide
> sufficient entropy yet, as is common during boot, and restored back to
> the original value again once that has changed.
> 
> More specifically, if the add_random_ready_callback() invoked from
> drbg_prepare_hrng() in the course of DRBG instantiation does not return
> -EALREADY, that is, if get_random_bytes() has not been fully initialized
> at this point yet, drbg_prepare_hrng() will lower ->reseed_threshold
> to a value of 50. The drbg_async_seed() scheduled from said
> random_ready_callback will eventually restore the original value.
> 
> A future patch will replace the random_ready_callback based notification
> mechanism and thus, there will be no add_random_ready_callback() return
> value anymore which could get compared to -EALREADY.
> 
> However, there's __drbg_seed() which gets invoked in the course of both,
> the DRBG instantiation as well as the eventual reseeding from
> get_random_bytes() in aforementioned drbg_async_seed(), if any. Moreover,
> it knows about the get_random_bytes() initialization state by the time the
> seed data had been obtained from it: the new_seed_state argument introduced
> with the previous patch would get set to DRBG_SEED_STATE_PARTIAL in case
> get_random_bytes() had not been fully initialized yet and to
> DRBG_SEED_STATE_FULL otherwise. Thus, __drbg_seed() provides a convenient
> alternative for managing that ->reseed_threshold lowering and restoring at
> a central place.
> 
> Move all ->reseed_threshold adjustment code from drbg_prepare_hrng() and
> drbg_async_seed() respectively to __drbg_seed(). Make __drbg_seed()
> lower the ->reseed_threshold to 50 in case its new_seed_state argument
> equals DRBG_SEED_STATE_PARTIAL and let it restore the original value
> otherwise.
> 
> There is no change in behaviour.
> 
> Signed-off-by: Nicolai Stange <nstange@suse.de>

Reviewed-by: Stephan Müller <smueller@chronox.de>

Ciao
Stephan

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

[Stephan Müller]

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

Re: [PATCH 5/6] crypto: DRBG - make drbg_prepare_hrng() handle jent instantiation errors

[Stephan Müller]

Am Montag, 25. Oktober 2021, 11:25:24 CEST schrieb Nicolai Stange:

Hi Nicolai,

> Now that drbg_prepare_hrng() doesn't do anything but to instantiate a
> jitterentropy crypto_rng instance, it looks a little odd to have the
> related error handling at its only caller, drbg_instantiate().
> 
> Move the handling of jitterentropy allocation failures from
> drbg_instantiate() close to the allocation itself in drbg_prepare_hrng().
> 
> There is no change in behaviour.
> 
> Signed-off-by: Nicolai Stange <nstange@suse.de>

Reviewed-by: Stephan Müller <smueller@chronox.de>

Ciao
Stephan

Re: [PATCH 6/6] crypto: DRBG - reseed 'nopr' drbgs periodically from get_random_bytes()

[Stephan Müller]

Am Montag, 25. Oktober 2021, 11:25:25 CEST schrieb Nicolai Stange:

Hi Nicolai,

> In contrast to the fully prediction resistant 'pr' DRBGs, the 'nopr'
> variants get seeded once at boot and reseeded only rarely thereafter,
> namely only after 2^20 requests have been served each. AFAICT, this
> reseeding based on the number of requests served is primarily motivated
> by information theoretic considerations, c.f. NIST SP800-90Ar1,
> sec. 8.6.8 ("Reseeding").
> 
> However, given the relatively large seed lifetime of 2^20 requests, the
> 'nopr' DRBGs can hardly be considered to provide any prediction resistance
> whatsoever, i.e. to protect against threats like side channel leaks of the
> internal DRBG state (think e.g. leaked VM snapshots). This is expected and
> completely in line with the 'nopr' naming, but as e.g. the
> "drbg_nopr_hmac_sha512" implementation is potentially being used for
> providing the "stdrng" and thus, the crypto_default_rng serving the
> in-kernel crypto, it would certainly be desirable to achieve at least the
> same level of prediction resistance as get_random_bytes() does.
> 
> Note that the chacha20 rngs underlying get_random_bytes() get reseeded
> every CRNG_RESEED_INTERVAL == 5min: the secondary, per-NUMA node rngs from
> the primary one and the primary rng in turn from the entropy pool, provided
> sufficient entropy is available.
> 
> The 'nopr' DRBGs do draw randomness from get_random_bytes() for their
> initial seed already, so making them to reseed themselves periodically from
> get_random_bytes() in order to let them benefit from the latter's
> prediction resistance is not such a big change conceptually.
> 
> In principle, it would have been also possible to make the 'nopr' DRBGs to
> periodically invoke a full reseeding operation, i.e. to also consider the
> jitterentropy source (if enabled) in addition to get_random_bytes() for the
> seed value. However, get_random_bytes() is relatively lightweight as
> compared to the jitterentropy generation process and thus, even though the
> 'nopr' reseeding is supposed to get invoked infrequently, it's IMO still
> worthwhile to avoid occasional latency spikes for drbg_generate() and
> stick to get_random_bytes() only. As an additional remark, note that
> drawing randomness from the non-SP800-90B-conforming get_random_bytes()
> only won't adversely affect SP800-90A conformance either: the very same is
> being done during boot via drbg_seed_from_random() already once
> rng_is_initialized() flips to true and it follows that if the DRBG
> implementation does conform to SP800-90A now, it will continue to do so.
> 
> Make the 'nopr' DRBGs to reseed themselves periodically from
> get_random_bytes() every CRNG_RESEED_INTERVAL == 5min.
> 
> More specifically, introduce a new member ->last_seed_time to struct
> drbg_state for recording in units of jiffies when the last seeding
> operation had taken place. Make __drbg_seed() maintain it and let
> drbg_generate() invoke a reseed from get_random_bytes() via
> drbg_seed_from_random() if more than 5min have passed by since the last
> seeding operation. Be careful to not to reseed if in testing mode though,
> or otherwise the drbg related tests in crypto/testmgr.c would fail to
> reproduce the expected output.
> 
> In order to keep the formatting clean in drbg_generate() wrap the logic
> for deciding whether or not a reseed is due in a new helper,
> drbg_nopr_reseed_interval_elapsed().
> 
> Signed-off-by: Nicolai Stange <nstange@suse.de>

For the code review:

Reviewed-by: Stephan Müller <smueller@chronox.de>

But with respect to the overall architecture of the seeding in the entire 
kernel, this is insufficient (note, I am not saying that this patch series 
should and can fix it though). It is insufficient, because:

- reseeding does not happen if new data is received by the kernel entropy 
gathering functions like the RNDADDENTROPY IOCTL or add_hwgenerator_randomness 
- i.e. externally provided data lingers without being used in the DRBG

- reseeding does not consider the amount of entropy added from the entropy 
sources allowing potential pathological weak reseeding operation

... and other seeding problems in random.c...

Ciao
Stephan

Re: [PATCH 0/6] crypto: DRBG - improve 'nopr' reseeding

[Nicolai Stange]

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.

-- 
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg), GF: Felix Imendörffer

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

[Nicolai Stange]

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)

-- 
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg), GF: Felix Imendörffer

Re: [PATCH 0/6] crypto: DRBG - improve 'nopr' reseeding

[Stephan Müller]

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

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