Re: [RFC] arm64: syscall: Direct PRNG kstack randomization

From: Jeremy Linton <jeremy.linton@arm.com>
To: "Jason A. Donenfeld" <Jason@zx2c4.com>,
	Kees Cook <keescook@chromium.org>
Cc: linux-arm-kernel@lists.infradead.org, catalin.marinas@arm.com,
	will@kernel.org, gustavoars@kernel.org, mark.rutland@arm.com,
	rostedt@goodmis.org, arnd@arndb.de, broonie@kernel.org,
	guohui@uniontech.com, Manoj.Iyer@arm.com,
	linux-kernel@vger.kernel.org, linux-hardening@vger.kernel.org,
	James Yang <james.yang@arm.com>,
	Shiyou Huang <shiyou.huang@arm.com>
Subject: Re: [RFC] arm64: syscall: Direct PRNG kstack randomization
Date: Wed, 21 Feb 2024 10:54:58 -0600	[thread overview]
Message-ID: <305d26ba-8738-4244-801f-939e1a9730a1@arm.com> (raw)
In-Reply-To: <CAHmME9pYS=YZM1b0ftrjj_qjz_U4V8PsiLzB15JmBRr39Av2Ng@mail.gmail.com>

Hi,

Thanks for looking at this.

On 2/21/24 06:44, Jason A. Donenfeld wrote:
> Hi,
> 
> On Wed, Feb 21, 2024 at 7:33 AM Kees Cook <keescook@chromium.org> wrote:
>>> +#ifdef CONFIG_RANDOMIZE_KSTACK_OFFSET
>>> +DEFINE_PER_CPU(u32, kstackrng);
>>> +static u32 xorshift32(u32 state)
>>> +{
>>> +     /*
>>> +      * From top of page 4 of Marsaglia, "Xorshift RNGs"
>>> +      * This algorithm is intended to have a period 2^32 -1
>>> +      * And should not be used anywhere else outside of this
>>> +      * code path.
>>> +      */
>>> +     state ^= state << 13;
>>> +     state ^= state >> 17;
>>> +     state ^= state << 5;
>>> +     return state;
>>> +}
> 
> Can we please *not* introduce yet another RNG? You can't just sprinkle
> this stuff all over the place with no rhyme or reason.
> 
> If you need repeatable randomness, use prandom_u32_state() or similar.
> If you need non-repeatable randomness, use get_random_bytes() or
> similar.

Sure prandom_u32_state() should have a similar effect being a bit 
slower, and a bit better due to the extra hidden state.

> 
> If you think prandom_u32_state() is insufficient for some reason or
> doesn't have some property or performance that you want, submit a
> patch to make it better.
> 
> Looking at the actual intention here, of using repeatable randomness,
> I find the intent pretty weird. Isn't the whole point of kstack
> randomization that you can't predict it? If so, get_random_u*() is
> what you want. If performance isn't sufficient, let's figure out some

There isn't anything wrong with get_random_u16 from a kstack 
randomization standpoint, except for the latency spikes of course.

> way to improve performance. And as Kees said, if the point of this is
> to have some repeatable benchmarks, maybe just don't enable the
> security-intended code whose purpose is non-determinism? Both exploits
> and now apparently benchmarks like determinism.

As I mentioned in the other email, benchmark is probably the wrong word. 
Its a better QoS response time distributions for a given workload. And 
its not strictly in RT kernel latency test types of things, but normal 
memcached style workloads on !RT kernels as well.