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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <202201281141.2491039E@keescook>
Date:   Fri, 28 Jan 2022 11:59:28 -0800
From:   Kees Cook <keescook@...omium.org>
To:     Marco Elver <elver@...gle.com>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...nel.org>,
        Nathan Chancellor <nathan@...nel.org>,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        Elena Reshetova <elena.reshetova@...el.com>,
        Alexander Potapenko <glider@...gle.com>, llvm@...ts.linux.dev,
        kasan-dev@...glegroups.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] stack: Constrain stack offset randomization with
 Clang builds

On Fri, Jan 28, 2022 at 08:23:02PM +0100, Marco Elver wrote:
> On Fri, 28 Jan 2022 at 20:10, Kees Cook <keescook@...omium.org> 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