On Tue, 7 Jun 2022 at 12:42, Jason A. Donenfeld [off-list ref] wrote:

Hey again,

On Tue, Jun 07, 2022 at 12:28:08PM +0200, Jason A. Donenfeld wrote:

quoted

Hi Ard,

On Tue, Jun 07, 2022 at 12:13:29PM +0200, Ard Biesheuvel wrote:

quoted

Hi Jason,

On Tue, 7 Jun 2022 at 12:04, Jason A. Donenfeld [off-list ref] wrote:

quoted

[ I would like to pursue fixing this more directly first before actually
  merging this, but I thought I'd send this to the list now anyway as a
  the "backup" plan. If I can't figure out how to make headway on the
  main plan in the next few days, it'll be easy to just do this. ]

What more direct fix did you have in mind here?

A non-broken version of https://lore.kernel.org/lkml/20220603121543.360283-1-Jason@zx2c4.com/ (local)

As I mentioned in https://lore.kernel.org/lkml/Yp8kQrBgE3WVqqC5@zx2c4.com/ (local) ,

    I would like a few days to see if there's some trivial way of not
    needing that on arm32. If it turns out to be easy, then I'd prefer the
    direct fix akin to the arm64 one. If it turns out to be not easy, then
    I'll merge the backup commit.

quoted

quoted

diff --git a/drivers/char/random.c b/drivers/char/random.c
index 4862d4d3ec49..f9a020ec08b9 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c

@@ -650,7 +650,8 @@ static void __cold _credit_init_bits(size_t bits)

        if (orig < POOL_READY_BITS && new >= POOL_READY_BITS) {
                crng_reseed(); /* Sets crng_init to CRNG_READY under base_crng.lock. */
-               execute_in_process_context(crng_set_ready, &set_ready);
+               if (static_key_initialized)
+                       execute_in_process_context(crng_set_ready, &set_ready);

Can we just drop this entirely, and rely on the hunk below to set the
static key? What justifies having two code paths that set the static
key in different ways on different architectures?

No, we can't. The hunk below (A) is called from init/main.c some time after
jump_label_init(). The hunk above (B) is called whenever we reach the
256-bit mark.

The order is (B)(A) on machines that get seeded from efi or device tree.
The order is (A)(B) on all other machines, which reach the 256-bit mark
at "some point"... could be after a second, a minute, whenever enough
estimated entropy has been accounted.

Just thinking about this a bit more, I should note that this is not the
first problem caused by EFI/DT doing its thing mega early in the boot
process. Dominik and I fixed up what felt like endless bugs all of
basically that same form back in January. Before it mostly had to do
with workqueues not being available yet. Now it has to do with jump
labels.

So in thinking about how to fix this, the two approaches thus far
discussed are:

1. initialize jump labels earlier, e.g. the arm64 patch (and proposed
   arm32 patch).
2. defer the static key enabling until later, e.g. this patch.

As a third, I could just defer doing anything with the bootloader seed
until random_init(). This might actually be the simplest solution...
I'll sketch something out. A downside, which might be sort of
significant, is that a few odd things actually use randomness before
random_init() is called. So these would miss out on having that seed.
I'll have to look what exactly to see if we're actually getting anything
real out of that.

This is kind of the point of using a firmware provided seed, i.e.,
that it is available much earlier than anything else.

Could we do this to defer the static key manipulation? That way, the
first call to crng_reseed() that occurs after the static keys API
becomes available will set the static key, and patch itself away at
the same time.

---------->8-------------

diff --git a/drivers/char/random.c b/drivers/char/random.c
index b691b9d59503..fad4e1a043ce 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c

@@ -110,7 +110,8 @@ EXPORT_SYMBOL(rng_is_initialized);

 static void __cold crng_set_ready(struct work_struct *work)
 {
-       static_branch_enable(&crng_is_ready);
+       if (static_key_initialized)
+               static_branch_enable(&crng_is_ready);
 }

 /* Used by wait_for_random_bytes(), and considered an entropy

collector, below. */

@@ -202,6 +203,7 @@ static void extract_entropy(void *buf, size_t len);
 /* This extracts a new crng key from the input pool. */
 static void crng_reseed(void)
 {
+       static struct execute_work set_ready;
        unsigned long flags;
        unsigned long next_gen;
        u8 key[CHACHA_KEY_SIZE];

@@ -221,8 +223,10 @@ static void crng_reseed(void)
                ++next_gen;
        WRITE_ONCE(base_crng.generation, next_gen);
        WRITE_ONCE(base_crng.birth, jiffies);
-       if (!static_branch_likely(&crng_is_ready))
+       if (!static_branch_likely(&crng_is_ready)) {
+               execute_in_process_context(crng_set_ready, &set_ready);
                crng_init = CRNG_READY;
+       }
        spin_unlock_irqrestore(&base_crng.lock, flags);
        memzero_explicit(key, sizeof(key));
 }

@@ -634,7 +638,6 @@ static void extract_entropy(void *buf, size_t len)

 static void __cold _credit_init_bits(size_t bits)
 {
-       static struct execute_work set_ready;
        unsigned int new, orig, add;
        unsigned long flags;

@@ -650,7 +653,6 @@ static void __cold _credit_init_bits(size_t bits)

        if (orig < POOL_READY_BITS && new >= POOL_READY_BITS) {
                crng_reseed(); /* Sets crng_init to CRNG_READY under

base_crng.lock. */
-               execute_in_process_context(crng_set_ready, &set_ready);
                wake_up_interruptible(&crng_init_wait);
                kill_fasync(&fasync, SIGIO, POLL_IN);
                pr_notice("crng init done\n");

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel