linux-kernel - Re: [PATCH v2] random: use SipHash as interrupt entropy accumulator

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:   Sat, 12 Mar 2022 13:39:15 -0800
From:   Eric Biggers <ebiggers@...nel.org>
To:     "Jason A. Donenfeld" <Jason@...c4.com>
Cc:     linux-kernel@...r.kernel.org, linux-crypto@...r.kernel.org,
        Theodore Ts'o <tytso@....edu>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jean-Philippe Aumasson <jeanphilippe.aumasson@...il.com>
Subject: Re: [PATCH v2] random: use SipHash as interrupt entropy accumulator

On Sun, Mar 06, 2022 at 09:51:23AM -0700, Jason A. Donenfeld wrote:
> 
> For this, we make use of SipHash-1-x on 64-bit and HalfSipHash-1-x on
> 32-bit, which are already in use in the kernel and achieve the same
> performance as the function they replace. It would be nice to do two
> rounds, but we don't exactly have the CPU budget handy for that, and one
> round alone is already sufficient.
> 

I'm a bit confused by the argument here.  It's not SipHash-1-x that's used
elsewhere in the kernel, but rather SipHash-2-4.  HalfSipHash-1-3 is used too,
but the documentation (Documentation/security/siphash.rst) is very explicit that
HalfSipHash-1-3 must only ever be used as a hashtable key function.

It sounds like you're not actually claiming cryptographic security here, and
this is probably better than the homebrew ARX that was there before.  But there
are definitely some contradictions in your explanation.

>  struct fast_pool {
> -	union {
> -		u32 pool32[4];
> -		u64 pool64[2];
> -	};
>  	struct work_struct mix;
> +	unsigned long pool[4];
>  	unsigned long last;
>  	unsigned int count;
>  	u16 reg_idx;
>  };

Increasing the size of the pool partially breaks mix_interrupt_randomness()
which does:

static void mix_interrupt_randomness(struct work_struct *work)
{
        struct fast_pool *fast_pool = container_of(work, struct fast_pool, mix);
        u8 pool[16];

	[...]

        /*
         * Copy the pool to the stack so that the mixer always has a
         * consistent view, before we reenable irqs again.
         */
        memcpy(pool, fast_pool->pool, sizeof(pool));

	[...]

So on 64-bit platforms it now throws away half of the pool.

It should use 'u8 pool[sizeof(fast_pool->pool)]' to avoid hardcoding a size.

- Eric