[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Tue, 24 Jul 2018 16:59:32 -0700
From: Kees Cook <keescook@...gle.com>
To: Alexander Popov <alex.popov@...ux.com>
Cc: Kernel Hardening <kernel-hardening@...ts.openwall.com>, PaX Team <pageexec@...email.hu>,
Brad Spengler <spender@...ecurity.net>, Ingo Molnar <mingo@...nel.org>,
Andy Lutomirski <luto@...nel.org>, Tycho Andersen <tycho@...ho.ws>, Laura Abbott <labbott@...hat.com>,
Mark Rutland <mark.rutland@....com>, Ard Biesheuvel <ard.biesheuvel@...aro.org>,
Borislav Petkov <bp@...en8.de>, Richard Sandiford <richard.sandiford@....com>,
Thomas Gleixner <tglx@...utronix.de>, "H . Peter Anvin" <hpa@...or.com>,
Peter Zijlstra <a.p.zijlstra@...llo.nl>, "Dmitry V . Levin" <ldv@...linux.org>,
Emese Revfy <re.emese@...il.com>, Jonathan Corbet <corbet@....net>,
Andrey Ryabinin <aryabinin@...tuozzo.com>,
"Kirill A . Shutemov" <kirill.shutemov@...ux.intel.com>, Thomas Garnier <thgarnie@...gle.com>,
Andrew Morton <akpm@...ux-foundation.org>, Alexei Starovoitov <ast@...nel.org>, Josef Bacik <jbacik@...com>,
Masami Hiramatsu <mhiramat@...nel.org>, Nicholas Piggin <npiggin@...il.com>,
Al Viro <viro@...iv.linux.org.uk>, "David S . Miller" <davem@...emloft.net>,
Ding Tianhong <dingtianhong@...wei.com>, David Woodhouse <dwmw@...zon.co.uk>,
Josh Poimboeuf <jpoimboe@...hat.com>, Steven Rostedt <rostedt@...dmis.org>,
Dominik Brodowski <linux@...inikbrodowski.net>, Juergen Gross <jgross@...e.com>,
Linus Torvalds <torvalds@...ux-foundation.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Dan Williams <dan.j.williams@...el.com>,
Dave Hansen <dave.hansen@...ux.intel.com>, Mathias Krause <minipli@...glemail.com>,
Vikas Shivappa <vikas.shivappa@...ux.intel.com>, Kyle Huey <me@...ehuey.com>,
Dmitry Safonov <dsafonov@...tuozzo.com>, Will Deacon <will.deacon@....com>,
Arnd Bergmann <arnd@...db.de>, Florian Weimer <fweimer@...hat.com>,
Boris Lukashev <blukashev@...pervictus.com>, Andrey Konovalov <andreyknvl@...gle.com>,
X86 ML <x86@...nel.org>, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v14 7/7] stackleak, sysctl: Allow runtime disabling of
kernel stack erasing
On Tue, Jul 24, 2018 at 4:41 PM, Alexander Popov <alex.popov@...ux.com> wrote:
> On 25.07.2018 01:56, Kees Cook wrote:
>> On Thu, Jul 19, 2018 at 4:31 AM, Alexander Popov <alex.popov@...ux.com> wrote:
>>> Introduce CONFIG_STACKLEAK_RUNTIME_DISABLE option, which provides
>>> 'stack_erasing_bypass' sysctl. It can be used in runtime to disable
>>> kernel stack erasing for kernels built with CONFIG_GCC_PLUGIN_STACKLEAK.
>>> Stack erasing will then remain disabled and STACKLEAK_METRICS will not
>>> be updated until the next boot.
>>>
>>> Signed-off-by: Alexander Popov <alex.popov@...ux.com>
>>> [...]
>>> +That erasing reduces the information which kernel stack leak bugs
>>> +can reveal and blocks some uninitialized stack variable attacks.
>>> +The tradeoff is the performance impact: on a single CPU system kernel
>>> +compilation sees a 1% slowdown, other systems and workloads may vary.
>>
>> I continue to have a hard time measuring even the 1% impact. Clearly I
>> need some better workloads. :)
>>
>>> [...]
>>> asmlinkage void stackleak_erase(void)
>>> {
>>> /* It would be nice not to have 'kstack_ptr' and 'boundary' on stack */
>>> @@ -22,6 +52,11 @@ asmlinkage void stackleak_erase(void)
>>> unsigned int poison_count = 0;
>>> const unsigned int depth = STACKLEAK_SEARCH_DEPTH / sizeof(unsigned long);
>>>
>>> +#ifdef CONFIG_STACKLEAK_RUNTIME_DISABLE
>>> + if (static_branch_unlikely(&stack_erasing_bypass))
>>> + return;
>>> +#endif
>>
>> I collapsed this into a macro (and took your other fix) and will push
>> this to my -next tree:
>>
>> +#define skip_erasing() static_branch_unlikely(&stack_erasing_bypass)
>> +#else
>> +#define skip_erasing() false
>> +#endif /* CONFIG_STACKLEAK_RUNTIME_DISABLE */
>> ...
>> + if (skip_erasing())
>> + return;
>> +
>
> That's nice! Thank you, I'll test it tomorrow.
>
>>> +
>>> /* Search for the poison value in the kernel stack */
>>> while (kstack_ptr > boundary && poison_count <= depth) {
>>> if (*(unsigned long *)kstack_ptr == STACKLEAK_POISON)
>>> @@ -78,6 +113,11 @@ void __used stackleak_track_stack(void)
>>> */
>>> unsigned long sp = (unsigned long)&sp;
>>>
>>> +#ifdef CONFIG_STACKLEAK_RUNTIME_DISABLE
>>> + if (static_branch_unlikely(&stack_erasing_bypass))
>>> + return;
>>> +#endif
>>
>> I would expect stackleak_erase() to be the expensive part, not the
>> tracking part? Shouldn't timings be unchanged by leaving this in
>> unconditionally, which would mean the sysctl could be re-enabled?
>
> Dropping the bypass in stackleak_track_stack() will not help against the
> troubles with re-enabling stack erasing (tracking and erasing depend on each
Isn't the tracking checking "sp < current->lowest_stack", so if
erasure was off, lowest_stack would only ever get further into the
stack? And when erasure was turned back on, it would start getting
reset correctly again. Or is the concern the poison searching could
break? It seems like it would still work right? I must be missing
something. :)
> other). Moreover, it will also make the STACKLEAK_METRICS show insane values. So
> I think we should have the bypass in both functions.
I left it as-is for now. It should appear in -next tomorrow.
Thanks!
-Kees
--
Kees Cook
Pixel Security
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
