Re: [PATCH 2/2] stack: Constrain stack offset randomization with Clang builds

[Kees Cook]

On Fri, Jan 28, 2022 at 08:23:02PM +0100, Marco Elver wrote:
> On Fri, 28 Jan 2022 at 20:10, Kees Cook <keescook@xxxxxxxxxxxx> wrote:
> [...]
> > >       2. Architectures adding add_random_kstack_offset() to syscall
> > >          entry implemented in C require them to be 'noinstr' (e.g. see
> > >          x86 and s390). The potential problem here is that a call to
> > >          memset may occur, which is not noinstr.
> [...]
> > > --- a/arch/Kconfig
> > > +++ b/arch/Kconfig
> > > @@ -1163,6 +1163,7 @@ config RANDOMIZE_KSTACK_OFFSET
> > >       bool "Support for randomizing kernel stack offset on syscall entry" if EXPERT
> > >       default y
> > >       depends on HAVE_ARCH_RANDOMIZE_KSTACK_OFFSET
> > > +     depends on INIT_STACK_NONE || !CC_IS_CLANG || CLANG_VERSION >= 140000
> >
> > This makes it _unavailable_ for folks with Clang < 14, which seems
> > too strong, especially since it's run-time off by default. I'd prefer
> > dropping this hunk and adding some language to the _DEFAULT help noting
> > the specific performance impact on Clang < 14.
> 
> You're right, if it was only about performance. But there's the
> correctness issue with ARCH_WANTS_NOINSTR architectures, where we
> really shouldn't emit a call. In those cases, even if compiled in,
> enabling the feature may cause trouble.

Hrm. While I suspect instrumentation of memset() from a C function that is
about to turn on instrumentation is likely quite safe, I guess the size
of the venn diagram overlap of folks wanting to use kstack randomization
and an older Clang quickly approaches zero. But everyone building with
an older Clang gets warnings spewed, so I agree: let's opt for complete
correctness here, and make this >= 14 as you have done.

-- 
Kees Cook