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 Date: Thu, 12 May 2022 23:03:57 +1000 [thread overview] Message-ID: <87r14y7via.fsf@mpe.ellerman.id.au> (raw) In-Reply-To: <202205100917.5480D91@keescook> 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
WARNING: multiple messages have this Message-ID (diff)
From: Michael Ellerman <mpe@ellerman.id.au> To: Kees Cook <keescook@chromium.org>, Nicholas Piggin <npiggin@gmail.com> Cc: mark.rutland@arm.com, Xiu Jianfeng <xiujianfeng@huawei.com>, linux-kernel@vger.kernel.org, paulus@samba.org, linux-hardening@vger.kernel.org, tglx@linutronix.de, linuxppc-dev@lists.ozlabs.org Subject: Re: [PATCH -next] powerpc: add support for syscall stack randomization Date: Thu, 12 May 2022 23:03:57 +1000 [thread overview] Message-ID: <87r14y7via.fsf@mpe.ellerman.id.au> (raw) In-Reply-To: <202205100917.5480D91@keescook> 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
next prev parent reply other threads:[~2022-05-12 13:04 UTC|newest] Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-05-05 11:19 [PATCH -next] powerpc: add support for syscall stack randomization Xiu Jianfeng 2022-05-05 11:19 ` Xiu Jianfeng 2022-05-10 9:23 ` Nicholas Piggin 2022-05-10 9:23 ` Nicholas Piggin 2022-05-10 16:19 ` Kees Cook 2022-05-10 16:19 ` Kees Cook 2022-05-11 8:36 ` xiujianfeng 2022-05-11 8:36 ` xiujianfeng 2022-05-12 13:03 ` Michael Ellerman [this message] 2022-05-12 13:03 ` Michael Ellerman 2022-05-11 8:34 ` xiujianfeng 2022-05-11 8:34 ` xiujianfeng 2022-05-12 13:17 ` Michael Ellerman 2022-05-12 13:17 ` Michael Ellerman 2022-05-16 7:29 ` xiujianfeng 2022-05-16 7:29 ` xiujianfeng
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=87r14y7via.fsf@mpe.ellerman.id.au \ --to=mpe@ellerman.id.au \ --cc=benh@kernel.crashing.org \ --cc=christophe.leroy@csgroup.eu \ --cc=keescook@chromium.org \ --cc=linux-hardening@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linuxppc-dev@lists.ozlabs.org \ --cc=mark.rutland@arm.com \ --cc=npiggin@gmail.com \ --cc=paulus@samba.org \ --cc=tglx@linutronix.de \ --cc=xiujianfeng@huawei.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.