Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754280AbdCMVx3 convert rfc822-to-8bit (ORCPT ); Mon, 13 Mar 2017 17:53:29 -0400 Received: from terminus.zytor.com ([65.50.211.136]:52342 "EHLO mail.zytor.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753920AbdCMVxI (ORCPT ); Mon, 13 Mar 2017 17:53:08 -0400 From: "H. Peter Anvin" Message-Id: <201703132148.v2DLmNa7028340@mail.zytor.com> Date: Mon, 13 Mar 2017 14:48:15 -0700 User-Agent: K-9 Mail for Android In-Reply-To: <20170311094200.GA27700@gmail.com> References: <20170311000501.46607-1-thgarnie@google.com> <20170311000501.46607-2-thgarnie@google.com> <20170311094200.GA27700@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8BIT Subject: Re: [PATCH v3 2/4] x86/syscalls: Specific usage of verify_pre_usermode_state To: Ingo Molnar , Thomas Garnier CC: Martin Schwidefsky , Heiko Carstens , David Howells , Arnd Bergmann , Al Viro , Dave Hansen , =?ISO-8859-1?Q?Ren=E9_Nyffenegger?= , Andrew Morton , Kees Cook , "Paul E . McKenney" , Andy Lutomirski , Ard Biesheuvel , Nicolas Pitre , Petr Mladek , Sebastian Andrzej Siewior , Sergey Senozhatsky , Helge Deller , Rik van Riel , John Stultz , Thomas Gleixner , Oleg Nesterov , Stephen Smalley , Pavel Tikhomirov , Frederic Weisbecker , Stanislav.Kinsburskiy@zytor.com Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4359 Lines: 102 ,Ingo Molnar ,Paolo Bonzini ,Dmitry Safonov ,Borislav Petkov ,Josh Poimboeuf ,Brian Gerst ,Jan Beulich ,Christian Borntraeger ,Fenghua Yu ,He Chen ,Russell King ,Vladimir Murzin ,Will Deacon ,Catalin Marinas ,Mark Rutland ,James Morse ,"David A . Long" ,Pratyush Anand ,Laura Abbott ,Andre Przywara ,Chris Metcalf ,linux-s390@vger.kernel.org,linux-kernel@vger.kernel.org,linux-api@vger.kernel.org,x86@kernel.org,linux-arm-kernel@lists.infradead.org,kernel-hardening@lists.openwall.com From: hpa@zytor.com Message-ID: On March 11, 2017 1:42:00 AM PST, Ingo Molnar wrote: > >* Thomas Garnier wrote: > >> Implement specific usage of verify_pre_usermode_state for user-mode >> returns for x86. >> --- >> Based on next-20170308 >> --- >> arch/x86/Kconfig | 1 + >> arch/x86/entry/common.c | 3 +++ >> arch/x86/entry/entry_64.S | 19 +++++++++++++++++++ >> arch/x86/include/asm/pgtable_64_types.h | 11 +++++++++++ >> arch/x86/include/asm/processor.h | 11 ----------- >> 5 files changed, 34 insertions(+), 11 deletions(-) >> >> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig >> index 005df7c825f5..6d48e18e6f09 100644 >> --- a/arch/x86/Kconfig >> +++ b/arch/x86/Kconfig >> @@ -63,6 +63,7 @@ config X86 >> select ARCH_MIGHT_HAVE_ACPI_PDC if ACPI >> select ARCH_MIGHT_HAVE_PC_PARPORT >> select ARCH_MIGHT_HAVE_PC_SERIO >> + select ARCH_NO_SYSCALL_VERIFY_PRE_USERMODE_STATE >> select ARCH_SUPPORTS_ATOMIC_RMW >> select ARCH_SUPPORTS_DEFERRED_STRUCT_PAGE_INIT >> select ARCH_SUPPORTS_NUMA_BALANCING if X86_64 >> diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c >> index 370c42c7f046..525edbb77f03 100644 >> --- a/arch/x86/entry/common.c >> +++ b/arch/x86/entry/common.c >> @@ -22,6 +22,7 @@ >> #include >> #include >> #include >> +#include >> >> #include >> #include >> @@ -180,6 +181,8 @@ __visible inline void >prepare_exit_to_usermode(struct pt_regs *regs) >> struct thread_info *ti = current_thread_info(); >> u32 cached_flags; >> >> + verify_pre_usermode_state(); >> + >> if (IS_ENABLED(CONFIG_PROVE_LOCKING) && WARN_ON(!irqs_disabled())) >> local_irq_disable(); >> >> diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S >> index d2b2a2948ffe..04db589be466 100644 >> --- a/arch/x86/entry/entry_64.S >> +++ b/arch/x86/entry/entry_64.S >> @@ -218,6 +218,25 @@ entry_SYSCALL_64_fastpath: >> testl $_TIF_ALLWORK_MASK, TASK_TI_flags(%r11) >> jnz 1f >> >> + /* >> + * Check user-mode state on fast path return, the same check is >done >> + * under the slow path through syscall_return_slowpath. >> + */ >> +#ifdef CONFIG_BUG_ON_DATA_CORRUPTION >> + call verify_pre_usermode_state >> +#else >> + /* >> + * Similar to set_fs(USER_DS) in verify_pre_usermode_state without >a >> + * warning. >> + */ >> + movq PER_CPU_VAR(current_task), %rax >> + movq $TASK_SIZE_MAX, %rcx >> + cmp %rcx, TASK_addr_limit(%rax) >> + jz 1f >> + movq %rcx, TASK_addr_limit(%rax) >> +1: >> +#endif >> + >> LOCKDEP_SYS_EXIT >> TRACE_IRQS_ON /* user mode is traced as IRQs on */ >> movq RIP(%rsp), %rcx > >Ugh, so you call an assembly function just to ... call another >function. > >Plus why is it in assembly to begin with? Is this some older code that >got >written when the x86 entry code was in assembly, and never properly >converted to C? > >Thanks, > > Ingo The code does a compare to jump around a store. It would be much cleaner and faster to simply clobber the value unconditionally. If there is a test it should be to avoid the function call, not (only) the assignment. -- Sent from my Android device with K-9 Mail. Please excuse my brevity.