linux-kernel - Re: [tip:x86/asm] x86/entry: Add enter_from_user

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] [thread-next>] [day] [month] [year] [list]
Message-ID: <CALCETrUGCsRWJrkMX0pAq_twfGbcsNrAZBMcF8n_eOXGt0c_ew@mail.gmail.com>
Date:	Tue, 14 Jul 2015 16:04:47 -0700
From:	Andy Lutomirski <luto@...capital.net>
To:	Frederic Weisbecker <fweisbec@...il.com>
Cc:	Brian Gerst <brgerst@...il.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Ingo Molnar <mingo@...nel.org>,
	Denys Vlasenko <vda.linux@...glemail.com>,
	Andrew Lutomirski <luto@...nel.org>,
	"H. Peter Anvin" <hpa@...or.com>,
	Denys Vlasenko <dvlasenk@...hat.com>,
	Rik van Riel <riel@...hat.com>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Borislav Petkov <bp@...en8.de>,
	Thomas Gleixner <tglx@...utronix.de>,
	Oleg Nesterov <oleg@...hat.com>,
	Kees Cook <keescook@...omium.org>,
	"linux-tip-commits@...r.kernel.org" 
	<linux-tip-commits@...r.kernel.org>
Subject: Re: [tip:x86/asm] x86/entry: Add enter_from_user_mode() and use it in syscalls

On Tue, Jul 14, 2015 at 4:00 PM, Frederic Weisbecker <fweisbec@...il.com> wrote:
> On Tue, Jul 07, 2015 at 03:51:29AM -0700, tip-bot for Andy Lutomirski wrote:
>> Commit-ID:  feed36cde0a10adb957445a37e48f957f30b2273
>> Gitweb:     http://git.kernel.org/tip/feed36cde0a10adb957445a37e48f957f30b2273
>> Author:     Andy Lutomirski <luto@...nel.org>
>> AuthorDate: Fri, 3 Jul 2015 12:44:25 -0700
>> Committer:  Ingo Molnar <mingo@...nel.org>
>> CommitDate: Tue, 7 Jul 2015 10:59:06 +0200
>>
>> x86/entry: Add enter_from_user_mode() and use it in syscalls
>>
>> Changing the x86 context tracking hooks is dangerous because
>> there are no good checks that we track our context correctly.
>> Add a helper to check that we're actually in CONTEXT_USER when
>> we enter from user mode and wire it up for syscall entries.
>>
>> Subsequent patches will wire this up for all non-NMI entries as
>> well.  NMIs are their own special beast and cannot currently
>> switch overall context tracking state.  Instead, they have their
>> own special RCU hooks.
>>
>> This is a tiny speedup if !CONFIG_CONTEXT_TRACKING (removes a
>> branch) and a tiny slowdown if CONFIG_CONTEXT_TRACING (adds a
>> layer of indirection).  Eventually, we should fix up the core
>> context tracking code to supply a function that does what we
>> want (and can be much simpler than user_exit), which will enable
>> us to get rid of the extra call.
>>
>> Signed-off-by: Andy Lutomirski <luto@...nel.org>
>> Cc: Andy Lutomirski <luto@...capital.net>
>> Cc: Borislav Petkov <bp@...en8.de>
>> Cc: Brian Gerst <brgerst@...il.com>
>> Cc: Denys Vlasenko <dvlasenk@...hat.com>
>> Cc: Denys Vlasenko <vda.linux@...glemail.com>
>> Cc: Frederic Weisbecker <fweisbec@...il.com>
>> Cc: H. Peter Anvin <hpa@...or.com>
>> Cc: Kees Cook <keescook@...omium.org>
>> Cc: Linus Torvalds <torvalds@...ux-foundation.org>
>> Cc: Oleg Nesterov <oleg@...hat.com>
>> Cc: Peter Zijlstra <peterz@...radead.org>
>> Cc: Rik van Riel <riel@...hat.com>
>> Cc: Thomas Gleixner <tglx@...utronix.de>
>> Cc: paulmck@...ux.vnet.ibm.com
>> Link: http://lkml.kernel.org/r/853b42420066ec3fb856779cdc223a6dcb5d355b.1435952415.git.luto@kernel.org
>> Signed-off-by: Ingo Molnar <mingo@...nel.org>
>> ---
>>  arch/x86/entry/common.c | 13 ++++++++++++-
>>  1 file changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c
>> index 917d0c3..9a327ee 100644
>> --- a/arch/x86/entry/common.c
>> +++ b/arch/x86/entry/common.c
>> @@ -28,6 +28,15 @@
>>  #define CREATE_TRACE_POINTS
>>  #include <trace/events/syscalls.h>
>>
>> +#ifdef CONFIG_CONTEXT_TRACKING
>> +/* Called on entry from user mode with IRQs off. */
>> +__visible void enter_from_user_mode(void)
>> +{
>> +     CT_WARN_ON(ct_state() != CONTEXT_USER);
>> +     user_exit();
>> +}
>> +#endif
>> +
>>  static void do_audit_syscall_entry(struct pt_regs *regs, u32 arch)
>>  {
>>  #ifdef CONFIG_X86_64
>> @@ -65,14 +74,16 @@ unsigned long syscall_trace_enter_phase1(struct pt_regs *regs, u32 arch)
>>       work = ACCESS_ONCE(current_thread_info()->flags) &
>>               _TIF_WORK_SYSCALL_ENTRY;
>>
>> +#ifdef CONFIG_CONTEXT_TRACKING
>>       /*
>>        * If TIF_NOHZ is set, we are required to call user_exit() before
>>        * doing anything that could touch RCU.
>>        */
>>       if (work & _TIF_NOHZ) {
>> -             user_exit();
>> +             enter_from_user_mode();
>>               work &= ~_TIF_NOHZ;
>
> We should move the sanity check to user_exit/enter() and use user_exit/enter()
> only when we actually enter/exit user.

I agree, but I don't know what other arches to.

> Here it's the case but syscall_trace_leave()
> and do_notify_resume() are special case that should probably use exception_enter/exit()
> unless your patchset have changed things such that there is only one call to user_exit()
> once we completed everything before resuming userspace. I need to review the rest of
> the patchset to discover that :-)

syscall_trace_leave and do_notify_resume may be so screwed up that
your suggestion wouldn't even work.  However, the next set of patches
(out for review but currently stalled pending Brian Gerst's vm86 work)
remove those functions entirely.

--Andy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/