From: Alexander Popov <alex.popov@linux.com> To: Boris Lukashev <blukashev@sempervictus.com>, Dave Hansen <dave.hansen@linux.intel.com>, kernel-hardening@lists.openwall.com, Kees Cook <keescook@chromium.org>, PaX Team <pageexec@freemail.hu>, Brad Spengler <spender@grsecurity.net>, Ingo Molnar <mingo@kernel.org>, Andy Lutomirski <luto@kernel.org>, Tycho Andersen <tycho@tycho.ws>, Laura Abbott <labbott@redhat.com>, Mark Rutland <mark.rutland@arm.com>, Ard Biesheuvel <ard.biesheuvel@linaro.org>, Borislav Petkov <bp@alien8.de>, Richard Sandiford <richard.sandiford@arm.com>, Thomas Gleixner <tglx@linutronix.de>, "H . Peter Anvin" <hpa@zytor.com>, Peter Zijlstra <a.p.zijlstra@chello.nl>, "Dmitry V . Levin" <ldv@altlinux.org>, Emese Revfy <re.emese@gmail.com>, Jonathan Corbet <corbet@lwn.net>, Andrey Ryabinin <aryabinin@virtuozzo.com>, "Kirill A . Shutemov" <kirill.shutemov@linux.intel.com>, Thomas Garnier <thgarnie@google.com>, Andrew Morton <akpm@linux-foundation.org>, Alexei Starovoitov <ast@kernel.org>, Josef Bacik <jbacik@fb.com>, Masami Hiramatsu <mhiramat@kernel.org>, Nicholas Piggin <npiggin@gmail.com>, Al Viro <viro@zeniv.linux.org.uk>, "David S . Miller" <davem@davemloft.net>, Ding Tianhong <dingtianhong@huawei.com>, David Woodhouse <dwmw@amazon.co.uk>, Josh Poimboeuf <jpoimboe@redhat.com>, Steven Rostedt <rostedt@goodmis.org>, Dominik Brodowski <linux@dominikbrodowski.net>, Juergen Gross <jgross@suse.com>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Dan Williams <dan.j.williams@intel.com>, Mathias Krause <minipli@googlemail.com>, Vikas Shivappa <vikas.shivappa@linux.intel.com>, Kyle Huey <me@kylehuey.com>, Dmitry Safonov <dsafonov@virtuozzo.com>, Will Deacon <will.deacon@arm.com>, Arnd Bergmann <arnd@arndb.de>, Florian Weimer <fweimer@redhat.com>, Boris Lukashev <blukashev@sempervictus.com>, x86@kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH RFC v10 2/6] x86/entry: Add STACKLEAK erasing the kernel stack at the end of syscalls Date: Thu, 29 Mar 2018 23:56:35 +0300 [thread overview] Message-ID: <ccb76413-0740-0f20-89fc-7a1c9053b06c@linux.com> (raw) In-Reply-To: <CAFUG7Cd7cSqSHCrqxKUFwBBQLuix0Mi5-=V6pq_U7KtFh20Kqg@mail.gmail.com> Hello Dave and Boris, Thanks for your replies! On 29.03.2018 18:09, Boris Lukashev wrote: > On Thu, Mar 29, 2018 at 9:51 AM, Dave Hansen > <dave.hansen@linux.intel.com> wrote: >> On 03/28/2018 11:58 PM, Alexander Popov wrote: >>>> I noticed the 64-bit version saves/restores registers while >>>> the 32-bit version doesn't. What's the reasoning there? >>> When erase_kstack() is called from the trampoline stack, it must save and >>> restore any modified registers, since all registers except RDI are live >>> (prepared for the userspace). >>> >>> When erase_kstack() is called from the thread stack, it can clobber registers >>> according the function call convention without any harm. >> >> Oh, and since there's no 32-bit trampoline stack, we don't need it on >> 32-bit? >> >> If end up reposting this set again, Hope so. Let's see... >> could you add a few comments about >> this around the ERASE_KSTACK macro definitions, or perhaps the call >> sites? You might even want to call them ERASE_KSTACK_CLOBBER (for >> 32-bit) and ERASE_KSTACK_NOCLOBBER (for 64-bit) to make this more clear. > > Not sure if the macro name differentiation is such a good idea, might > entice improper use attempts. > A more detailed explanation of this should probably go into the > headers and doc/commit log for future implementation on architectures > which may have their own weird semantics around the trampoline > stack/not have one. Ok, I see. Let me give the overview and propose the solution. The current version has 3 separate ERASE_KSTACK definitions: 1. a simple one in entry_32.S, used only in that file: +.macro ERASE_KSTACK +#ifdef CONFIG_GCC_PLUGIN_STACKLEAK + call erase_kstack +#endif +.endm 2. another one saving registers in entry_64.S, used only in that file for erasing from the trampoline stack: +.macro ERASE_KSTACK +#ifdef CONFIG_GCC_PLUGIN_STACKLEAK + PUSH_AND_CLEAR_REGS + call erase_kstack + POP_REGS +#endif +.endm The call sights are already prepared and documented by Andy Lutomirski: /* * We are on the trampoline stack. All regs except RDI are live. * We can do future final exit work right here. */ + ERASE_KSTACK 3. a simple one in entry_64_compat.S (similar to case 1), used only in that file: + .macro ERASE_KSTACK +#ifdef CONFIG_GCC_PLUGIN_STACKLEAK + call erase_kstack +#endif + .endm + The call sight is documented as well: sysret32_from_system_call: + /* + * We are not going to return to the userspace from the trampoline + * stack. So let's erase the thread stack right now. + */ + ERASE_KSTACK If STACKLEAK is not banned, would you like me to introduce ERASE_KSTACK (for cases 1 and 3) and ERASE_KSTACK_NOCLOBBER (for case 2) in arch/x86/entry/calling.h? Best regards, Alexander
WARNING: multiple messages have this Message-ID (diff)
From: Alexander Popov <alex.popov@linux.com> To: Boris Lukashev <blukashev@sempervictus.com>, Dave Hansen <dave.hansen@linux.intel.com>, kernel-hardening@lists.openwall.com, Kees Cook <keescook@chromium.org>, PaX Team <pageexec@freemail.hu>, Brad Spengler <spender@grsecurity.net>, Ingo Molnar <mingo@kernel.org>, Andy Lutomirski <luto@kernel.org>, Tycho Andersen <tycho@tycho.ws>, Laura Abbott <labbott@redhat.com>, Mark Rutland <mark.rutland@arm.com>, Ard Biesheuvel <ard.biesheuvel@linaro.org>, Borislav Petkov <bp@alien8.de>, Richard Sandiford <richard.sandiford@arm.com>, Thomas Gleixner <tglx@linutronix.de>, "H . Peter Anvin" <hpa@zytor.com>, Peter Zijlstra <a.p.zijlstra@chello.nl>, "Dmitry V . Levin" <ldv@altlinux.org>, Emese Revfy <re.emese@gmail.com>, Jonathan Corbet <corbet@lwn.net>, Andrey Ryabinin <aryabinin@virtuozzo.com>, "Kirill A . Shutemov" <kirill.shutemov@linux.intel.com>, Thomas Garnier <thgarnie@google.com>, Andrew Morton <akpm@linux-foundation.org>, Alexei Starovoitov <ast@kernel.org>, Josef Bacik <jbacik@fb.com>, Masami Hiramatsu <mhiramat@kernel.org>, Nicholas Piggin <npiggin@gmail.com>, Al Viro <viro@zeniv.linux.org.uk>, "David S . Miller" <davem@davemloft.net>, Ding Tianhong <dingtianhong@huawei.com>, David Woodhouse <dwmw@amazon.co.uk>, Josh Poimboeuf <jpoimboe@redhat.com>, Steven Rostedt <rostedt@goodmis.org>, Dominik Brodowski <linux@dominikbrodowski.net>, Juergen Gross <jgross@suse.com>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Dan Williams <dan.j.williams@intel.com>, Mathias Krause <minipli@googlemail.com>, Vikas Shivappa <vikas.shivappa@linux.intel.com>, Kyle Huey <me@kylehuey.com>, Dmitry Safonov <dsafonov@virtuozzo.com>, Will Deacon <will.deacon@arm.com>, Arnd Bergmann <arnd@arndb.de>, Florian Weimer <fweimer@redhat.com>Boris Lukashev <blukashev@sempervictus.com>, x86@kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH RFC v10 2/6] x86/entry: Add STACKLEAK erasing the kernel stack at the end of syscalls Date: Thu, 29 Mar 2018 23:56:35 +0300 [thread overview] Message-ID: <ccb76413-0740-0f20-89fc-7a1c9053b06c@linux.com> (raw) In-Reply-To: <CAFUG7Cd7cSqSHCrqxKUFwBBQLuix0Mi5-=V6pq_U7KtFh20Kqg@mail.gmail.com> Hello Dave and Boris, Thanks for your replies! On 29.03.2018 18:09, Boris Lukashev wrote: > On Thu, Mar 29, 2018 at 9:51 AM, Dave Hansen > <dave.hansen@linux.intel.com> wrote: >> On 03/28/2018 11:58 PM, Alexander Popov wrote: >>>> I noticed the 64-bit version saves/restores registers while >>>> the 32-bit version doesn't. What's the reasoning there? >>> When erase_kstack() is called from the trampoline stack, it must save and >>> restore any modified registers, since all registers except RDI are live >>> (prepared for the userspace). >>> >>> When erase_kstack() is called from the thread stack, it can clobber registers >>> according the function call convention without any harm. >> >> Oh, and since there's no 32-bit trampoline stack, we don't need it on >> 32-bit? >> >> If end up reposting this set again, Hope so. Let's see... >> could you add a few comments about >> this around the ERASE_KSTACK macro definitions, or perhaps the call >> sites? You might even want to call them ERASE_KSTACK_CLOBBER (for >> 32-bit) and ERASE_KSTACK_NOCLOBBER (for 64-bit) to make this more clear. > > Not sure if the macro name differentiation is such a good idea, might > entice improper use attempts. > A more detailed explanation of this should probably go into the > headers and doc/commit log for future implementation on architectures > which may have their own weird semantics around the trampoline > stack/not have one. Ok, I see. Let me give the overview and propose the solution. The current version has 3 separate ERASE_KSTACK definitions: 1. a simple one in entry_32.S, used only in that file: +.macro ERASE_KSTACK +#ifdef CONFIG_GCC_PLUGIN_STACKLEAK + call erase_kstack +#endif +.endm 2. another one saving registers in entry_64.S, used only in that file for erasing from the trampoline stack: +.macro ERASE_KSTACK +#ifdef CONFIG_GCC_PLUGIN_STACKLEAK + PUSH_AND_CLEAR_REGS + call erase_kstack + POP_REGS +#endif +.endm The call sights are already prepared and documented by Andy Lutomirski: /* * We are on the trampoline stack. All regs except RDI are live. * We can do future final exit work right here. */ + ERASE_KSTACK 3. a simple one in entry_64_compat.S (similar to case 1), used only in that file: + .macro ERASE_KSTACK +#ifdef CONFIG_GCC_PLUGIN_STACKLEAK + call erase_kstack +#endif + .endm + The call sight is documented as well: sysret32_from_system_call: + /* + * We are not going to return to the userspace from the trampoline + * stack. So let's erase the thread stack right now. + */ + ERASE_KSTACK If STACKLEAK is not banned, would you like me to introduce ERASE_KSTACK (for cases 1 and 3) and ERASE_KSTACK_NOCLOBBER (for case 2) in arch/x86/entry/calling.h? Best regards, Alexander
next prev parent reply other threads:[~2018-03-29 20:56 UTC|newest] Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-03-28 19:57 [PATCH RFC v10 0/6] Introduce the STACKLEAK feature and a test for it Alexander Popov 2018-03-28 19:57 ` [PATCH RFC v10 1/6] gcc-plugins: Clean up the cgraph_create_edge* macros Alexander Popov 2018-03-28 19:57 ` [PATCH RFC v10 2/6] x86/entry: Add STACKLEAK erasing the kernel stack at the end of syscalls Alexander Popov 2018-03-28 22:55 ` Dave Hansen 2018-03-29 6:58 ` Alexander Popov 2018-03-29 13:51 ` Dave Hansen [not found] ` <CAFUG7Cd7cSqSHCrqxKUFwBBQLuix0Mi5-=V6pq_U7KtFh20Kqg@mail.gmail.com> 2018-03-29 20:56 ` Alexander Popov [this message] 2018-03-29 20:56 ` Alexander Popov 2018-03-29 21:00 ` Dave Hansen [not found] ` <alpine.DEB.2.10.1803291137150.27913@vshiva-Udesk> 2018-03-29 21:34 ` Alexander Popov 2018-03-29 21:34 ` Alexander Popov 2018-03-28 19:57 ` [PATCH RFC v10 3/6] gcc-plugins: Add STACKLEAK plugin for tracking the kernel stack Alexander Popov 2018-03-28 19:57 ` [PATCH RFC v10 4/6] lkdtm: Add a test for STACKLEAK Alexander Popov 2018-03-28 19:57 ` [PATCH RFC v10 5/6] fs/proc: Show STACKLEAK metrics in the /proc file system Alexander Popov 2018-03-28 19:57 ` [PATCH RFC v10 6/6] doc: self-protection: Add information about STACKLEAK feature Alexander Popov 2018-03-28 20:13 ` [PATCH RFC v10 0/6] Introduce the STACKLEAK feature and a test for it Kees Cook
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=ccb76413-0740-0f20-89fc-7a1c9053b06c@linux.com \ --to=alex.popov@linux.com \ --cc=a.p.zijlstra@chello.nl \ --cc=akpm@linux-foundation.org \ --cc=ard.biesheuvel@linaro.org \ --cc=arnd@arndb.de \ --cc=aryabinin@virtuozzo.com \ --cc=ast@kernel.org \ --cc=blukashev@sempervictus.com \ --cc=bp@alien8.de \ --cc=corbet@lwn.net \ --cc=dan.j.williams@intel.com \ --cc=dave.hansen@linux.intel.com \ --cc=davem@davemloft.net \ --cc=dingtianhong@huawei.com \ --cc=dsafonov@virtuozzo.com \ --cc=dwmw@amazon.co.uk \ --cc=fweimer@redhat.com \ --cc=gregkh@linuxfoundation.org \ --cc=hpa@zytor.com \ --cc=jbacik@fb.com \ --cc=jgross@suse.com \ --cc=jpoimboe@redhat.com \ --cc=keescook@chromium.org \ --cc=kernel-hardening@lists.openwall.com \ --cc=kirill.shutemov@linux.intel.com \ --cc=labbott@redhat.com \ --cc=ldv@altlinux.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux@dominikbrodowski.net \ --cc=luto@kernel.org \ --cc=mark.rutland@arm.com \ --cc=me@kylehuey.com \ --cc=mhiramat@kernel.org \ --cc=mingo@kernel.org \ --cc=minipli@googlemail.com \ --cc=npiggin@gmail.com \ --cc=pageexec@freemail.hu \ --cc=re.emese@gmail.com \ --cc=richard.sandiford@arm.com \ --cc=rostedt@goodmis.org \ --cc=spender@grsecurity.net \ --cc=tglx@linutronix.de \ --cc=thgarnie@google.com \ --cc=tycho@tycho.ws \ --cc=vikas.shivappa@linux.intel.com \ --cc=viro@zeniv.linux.org.uk \ --cc=will.deacon@arm.com \ --cc=x86@kernel.org \ /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.