Re: [RFC/RFT PATCH 5/6] random: Plug race in preceding patch

david laight
2025-11-28T11:13:39Z

On Thu, 27 Nov 2025 10:22:32 +0100
Ard Biesheuvel <ardb+git@google.com> wrote:

> From: Ard Biesheuvel <ardb@kernel.org>
>
> The lockless get_random_uXX() reads the next value from the linear
> buffer and then overwrites it with a 0x0 value. This is racy, as the
> code might be re-entered by an interrupt handler, and so the store might
> redundantly wipe the location accessed by the interrupt context rather
> than the interrupted context.

Is overwriting the used value even useful?
If someone manages to read the 'last' value, then they can equally read
the 'next' one - which is likely to be just as useful.
Moreover the zeros tell anyone who has managed the access the buffer
which entry will be used next - without having to find 'batch->position'.

There is probably more to gain from putting the control data in a
completely different piece of memory from the buffer.

David

>
> To plug this race, wipe the preceding location when reading the next
> value from the linear buffer. Given that the position is always non-zero
> outside of the critical section, this is guaranteed to be safe, and
> ensures that the produced values are always wiped from the buffer.
>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
> drivers/char/random.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/char/random.c b/drivers/char/random.c
> index 71bd74871540..e8ba460c5c9c 100644
> --- a/drivers/char/random.c
> +++ b/drivers/char/random.c
> @@ -547,6 +547,7 @@ type get_random_ ##type(void) \
> next = (u64)next_gen << 32; \
> if (likely(batch->position < ARRAY_SIZE(batch->entropy))) { \
> next |= batch->position + 1; /* next-1 is bogus otherwise */ \
> + batch->entropy[batch->position - 1] = 0; \
> ret = batch->entropy[batch->position]; \
> } \
> if (cmpxchg64_local(&batch->posgen, next, next - 1) != next - 1) { \