From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thomas Garnier Subject: Re: [PATCH v2 1/4] syscalls: Restore address limit after a syscall Date: Thu, 9 Mar 2017 09:41:14 -0800 Message-ID: References: <20170309012456.5631-1-thgarnie@google.com> <20170309084208.ims3ehcnieo7uqew@pd.tnic> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Return-path: List-Post: List-Help: List-Unsubscribe: List-Subscribe: In-Reply-To: To: Andy Lutomirski Cc: Borislav Petkov , David Howells , Dave Hansen , Arnd Bergmann , Al Viro , =?UTF-8?Q?Ren=C3=A9_Nyffenegger?= , Andrew Morton , Kees Cook , "Paul E . McKenney" , "David S . Miller" , Andy Lutomirski , Ard Biesheuvel , Nicolas Pitre , Petr Mladek , Sebastian Andrzej Siewior , Sergey Senozhatsky , Helge Deller , Rik van Riel , Ingo Molnar , Oleg Nesterov , John Stultz , Thomas Gleixner , Pavel List-Id: linux-api@vger.kernel.org On Thu, Mar 9, 2017 at 9:27 AM, Andy Lutomirski wrote: > On Thu, Mar 9, 2017 at 12:42 AM, Borislav Petkov wrote: >> On Wed, Mar 08, 2017 at 05:24:53PM -0800, Thomas Garnier wrote: >>> @@ -191,6 +191,22 @@ extern struct trace_event_functions exit_syscall_print_funcs; >>> SYSCALL_METADATA(sname, x, __VA_ARGS__) \ >>> __SYSCALL_DEFINEx(x, sname, __VA_ARGS__) >>> >>> +asmlinkage void verify_pre_usermode_state(void); >>> + >>> +#ifndef CONFIG_ARCH_NO_SYSCALL_VERIFY_PRE_USERMODE_STATE >>> +static inline bool has_user_ds(void) { >>> + bool ret = segment_eq(get_fs(), USER_DS); >>> + // Prevent re-ordering the call >> >> This is not the kernel comments style. Use /* */ instead. >> >>> + barrier(); >>> + return ret; >>> +} >>> +#else >>> +static inline bool has_user_ds(void) { >>> + return false; >>> +} >>> +#endif >> >> ... and then you could slim down the ifdeffery a bit: >> >> static inline bool has_user_ds(void) { >> bool ret = false; >> >> #ifndef CONFIG_ARCH_NO_SYSCALL_VERIFY_PRE_USERMODE_STATE >> ret = segment_eq(get_fs(), USER_DS); >> /* Prevent re-ordering the call. */ >> barrier(); >> #endif >> >> return ret; >> } > > I don't like having any kernel configuration in which has_user_ds() > unconditionally returns false. Can we put the ifdeffery in the caller > instead? I don't like it either to be honest. We could have something like: #ifndef CONFIG_ARCH_NO_SYSCALL_VERIFY_PRE_USERMODE_STATE #define CHECK_USER_CALLER(_x) bool _x = segment_eq(get_fs(), USER_DS) #else #define CHECK_USER_CALLER(_x) bool _x = false #endif // In the syscall macro: CHECK_CALLED_BY_USER(user_caller); > > --Andy -- Thomas From mboxrd@z Thu Jan 1 00:00:00 1970 MIME-Version: 1.0 In-Reply-To: References: <20170309012456.5631-1-thgarnie@google.com> <20170309084208.ims3ehcnieo7uqew@pd.tnic> From: Thomas Garnier Date: Thu, 9 Mar 2017 09:41:14 -0800 Message-ID: Content-Type: text/plain; charset=UTF-8 Subject: [kernel-hardening] Re: [PATCH v2 1/4] syscalls: Restore address limit after a syscall To: Andy Lutomirski Cc: Borislav Petkov , David Howells , Dave Hansen , Arnd Bergmann , Al Viro , =?UTF-8?Q?Ren=C3=A9_Nyffenegger?= , Andrew Morton , Kees Cook , "Paul E . McKenney" , "David S . Miller" , Andy Lutomirski , Ard Biesheuvel , Nicolas Pitre , Petr Mladek , Sebastian Andrzej Siewior , Sergey Senozhatsky , Helge Deller , Rik van Riel , Ingo Molnar , Oleg Nesterov , John Stultz , Thomas Gleixner , Pavel Tikhomirov , Frederic Weisbecker , Stephen Smalley , Stanislav Kinsburskiy , Ingo Molnar , "H . Peter Anvin" , Paolo Bonzini , Josh Poimboeuf , Brian Gerst , Jan Beulich , Christian Borntraeger , "Luis R . Rodriguez" , He Chen , Russell King , Will Deacon , Catalin Marinas , Mark Rutland , James Morse , Pratyush Anand , Vladimir Murzin , Chris Metcalf , Andre Przywara , Linux API , "linux-kernel@vger.kernel.org" , X86 ML , "linux-arm-kernel@lists.infradead.org" , "kernel-hardening@lists.openwall.com" List-ID: On Thu, Mar 9, 2017 at 9:27 AM, Andy Lutomirski wrote: > On Thu, Mar 9, 2017 at 12:42 AM, Borislav Petkov wrote: >> On Wed, Mar 08, 2017 at 05:24:53PM -0800, Thomas Garnier wrote: >>> @@ -191,6 +191,22 @@ extern struct trace_event_functions exit_syscall_print_funcs; >>> SYSCALL_METADATA(sname, x, __VA_ARGS__) \ >>> __SYSCALL_DEFINEx(x, sname, __VA_ARGS__) >>> >>> +asmlinkage void verify_pre_usermode_state(void); >>> + >>> +#ifndef CONFIG_ARCH_NO_SYSCALL_VERIFY_PRE_USERMODE_STATE >>> +static inline bool has_user_ds(void) { >>> + bool ret = segment_eq(get_fs(), USER_DS); >>> + // Prevent re-ordering the call >> >> This is not the kernel comments style. Use /* */ instead. >> >>> + barrier(); >>> + return ret; >>> +} >>> +#else >>> +static inline bool has_user_ds(void) { >>> + return false; >>> +} >>> +#endif >> >> ... and then you could slim down the ifdeffery a bit: >> >> static inline bool has_user_ds(void) { >> bool ret = false; >> >> #ifndef CONFIG_ARCH_NO_SYSCALL_VERIFY_PRE_USERMODE_STATE >> ret = segment_eq(get_fs(), USER_DS); >> /* Prevent re-ordering the call. */ >> barrier(); >> #endif >> >> return ret; >> } > > I don't like having any kernel configuration in which has_user_ds() > unconditionally returns false. Can we put the ifdeffery in the caller > instead? I don't like it either to be honest. We could have something like: #ifndef CONFIG_ARCH_NO_SYSCALL_VERIFY_PRE_USERMODE_STATE #define CHECK_USER_CALLER(_x) bool _x = segment_eq(get_fs(), USER_DS) #else #define CHECK_USER_CALLER(_x) bool _x = false #endif // In the syscall macro: CHECK_CALLED_BY_USER(user_caller); > > --Andy -- Thomas