* [PATCH 2/2] crypto/drbg: account for no longer returning -EALREADY
2017-10-19 20:45 ` [PATCH 1/2] random: always call random ready function Jason A. Donenfeld
@ 2017-10-19 20:45 ` Jason A. Donenfeld
2017-10-21 19:22 ` Stephan Mueller
2017-10-19 20:45 ` [PATCH 1/2] random: always call random ready function Jason A. Donenfeld
2017-10-19 20:58 ` Kees Cook
2 siblings, 1 reply; 6+ messages in thread
From: Jason A. Donenfeld @ 2017-10-19 20:45 UTC (permalink / raw)
To: Kees Cook, LKML, linux-crypto, herbert, tytso, gregkh; +Cc: Jason A. Donenfeld
We now structure things in a way that assumes the seeding function is
always eventually called.
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
crypto/drbg.c | 20 +++++---------------
1 file changed, 5 insertions(+), 15 deletions(-)
diff --git a/crypto/drbg.c b/crypto/drbg.c
index 70018397e59a..501e013cb96c 100644
--- a/crypto/drbg.c
+++ b/crypto/drbg.c
@@ -1411,18 +1411,8 @@ static int drbg_prepare_hrng(struct drbg_state *drbg)
err = add_random_ready_callback(&drbg->random_ready);
- switch (err) {
- case 0:
- break;
-
- case -EALREADY:
- err = 0;
- /* fall through */
-
- default:
- drbg->random_ready.func = NULL;
+ if (err)
return err;
- }
drbg->jent = crypto_alloc_rng("jitterentropy_rng", 0, 0);
@@ -1432,7 +1422,7 @@ static int drbg_prepare_hrng(struct drbg_state *drbg)
*/
drbg->reseed_threshold = 50;
- return err;
+ return 0;
}
/*
@@ -1526,9 +1516,9 @@ 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);
+ del_random_ready_callback(&drbg->random_ready);
+ cancel_work_sync(&drbg->seed_work);
+ if (drbg->jent) {
crypto_free_rng(drbg->jent);
drbg->jent = NULL;
}
--
2.14.2
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 2/2] crypto/drbg: account for no longer returning -EALREADY
2017-10-19 20:45 ` [PATCH 2/2] crypto/drbg: account for no longer returning -EALREADY Jason A. Donenfeld
@ 2017-10-21 19:22 ` Stephan Mueller
0 siblings, 0 replies; 6+ messages in thread
From: Stephan Mueller @ 2017-10-21 19:22 UTC (permalink / raw)
To: Jason A. Donenfeld; +Cc: Kees Cook, LKML, linux-crypto, herbert, tytso, gregkh
Am Donnerstag, 19. Oktober 2017, 22:45:06 CEST schrieb Jason A. Donenfeld:
Hi Jason,
> We now structure things in a way that assumes the seeding function is
> always eventually called.
>
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> ---
> crypto/drbg.c | 20 +++++---------------
> 1 file changed, 5 insertions(+), 15 deletions(-)
>
> diff --git a/crypto/drbg.c b/crypto/drbg.c
> index 70018397e59a..501e013cb96c 100644
> --- a/crypto/drbg.c
> +++ b/crypto/drbg.c
> @@ -1411,18 +1411,8 @@ static int drbg_prepare_hrng(struct drbg_state *drbg)
>
> err = add_random_ready_callback(&drbg->random_ready);
>
> - switch (err) {
> - case 0:
> - break;
> -
> - case -EALREADY:
> - err = 0;
> - /* fall through */
> -
> - default:
> - drbg->random_ready.func = NULL;
> + if (err)
> return err;
Don't you change the logic flow here? In case the add_random_ready_callback
returns 0 because the ready function is already called due to crng_ready(),
the new code above in the patch set continues. But with the current code, it
will return at this point and do not perform the allocation below.
Ciao
Stephan
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] random: always call random ready function
2017-10-19 20:45 ` [PATCH 1/2] random: always call random ready function Jason A. Donenfeld
2017-10-19 20:45 ` [PATCH 2/2] crypto/drbg: account for no longer returning -EALREADY Jason A. Donenfeld
@ 2017-10-19 20:45 ` Jason A. Donenfeld
2017-10-19 20:58 ` Kees Cook
2 siblings, 0 replies; 6+ messages in thread
From: Jason A. Donenfeld @ 2017-10-19 20:45 UTC (permalink / raw)
To: Kees Cook, LKML, Linux Crypto Mailing List, Herbert Xu,
Theodore Ts'o, Greg Kroah-Hartman
Cc: Jason A. Donenfeld
These are mostly untested, but I wanted to spec it out for a taste of
what it looks like.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] random: always call random ready function
2017-10-19 20:45 ` [PATCH 1/2] random: always call random ready function Jason A. Donenfeld
2017-10-19 20:45 ` [PATCH 2/2] crypto/drbg: account for no longer returning -EALREADY Jason A. Donenfeld
2017-10-19 20:45 ` [PATCH 1/2] random: always call random ready function Jason A. Donenfeld
@ 2017-10-19 20:58 ` Kees Cook
2017-10-19 21:12 ` Jason A. Donenfeld
2 siblings, 1 reply; 6+ messages in thread
From: Kees Cook @ 2017-10-19 20:58 UTC (permalink / raw)
To: Jason A. Donenfeld; +Cc: LKML, linux-crypto, Herbert Xu, Ted Ts'o, Greg KH
On Thu, Oct 19, 2017 at 1:45 PM, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> As this interface becomes more heavily used, it will be painful for
> callers to always need to check for -EALREADY.
>
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> ---
> drivers/char/random.c | 24 ++++++++++++++++--------
> 1 file changed, 16 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/char/random.c b/drivers/char/random.c
> index 8ad92707e45f..e161acf35d4a 100644
> --- a/drivers/char/random.c
> +++ b/drivers/char/random.c
> @@ -1556,40 +1556,45 @@ EXPORT_SYMBOL(wait_for_random_bytes);
>
> /*
> * Add a callback function that will be invoked when the nonblocking
> - * pool is initialised.
> + * pool is initialised, or calls that function if it already is.
> *
> * returns: 0 if callback is successfully added
> - * -EALREADY if pool is already initialised (callback not called)
> * -ENOENT if module for callback is not alive
> */
> int add_random_ready_callback(struct random_ready_callback *rdy)
> {
> struct module *owner;
> unsigned long flags;
> - int err = -EALREADY;
>
> - if (crng_ready())
> - return err;
> + BUG_ON(!rdy->func);
Better to WARN_ON() and return a failure.
> +
> + if (crng_ready()) {
> + rdy->func(rdy);
> + rdy->func = NULL;
> + return 0;
> + }
>
> owner = rdy->owner;
> if (!try_module_get(owner))
> return -ENOENT;
>
> spin_lock_irqsave(&random_ready_list_lock, flags);
> - if (crng_ready())
> + if (crng_ready()) {
> + rdy->func(rdy);
> + rdy->func = NULL;
> goto out;
> + }
>
> owner = NULL;
>
> list_add(&rdy->list, &random_ready_list);
> - err = 0;
>
> out:
> spin_unlock_irqrestore(&random_ready_list_lock, flags);
>
> module_put(owner);
>
> - return err;
> + return 0;
> }
> EXPORT_SYMBOL(add_random_ready_callback);
>
> @@ -1601,6 +1606,9 @@ void del_random_ready_callback(struct random_ready_callback *rdy)
> unsigned long flags;
> struct module *owner = NULL;
>
> + if (!rdy->func)
> + return;
Perhaps add a note here about what a NULL func means here? (e.g.
"Already run, not in the list.")
> +
> spin_lock_irqsave(&random_ready_list_lock, flags);
> if (!list_empty(&rdy->list)) {
> list_del_init(&rdy->list);
> --
> 2.14.2
>
Otherwise, yeah, looks sensible.
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply [flat|nested] 6+ messages in thread