linux-kernel - Re: [PATCH v6] random: defer fast pool mixing to worker

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 11 Feb 2022 17:44:18 +0100
From:   Sebastian Andrzej Siewior <bigeasy@...utronix.de>
To:     "Jason A. Donenfeld" <Jason@...c4.com>
Cc:     linux-kernel@...r.kernel.org, linux-crypto@...r.kernel.org,
        Thomas Gleixner <tglx@...utronix.de>,
        Peter Zijlstra <peterz@...radead.org>,
        Theodore Ts'o <tytso@....edu>,
        Sultan Alsawaf <sultan@...neltoast.com>,
        Jonathan Neuschäfer <j.neuschaefer@....net>,
        Dominik Brodowski <linux@...inikbrodowski.net>
Subject: Re: [PATCH v6] random: defer fast pool mixing to worker

On 2022-02-11 17:25:15 [+0100], Jason A. Donenfeld wrote:
> diff --git a/drivers/char/random.c b/drivers/char/random.c
> index c42c07a7eb56..20b11a4b6559 100644
> --- a/drivers/char/random.c
> +++ b/drivers/char/random.c
> @@ -1214,12 +1215,59 @@ static u32 get_reg(struct fast_pool *f, struct pt_regs *regs)
>  	return *ptr;
>  }
>  
> +static void mix_interrupt_randomness(struct work_struct *work)
> +{
> +	struct fast_pool *fast_pool = container_of(work, struct fast_pool, mix);
> +	unsigned long pool[ARRAY_SIZE(fast_pool->pool)];
> +	int count;
> +
> +	/* Check to see if we're running on the wrong CPU due to hotplug. */
> +	migrate_disable();
> +	if (fast_pool != this_cpu_ptr(&irq_randomness)) {
> +		migrate_enable();
> +		/*
> +		 * If we are unlucky enough to have been moved to another CPU,

+ "during CPU hotplug while the CPU was shutdown". It should not look
like the worker can be migrated on system without CPU-hotplug involved.

> +		 * then we set our count to zero atomically so that when the
> +		 * CPU comes back online, it can enqueue work again. The
> +		 * _release here pairs with the atomic_inc_return_acquire in
> +		 * add_interrupt_randomness().
> +		 */
> +		atomic_set_release(&fast_pool->count, 0);
> +		return;
> +	}
> +
> +	/*
> +	 * Copy the pool to the stack so that the mixer always has a
> +	 * consistent view. It's extremely unlikely but possible that
> +	 * this 2 or 4 word read is interrupted by an irq, but in case
> +	 * it is, we double check that count stays the same.
> +	 *
> +	 * We set the count to 0 so that irqs can immediately begin to
> +	 * accumulate again after. Since any possible interruptions
> +	 * at this stage are guaranteed to be on the same CPU, we can
> +	 * use cmpxchg_relaxed.
> +	 */
> +	count = atomic_read(&fast_pool->count);
> +	do {
> +		memcpy(pool, fast_pool->pool, sizeof(pool));
> +	} while (atomic_try_cmpxchg_relaxed(&fast_pool->count, &count, 0));

I *think* we could drop that "fast_pool !=
this_cpu_ptr(&irq_randomness)" check at the top since that cmpxchg will
save us and redo the loop. But if I remember correctly you worried about
fast_pool->pool being modified (which is only a corner case if we are on
the other CPU while the orig CPU is back again). Either way, it would be
random and we would not consume more entropy.

So if we have to keep this then please swap that migrate_disable() with
local_irq_disable(). Otherwise PeterZ will yell at me.

> +	fast_pool->last = jiffies;
> +	migrate_enable();
> +
> +	mix_pool_bytes(pool, sizeof(pool));
> +	credit_entropy_bits(1);
> +	memzero_explicit(pool, sizeof(pool));
> +}
> +
…
> @@ -1235,12 +1283,13 @@ void add_interrupt_randomness(int irq)
>  	}
>  
>  	fast_mix((u32 *)fast_pool->pool);
> -	++fast_pool->count;
> +	/* The _acquire here pairs with the atomic_set_release in mix_interrupt_randomness(). */
> +	new_count = (unsigned int)atomic_inc_return_acquire(&fast_pool->count);
>  
>  	if (unlikely(crng_init == 0)) {
> -		if (fast_pool->count >= 64 &&
> +		if (new_count >= 64 &&
>  		    crng_fast_load(fast_pool->pool, sizeof(fast_pool->pool)) > 0) {
> -			fast_pool->count = 0;
> +			atomic_set(&fast_pool->count, 0);
>  			fast_pool->last = now;

I'm fine if we keep this as is for now.
What do we do here vs RT? I suggested this
  https://git.kernel.org/pub/scm/linux/kernel/git/rt/linux-rt-devel.git/commit/?id=a2d2d54409481aa23a3e11ab9559a843e36a79ec

Is this doable?

Sebastian