Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
From:   Michael Ellerman <mpe@ellerman.id.au>
To:     Kees Cook <keescook@chromium.org>,
        Nicholas Piggin <npiggin@gmail.com>
Cc:     benh@kernel.crashing.org, christophe.leroy@csgroup.eu,
        mark.rutland@arm.com, paulus@samba.org, tglx@linutronix.de,
        Xiu Jianfeng <xiujianfeng@huawei.com>,
        linux-hardening@vger.kernel.org, linux-kernel@vger.kernel.org,
        linuxppc-dev@lists.ozlabs.org
Subject: Re: [PATCH -next] powerpc: add support for syscall stack randomization
In-Reply-To: <202205100917.5480D91@keescook>
References: <20220505111932.228814-1-xiujianfeng@huawei.com>
 <1652173338.7bltwybi0c.astroid@bobo.none> <202205100917.5480D91@keescook>
Date:   Thu, 12 May 2022 23:03:57 +1000
Message-ID: <87r14y7via.fsf@mpe.ellerman.id.au>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Kees Cook <keescook@chromium.org> writes:
> On Tue, May 10, 2022 at 07:23:46PM +1000, Nicholas Piggin wrote:
...
>> 
>> I wonder why the choose is separated from the add? I guess it's to
>> avoid a data dependency for stack access on an expensive random
>> function, so that makes sense (a comment would be nice in the
>> generic code).
>
> How does this read? I can send a "real" patch if it looks good:
>
>
> diff --git a/include/linux/randomize_kstack.h b/include/linux/randomize_kstack.h
> index 1468caf001c0..ad3e80275c74 100644
> --- a/include/linux/randomize_kstack.h
> +++ b/include/linux/randomize_kstack.h
> @@ -40,8 +40,11 @@ DECLARE_PER_CPU(u32, kstack_offset);
>   */
>  #define KSTACK_OFFSET_MAX(x)	((x) & 0x3FF)
>  
> -/*
> - * These macros must be used during syscall entry when interrupts and
> +/**
> + * add_random_kstack_offset - Increase stack utilization by previously
> + *			      chosen random offset
> + *
> + * This should be used in the syscall entry path when interrupts and
 
I would say "called" rather than used, but that's a nit-pick.

>   * preempt are disabled, and after user registers have been stored to
>   * the stack.
>   */
> @@ -55,6 +58,24 @@ DECLARE_PER_CPU(u32, kstack_offset);
>  	}								\
>  } while (0)
>  
> +/**
> + * choose_random_kstack_offset - Choose the random offsset for the next
> + *				 add_random_kstack_offset()

The name "choose" tricked me into thinking the offset is used verbatim.
But it's actually xor'ed into the existing offset.

I was pretty dubious about using mftb (~= rdtsc) based on that, but the
xor makes me less worried.

Obviously you don't want to change the name now, but it would be good if
the doc comment mentioned that the value is combined with the existing
value, not used as-is.

> + * This should only be used during syscall exit when interrupts and
> + * preempt are disabled, and before user registers have been restored
> + * from the stack. This is done to frustrate attack attempts from
> + * userspace to learn the offset:
> + * - Maximize the timing uncertainty visible from userspace: if the
> + *   the offset is chosen at syscall entry, userspace has much more

You have a "the the" across the line-break there.

> + *   control over the timing between chosen offsets. "How long will we
> + *   be in kernel mode?" tends to be more difficult to know than "how
> + *   long will be be in user mode?"
> + * - Reduce the lifetime of the new offset sitting in memory during
> + *   kernel mode execution. Exposures of "thread-local" (e.g. current,
> + *   percpu, etc) memory contents tends to be easier than arbitrary
> + *   location memory exposures.
> + */
>  #define choose_random_kstack_offset(rand) do {				\
>  	if (static_branch_maybe(CONFIG_RANDOMIZE_KSTACK_OFFSET_DEFAULT,	\
>  				&randomize_kstack_offset)) {		\
>

cheers