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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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
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 <[email protected]>
Reviewed-by: Stephan M?ller <[email protected]>
Ciao
Stephan
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 <[email protected]>
Reviewed-by: Stephan M?ller <[email protected]>
Ciao
Stephan
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 <[email protected]>
> ---
> 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
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 <[email protected]>
Reviewed-by: Stephan M?ller <[email protected]>
Ciao
Stephan
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 <[email protected]>
For the code review:
Reviewed-by: Stephan M?ller <[email protected]>
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
Hi Stephan,
first of all, many thanks for your prompt review!
Stephan Müller <[email protected]> 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
Hi Stephan,
Stephan Müller <[email protected]> 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 <[email protected]>
>> ---
>> 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
Am Mittwoch, 27. Oktober 2021, 11:19:50 CEST schrieb Nicolai Stange:
Hi Nicolai,
> Hi Stephan,
>
> Stephan M?ller <[email protected]> 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 <[email protected]>
> >> ---
> >>
> >> 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