* [PATCH v1 1/4] syscalls: Restore address limit after a syscall
@ 2017-03-08 21:38 Thomas Garnier
2017-03-08 21:38 ` [PATCH v1 2/4] x86/syscalls: Specific usage of verify_pre_usermode_state Thomas Garnier
` (4 more replies)
0 siblings, 5 replies; 12+ messages in thread
From: Thomas Garnier @ 2017-03-08 21:38 UTC (permalink / raw)
To: Arnd Bergmann, David Howells, Al Viro, Dave Hansen,
Thomas Garnier, René Nyffenegger, Andrew Morton, Kees Cook,
Paul E . McKenney, Petr Mladek, Andy Lutomirski, Ard Biesheuvel,
Nicolas Pitre, Sebastian Andrzej Siewior, Sergey Senozhatsky,
Helge Deller, Rik van Riel, Ingo Molnar, John Stultz,
Thomas Gleixner, Oleg Nesterov, Stanislav Kinsburskiy
Cc: linux-api, linux-kernel, x86, linux-arm-kernel, kernel-hardening
This patch prevents a syscall to modify the address limit of the
caller. The address limit is kept by the syscall wrapper and restored
just after the syscall ends.
For example, it would mitigation this bug:
- https://bugs.chromium.org/p/project-zero/issues/detail?id=990
By default, this change warns if the segment is incorrect while
returning to user-mode and fix it. The
CONFIG_VERIFY_PRE_USERMODE_STATE_BUG option can be enabled to halt
instead if needed.
The CONFIG_ARCH_NO_SYSCALL_VERIFY_PRE_USERMODE_STATE option is also
added so each architecture can optimize how the
verify_pre_usermode_state function is called.
Signed-off-by: Thomas Garnier <thgarnie@google.com>
---
Based on next-20170308
---
include/linux/syscalls.h | 19 +++++++++++++++++++
init/Kconfig | 16 ++++++++++++++++
kernel/sys.c | 11 +++++++++++
3 files changed, 46 insertions(+)
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index 980c3c9b06f8..78a2268ecd6e 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -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
+ barrier();
+ return ret;
+}
+#else
+static inline bool has_user_ds(void) {
+ return false;
+}
+#endif
+
+
#define __PROTECT(...) asmlinkage_protect(__VA_ARGS__)
#define __SYSCALL_DEFINEx(x, name, ...) \
asmlinkage long sys##name(__MAP(x,__SC_DECL,__VA_ARGS__)) \
@@ -199,7 +215,10 @@ extern struct trace_event_functions exit_syscall_print_funcs;
asmlinkage long SyS##name(__MAP(x,__SC_LONG,__VA_ARGS__)); \
asmlinkage long SyS##name(__MAP(x,__SC_LONG,__VA_ARGS__)) \
{ \
+ bool user_caller = has_user_ds(); \
long ret = SYSC##name(__MAP(x,__SC_CAST,__VA_ARGS__)); \
+ if (user_caller) \
+ verify_pre_usermode_state(); \
__MAP(x,__SC_TEST,__VA_ARGS__); \
__PROTECT(x, ret,__MAP(x,__SC_ARGS,__VA_ARGS__)); \
return ret; \
diff --git a/init/Kconfig b/init/Kconfig
index c859c993c26f..ab958b59063f 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1929,6 +1929,22 @@ config PROFILING
config TRACEPOINTS
bool
+#
+# Set by each architecture that want to optimize how verify_pre_usermode_state
+# is called.
+#
+config ARCH_NO_SYSCALL_VERIFY_PRE_USERMODE_STATE
+ bool
+
+config VERIFY_PRE_USERMODE_STATE_BUG
+ bool "Halt on incorrect state on returning to user-mode"
+ default n
+ help
+ By default a warning is logged and the state is fixed. This option
+ crashes the kernel instead.
+
+ If unsure, say Y.
+
source "arch/Kconfig"
endmenu # General setup
diff --git a/kernel/sys.c b/kernel/sys.c
index 196c7134bee6..cc2ebf7fae55 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -2459,3 +2459,14 @@ COMPAT_SYSCALL_DEFINE1(sysinfo, struct compat_sysinfo __user *, info)
return 0;
}
#endif /* CONFIG_COMPAT */
+
+/* Called before coming back to user-mode */
+asmlinkage void verify_pre_usermode_state(void)
+{
+#ifdef CONFIG_VERIFY_PRE_USERMODE_STATE_BUG
+ BUG_ON(!segment_eq(get_fs(), USER_DS));
+#else
+ if (WARN_ON(!segment_eq(get_fs(), USER_DS)))
+ set_fs(USER_DS);
+#endif
+}
--
2.12.0.246.ga2ecc84866-goog
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v1 2/4] x86/syscalls: Specific usage of verify_pre_usermode_state
2017-03-08 21:38 [PATCH v1 1/4] syscalls: Restore address limit after a syscall Thomas Garnier
@ 2017-03-08 21:38 ` Thomas Garnier
2017-03-08 21:38 ` [PATCH v1 3/4] arm/syscalls: " Thomas Garnier
` (3 subsequent siblings)
4 siblings, 0 replies; 12+ messages in thread
From: Thomas Garnier @ 2017-03-08 21:38 UTC (permalink / raw)
To: Arnd Bergmann, David Howells, Al Viro, Dave Hansen,
Thomas Garnier, René Nyffenegger, Andrew Morton, Kees Cook,
Paul E . McKenney, Petr Mladek, Andy Lutomirski, Ard Biesheuvel,
Nicolas Pitre, Sebastian Andrzej Siewior, Sergey Senozhatsky,
Helge Deller, Rik van Riel, Ingo Molnar, John Stultz,
Thomas Gleixner, Oleg Nesterov, Stanislav Kinsburskiy
Cc: linux-api, linux-kernel, x86, linux-arm-kernel, kernel-hardening
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 | 6 ++++++
3 files changed, 10 insertions(+)
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 <linux/context_tracking.h>
#include <linux/user-return-notifier.h>
#include <linux/uprobes.h>
+#include <linux/syscalls.h>
#include <asm/desc.h>
#include <asm/traps.h>
@@ -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..795c31eebd09 100644
--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -201,6 +201,7 @@ entry_SYSCALL_64_fastpath:
* It might end up jumping to the slow path. If it jumps, RAX
* and all argument registers are clobbered.
*/
+
call *sys_call_table(, %rax, 8)
.Lentry_SYSCALL_64_after_fastpath_call:
@@ -218,6 +219,11 @@ 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.
+ */
+ call verify_pre_usermode_state
LOCKDEP_SYS_EXIT
TRACE_IRQS_ON /* user mode is traced as IRQs on */
movq RIP(%rsp), %rcx
--
2.12.0.246.ga2ecc84866-goog
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v1 3/4] arm/syscalls: Specific usage of verify_pre_usermode_state
2017-03-08 21:38 [PATCH v1 1/4] syscalls: Restore address limit after a syscall Thomas Garnier
2017-03-08 21:38 ` [PATCH v1 2/4] x86/syscalls: Specific usage of verify_pre_usermode_state Thomas Garnier
@ 2017-03-08 21:38 ` Thomas Garnier
2017-03-08 21:49 ` Russell King - ARM Linux
2017-03-08 22:05 ` Nicolas Pitre
2017-03-08 21:38 ` [PATCH v1 4/4] arm64/syscalls: " Thomas Garnier
` (2 subsequent siblings)
4 siblings, 2 replies; 12+ messages in thread
From: Thomas Garnier @ 2017-03-08 21:38 UTC (permalink / raw)
To: Arnd Bergmann, David Howells, Al Viro, Dave Hansen,
Thomas Garnier, René Nyffenegger, Andrew Morton, Kees Cook,
Paul E . McKenney, Petr Mladek, Andy Lutomirski, Ard Biesheuvel,
Nicolas Pitre, Sebastian Andrzej Siewior, Sergey Senozhatsky,
Helge Deller, Rik van Riel, Ingo Molnar, John Stultz,
Thomas Gleixner, Oleg Nesterov, Stanislav Kinsburskiy
Cc: linux-api, linux-kernel, x86, linux-arm-kernel, kernel-hardening
Implement specific usage of verify_pre_usermode_state for user-mode
returns for arm.
---
Based on next-20170308
---
arch/arm/Kconfig | 1 +
arch/arm/kernel/entry-common.S | 5 +++++
2 files changed, 6 insertions(+)
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 0d4e71b42c77..704fd8f197fa 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -12,6 +12,7 @@ config ARM
select ARCH_HAVE_CUSTOM_GPIO_H
select ARCH_HAS_GCOV_PROFILE_ALL
select ARCH_MIGHT_HAVE_PC_PARPORT
+ select ARCH_NO_SYSCALL_VERIFY_PRE_USERMODE_STATE
select ARCH_OPTIONAL_KERNEL_RWX if ARCH_HAS_STRICT_KERNEL_RWX
select ARCH_OPTIONAL_KERNEL_RWX_DEFAULT if CPU_V7
select ARCH_SUPPORTS_ATOMIC_RMW
diff --git a/arch/arm/kernel/entry-common.S b/arch/arm/kernel/entry-common.S
index eb5cd77bf1d8..80cfdc7fabde 100644
--- a/arch/arm/kernel/entry-common.S
+++ b/arch/arm/kernel/entry-common.S
@@ -39,6 +39,9 @@
ret_fast_syscall:
UNWIND(.fnstart )
UNWIND(.cantunwind )
+ push {r0} @ save returned r0
+ bl verify_pre_usermode_state
+ pop {r0} @ restore r0
disable_irq_notrace @ disable interrupts
ldr r1, [tsk, #TI_FLAGS] @ re-check for syscall tracing
tst r1, #_TIF_SYSCALL_WORK | _TIF_WORK_MASK
@@ -65,6 +68,7 @@ ret_fast_syscall:
UNWIND(.fnstart )
UNWIND(.cantunwind )
str r0, [sp, #S_R0 + S_OFF]! @ save returned r0
+ bl verify_pre_usermode_state
disable_irq_notrace @ disable interrupts
ldr r1, [tsk, #TI_FLAGS] @ re-check for syscall tracing
tst r1, #_TIF_SYSCALL_WORK | _TIF_WORK_MASK
@@ -263,6 +267,7 @@ __sys_trace:
__sys_trace_return:
str r0, [sp, #S_R0 + S_OFF]! @ save returned r0
+ bl verify_pre_usermode_state
mov r0, sp
bl syscall_trace_exit
b ret_slow_syscall
--
2.12.0.246.ga2ecc84866-goog
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v1 4/4] arm64/syscalls: Specific usage of verify_pre_usermode_state
2017-03-08 21:38 [PATCH v1 1/4] syscalls: Restore address limit after a syscall Thomas Garnier
2017-03-08 21:38 ` [PATCH v1 2/4] x86/syscalls: Specific usage of verify_pre_usermode_state Thomas Garnier
2017-03-08 21:38 ` [PATCH v1 3/4] arm/syscalls: " Thomas Garnier
@ 2017-03-08 21:38 ` Thomas Garnier
2017-03-08 21:57 ` [PATCH v1 1/4] syscalls: Restore address limit after a syscall Kees Cook
2017-03-08 21:58 ` Russell King - ARM Linux
4 siblings, 0 replies; 12+ messages in thread
From: Thomas Garnier @ 2017-03-08 21:38 UTC (permalink / raw)
To: Arnd Bergmann, David Howells, Al Viro, Dave Hansen,
Thomas Garnier, René Nyffenegger, Andrew Morton, Kees Cook,
Paul E . McKenney, Petr Mladek, Andy Lutomirski, Ard Biesheuvel,
Nicolas Pitre, Sebastian Andrzej Siewior, Sergey Senozhatsky,
Helge Deller, Rik van Riel, Ingo Molnar, John Stultz,
Thomas Gleixner, Oleg Nesterov, Stanislav Kinsburskiy
Cc: linux-api, linux-kernel, x86, linux-arm-kernel, kernel-hardening
Implement specific usage of verify_pre_usermode_state for user-mode
returns for arm64.
---
Based on next-20170308
---
arch/arm64/Kconfig | 1 +
arch/arm64/kernel/entry.S | 2 ++
2 files changed, 3 insertions(+)
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 896eba61e5ed..da54774838d8 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -24,6 +24,7 @@ config ARM64
select ARCH_WANT_COMPAT_IPC_PARSE_VERSION
select ARCH_WANT_FRAME_POINTERS
select ARCH_HAS_UBSAN_SANITIZE_ALL
+ select ARCH_NO_SYSCALL_VERIFY_PRE_USERMODE_STATE
select ARM_AMBA
select ARM_ARCH_TIMER
select ARM_GIC
diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index 43512d4d7df2..b3d1d9e5c7c1 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -744,6 +744,7 @@ ENDPROC(cpu_switch_to)
ret_fast_syscall:
disable_irq // disable interrupts
str x0, [sp, #S_X0] // returned x0
+ bl verify_pre_usermode_state
ldr x1, [tsk, #TSK_TI_FLAGS] // re-check for syscall tracing
and x2, x1, #_TIF_SYSCALL_WORK
cbnz x2, ret_fast_syscall_trace
@@ -771,6 +772,7 @@ work_pending:
*/
ret_to_user:
disable_irq // disable interrupts
+ bl verify_pre_usermode_state
ldr x1, [tsk, #TSK_TI_FLAGS]
and x2, x1, #_TIF_WORK_MASK
cbnz x2, work_pending
--
2.12.0.246.ga2ecc84866-goog
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v1 3/4] arm/syscalls: Specific usage of verify_pre_usermode_state
2017-03-08 21:38 ` [PATCH v1 3/4] arm/syscalls: " Thomas Garnier
@ 2017-03-08 21:49 ` Russell King - ARM Linux
2017-03-08 22:05 ` Nicolas Pitre
1 sibling, 0 replies; 12+ messages in thread
From: Russell King - ARM Linux @ 2017-03-08 21:49 UTC (permalink / raw)
To: Thomas Garnier
Cc: Arnd Bergmann, David Howells, Al Viro, Dave Hansen,
René Nyffenegger, Andrew Morton, Kees Cook,
Paul E . McKenney, Petr Mladek, Andy Lutomirski, Ard Biesheuvel,
Nicolas Pitre, Sebastian Andrzej Siewior, Sergey Senozhatsky,
Helge Deller, Rik van Riel, Ingo Molnar, John Stultz,
Thomas Gleixner
On Wed, Mar 08, 2017 at 01:38:43PM -0800, Thomas Garnier wrote:
> Implement specific usage of verify_pre_usermode_state for user-mode
> returns for arm.
> ---
> Based on next-20170308
> ---
> arch/arm/Kconfig | 1 +
> arch/arm/kernel/entry-common.S | 5 +++++
> 2 files changed, 6 insertions(+)
>
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 0d4e71b42c77..704fd8f197fa 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -12,6 +12,7 @@ config ARM
> select ARCH_HAVE_CUSTOM_GPIO_H
> select ARCH_HAS_GCOV_PROFILE_ALL
> select ARCH_MIGHT_HAVE_PC_PARPORT
> + select ARCH_NO_SYSCALL_VERIFY_PRE_USERMODE_STATE
> select ARCH_OPTIONAL_KERNEL_RWX if ARCH_HAS_STRICT_KERNEL_RWX
> select ARCH_OPTIONAL_KERNEL_RWX_DEFAULT if CPU_V7
> select ARCH_SUPPORTS_ATOMIC_RMW
> diff --git a/arch/arm/kernel/entry-common.S b/arch/arm/kernel/entry-common.S
> index eb5cd77bf1d8..80cfdc7fabde 100644
> --- a/arch/arm/kernel/entry-common.S
> +++ b/arch/arm/kernel/entry-common.S
> @@ -39,6 +39,9 @@
> ret_fast_syscall:
> UNWIND(.fnstart )
> UNWIND(.cantunwind )
> + push {r0} @ save returned r0
> + bl verify_pre_usermode_state
> + pop {r0} @ restore r0
This really makes the fast exit utterly pointless, and we might as well
rip all that out.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v1 1/4] syscalls: Restore address limit after a syscall
2017-03-08 21:38 [PATCH v1 1/4] syscalls: Restore address limit after a syscall Thomas Garnier
` (2 preceding siblings ...)
2017-03-08 21:38 ` [PATCH v1 4/4] arm64/syscalls: " Thomas Garnier
@ 2017-03-08 21:57 ` Kees Cook
2017-03-09 1:13 ` Thomas Garnier
2017-03-08 21:58 ` Russell King - ARM Linux
4 siblings, 1 reply; 12+ messages in thread
From: Kees Cook @ 2017-03-08 21:57 UTC (permalink / raw)
To: Thomas Garnier
Cc: Arnd Bergmann, David Howells, Al Viro, Dave Hansen,
René Nyffenegger, Andrew Morton, Paul E . McKenney,
Petr Mladek, Andy Lutomirski, Ard Biesheuvel, Nicolas Pitre,
Sebastian Andrzej Siewior, Sergey Senozhatsky, Helge Deller,
Rik van Riel, Ingo Molnar, John Stultz, Thomas Gleixner,
Oleg Nesterov, Stanislav Kinsburskiy, Pavel Tikhomirov,
Stephen Smalley
On Wed, Mar 8, 2017 at 1:38 PM, Thomas Garnier <thgarnie@google.com> wrote:
> This patch prevents a syscall to modify the address limit of the
> caller. The address limit is kept by the syscall wrapper and restored
> just after the syscall ends.
>
> For example, it would mitigation this bug:
>
> - https://bugs.chromium.org/p/project-zero/issues/detail?id=990
>
> By default, this change warns if the segment is incorrect while
> returning to user-mode and fix it. The
> CONFIG_VERIFY_PRE_USERMODE_STATE_BUG option can be enabled to halt
> instead if needed.
Instead of this new config, please reuse the CHECK_DATA_CORRUPTION
test instead, which already controls very similar WARN vs BUG
behavior. Example below...
>
> The CONFIG_ARCH_NO_SYSCALL_VERIFY_PRE_USERMODE_STATE option is also
> added so each architecture can optimize how the
> verify_pre_usermode_state function is called.
>
> Signed-off-by: Thomas Garnier <thgarnie@google.com>
> ---
> Based on next-20170308
> ---
> include/linux/syscalls.h | 19 +++++++++++++++++++
> init/Kconfig | 16 ++++++++++++++++
> kernel/sys.c | 11 +++++++++++
> 3 files changed, 46 insertions(+)
>
> diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
> index 980c3c9b06f8..78a2268ecd6e 100644
> --- a/include/linux/syscalls.h
> +++ b/include/linux/syscalls.h
> @@ -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
> + barrier();
> + return ret;
> +}
> +#else
> +static inline bool has_user_ds(void) {
> + return false;
> +}
> +#endif
> +
> +
> #define __PROTECT(...) asmlinkage_protect(__VA_ARGS__)
> #define __SYSCALL_DEFINEx(x, name, ...) \
> asmlinkage long sys##name(__MAP(x,__SC_DECL,__VA_ARGS__)) \
> @@ -199,7 +215,10 @@ extern struct trace_event_functions exit_syscall_print_funcs;
> asmlinkage long SyS##name(__MAP(x,__SC_LONG,__VA_ARGS__)); \
> asmlinkage long SyS##name(__MAP(x,__SC_LONG,__VA_ARGS__)) \
> { \
> + bool user_caller = has_user_ds(); \
> long ret = SYSC##name(__MAP(x,__SC_CAST,__VA_ARGS__)); \
> + if (user_caller) \
> + verify_pre_usermode_state(); \
> __MAP(x,__SC_TEST,__VA_ARGS__); \
> __PROTECT(x, ret,__MAP(x,__SC_ARGS,__VA_ARGS__)); \
> return ret; \
> diff --git a/init/Kconfig b/init/Kconfig
> index c859c993c26f..ab958b59063f 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -1929,6 +1929,22 @@ config PROFILING
> config TRACEPOINTS
> bool
>
> +#
> +# Set by each architecture that want to optimize how verify_pre_usermode_state
> +# is called.
> +#
> +config ARCH_NO_SYSCALL_VERIFY_PRE_USERMODE_STATE
> + bool
> +
> +config VERIFY_PRE_USERMODE_STATE_BUG
> + bool "Halt on incorrect state on returning to user-mode"
> + default n
> + help
> + By default a warning is logged and the state is fixed. This option
> + crashes the kernel instead.
> +
> + If unsure, say Y.
> +
> source "arch/Kconfig"
>
> endmenu # General setup
> diff --git a/kernel/sys.c b/kernel/sys.c
> index 196c7134bee6..cc2ebf7fae55 100644
> --- a/kernel/sys.c
> +++ b/kernel/sys.c
> @@ -2459,3 +2459,14 @@ COMPAT_SYSCALL_DEFINE1(sysinfo, struct compat_sysinfo __user *, info)
> return 0;
> }
> #endif /* CONFIG_COMPAT */
> +
> +/* Called before coming back to user-mode */
> +asmlinkage void verify_pre_usermode_state(void)
> +{
> +#ifdef CONFIG_VERIFY_PRE_USERMODE_STATE_BUG
> + BUG_ON(!segment_eq(get_fs(), USER_DS));
> +#else
> + if (WARN_ON(!segment_eq(get_fs(), USER_DS)))
> + set_fs(USER_DS);
> +#endif
I would just make this:
if (CHECK_DATA_CORRUPTION(!segment_eq(get_fs(), USER_DS))
set_fs(USER_DS);
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v1 1/4] syscalls: Restore address limit after a syscall
2017-03-08 21:38 [PATCH v1 1/4] syscalls: Restore address limit after a syscall Thomas Garnier
` (3 preceding siblings ...)
2017-03-08 21:57 ` [PATCH v1 1/4] syscalls: Restore address limit after a syscall Kees Cook
@ 2017-03-08 21:58 ` Russell King - ARM Linux
2017-03-08 22:20 ` Andy Lutomirski
4 siblings, 1 reply; 12+ messages in thread
From: Russell King - ARM Linux @ 2017-03-08 21:58 UTC (permalink / raw)
To: Thomas Garnier
Cc: Nicolas Pitre, Mark Rutland, kernel-hardening,
Frederic Weisbecker, Sebastian Andrzej Siewior, Will Deacon,
linux-kernel, David Howells, Dave Hansen, Alexander Potapenko,
Jan Beulich, H . Peter Anvin, Borislav Petkov, Ingo Molnar,
Pavel Tikhomirov, Chris Metcalf, Helge Deller, x86,
René Nyffenegger, Catalin Marinas, Paul E . McKenney,
Stephen Smalley, Petr Mladek
On Wed, Mar 08, 2017 at 01:38:41PM -0800, Thomas Garnier wrote:
> This patch prevents a syscall to modify the address limit of the
> caller. The address limit is kept by the syscall wrapper and restored
> just after the syscall ends.
I would much rather architectures were given the opportunity to code up
checks like this efficiently (iow, inline in the exit path assembly),
rather than having to unconditionally call an additional function on
every syscall, with its register saving overheads.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v1 3/4] arm/syscalls: Specific usage of verify_pre_usermode_state
2017-03-08 21:38 ` [PATCH v1 3/4] arm/syscalls: " Thomas Garnier
2017-03-08 21:49 ` Russell King - ARM Linux
@ 2017-03-08 22:05 ` Nicolas Pitre
2017-03-08 22:33 ` Thomas Garnier
1 sibling, 1 reply; 12+ messages in thread
From: Nicolas Pitre @ 2017-03-08 22:05 UTC (permalink / raw)
To: Thomas Garnier
Cc: Arnd Bergmann, David Howells, Al Viro, Dave Hansen,
René Nyffenegger, Andrew Morton, Kees Cook,
Paul E . McKenney, Petr Mladek, Andy Lutomirski, Ard Biesheuvel,
Sebastian Andrzej Siewior, Sergey Senozhatsky, Helge Deller,
Rik van Riel, Ingo Molnar, John Stultz, Thomas Gleixner,
Oleg Nesterov, Stanislav Kinsburskiy
On Wed, 8 Mar 2017, Thomas Garnier wrote:
> Implement specific usage of verify_pre_usermode_state for user-mode
> returns for arm.
> ---
> Based on next-20170308
> ---
> arch/arm/Kconfig | 1 +
> arch/arm/kernel/entry-common.S | 5 +++++
> 2 files changed, 6 insertions(+)
>
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 0d4e71b42c77..704fd8f197fa 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -12,6 +12,7 @@ config ARM
> select ARCH_HAVE_CUSTOM_GPIO_H
> select ARCH_HAS_GCOV_PROFILE_ALL
> select ARCH_MIGHT_HAVE_PC_PARPORT
> + select ARCH_NO_SYSCALL_VERIFY_PRE_USERMODE_STATE
> select ARCH_OPTIONAL_KERNEL_RWX if ARCH_HAS_STRICT_KERNEL_RWX
> select ARCH_OPTIONAL_KERNEL_RWX_DEFAULT if CPU_V7
> select ARCH_SUPPORTS_ATOMIC_RMW
> diff --git a/arch/arm/kernel/entry-common.S b/arch/arm/kernel/entry-common.S
> index eb5cd77bf1d8..80cfdc7fabde 100644
> --- a/arch/arm/kernel/entry-common.S
> +++ b/arch/arm/kernel/entry-common.S
> @@ -39,6 +39,9 @@
> ret_fast_syscall:
> UNWIND(.fnstart )
> UNWIND(.cantunwind )
> + push {r0} @ save returned r0
> + bl verify_pre_usermode_state
> + pop {r0} @ restore r0
> disable_irq_notrace @ disable interrupts
> ldr r1, [tsk, #TI_FLAGS] @ re-check for syscall tracing
> tst r1, #_TIF_SYSCALL_WORK | _TIF_WORK_MASK
This feature is configurable, right?
Here the branch overhead is imposed even if the feature is configured
out. You should consider conditionally defining a macro like some other
features do.
Furthermore I think we still support old toolchains that don't know what
push and pop mean. You should use the legacy syntax instead.
Nicolas
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v1 1/4] syscalls: Restore address limit after a syscall
2017-03-08 21:58 ` Russell King - ARM Linux
@ 2017-03-08 22:20 ` Andy Lutomirski
2017-03-08 22:27 ` Thomas Garnier
0 siblings, 1 reply; 12+ messages in thread
From: Andy Lutomirski @ 2017-03-08 22:20 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: Thomas Garnier, Arnd Bergmann, David Howells, Al Viro,
Dave Hansen, René Nyffenegger, Andrew Morton, Kees Cook,
Paul E . McKenney, Petr Mladek, Andy Lutomirski, Ard Biesheuvel,
Nicolas Pitre, Sebastian Andrzej Siewior, Sergey Senozhatsky,
Helge Deller, Rik van Riel, Ingo Molnar, John Stultz,
Thomas Gleixner, Oleg Nesterov, Stanislav Kinsburskiy
On Wed, Mar 8, 2017 at 1:58 PM, Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
> On Wed, Mar 08, 2017 at 01:38:41PM -0800, Thomas Garnier wrote:
>> This patch prevents a syscall to modify the address limit of the
>> caller. The address limit is kept by the syscall wrapper and restored
>> just after the syscall ends.
>
> I would much rather architectures were given the opportunity to code up
> checks like this efficiently (iow, inline in the exit path assembly),
> rather than having to unconditionally call an additional function on
> every syscall, with its register saving overheads.
>
Me too. I think the two config choices should be:
(a) BUG_ON(!segment_eq(...));
(b) No generic check at all -- arch code will handle it
--Andy
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v1 1/4] syscalls: Restore address limit after a syscall
2017-03-08 22:20 ` Andy Lutomirski
@ 2017-03-08 22:27 ` Thomas Garnier
0 siblings, 0 replies; 12+ messages in thread
From: Thomas Garnier @ 2017-03-08 22:27 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Russell King - ARM Linux, Arnd Bergmann, David Howells, Al Viro,
Dave Hansen, René Nyffenegger, Andrew Morton, Kees Cook,
Paul E . McKenney, Petr Mladek, Andy Lutomirski, Ard Biesheuvel,
Nicolas Pitre, Sebastian Andrzej Siewior, Sergey Senozhatsky,
Helge Deller, Rik van Riel, Ingo Molnar, John Stultz,
Thomas Gleixner, Oleg Nesterov, Stanislav Kinsburskiy
That make sense. I will optimize each architecture to not require a call.
On Wed, Mar 8, 2017 at 2:20 PM, Andy Lutomirski <luto@amacapital.net> wrote:
> On Wed, Mar 8, 2017 at 1:58 PM, Russell King - ARM Linux
> <linux@armlinux.org.uk> wrote:
>> On Wed, Mar 08, 2017 at 01:38:41PM -0800, Thomas Garnier wrote:
>>> This patch prevents a syscall to modify the address limit of the
>>> caller. The address limit is kept by the syscall wrapper and restored
>>> just after the syscall ends.
>>
>> I would much rather architectures were given the opportunity to code up
>> checks like this efficiently (iow, inline in the exit path assembly),
>> rather than having to unconditionally call an additional function on
>> every syscall, with its register saving overheads.
>>
>
> Me too. I think the two config choices should be:
>
> (a) BUG_ON(!segment_eq(...));
>
> (b) No generic check at all -- arch code will handle it
>
> --Andy
--
Thomas
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v1 3/4] arm/syscalls: Specific usage of verify_pre_usermode_state
2017-03-08 22:05 ` Nicolas Pitre
@ 2017-03-08 22:33 ` Thomas Garnier
0 siblings, 0 replies; 12+ messages in thread
From: Thomas Garnier @ 2017-03-08 22:33 UTC (permalink / raw)
To: Nicolas Pitre
Cc: Arnd Bergmann, David Howells, Al Viro, Dave Hansen,
René Nyffenegger, Andrew Morton, Kees Cook,
Paul E . McKenney, Petr Mladek, Andy Lutomirski, Ard Biesheuvel,
Sebastian Andrzej Siewior, Sergey Senozhatsky, Helge Deller,
Rik van Riel, Ingo Molnar, John Stultz, Thomas Gleixner,
Oleg Nesterov, Stanislav Kinsburskiy, Pavel Tikhomirov,
Stephen Smalley
Make sense, as discussed on patch number one, I will write custom
assembly for each architecture when BUG_ON is not required. I will try
to use macros to make it easier to read.
On Wed, Mar 8, 2017 at 2:05 PM, Nicolas Pitre <nicolas.pitre@linaro.org> wrote:
> On Wed, 8 Mar 2017, Thomas Garnier wrote:
>
>> Implement specific usage of verify_pre_usermode_state for user-mode
>> returns for arm.
>> ---
>> Based on next-20170308
>> ---
>> arch/arm/Kconfig | 1 +
>> arch/arm/kernel/entry-common.S | 5 +++++
>> 2 files changed, 6 insertions(+)
>>
>> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
>> index 0d4e71b42c77..704fd8f197fa 100644
>> --- a/arch/arm/Kconfig
>> +++ b/arch/arm/Kconfig
>> @@ -12,6 +12,7 @@ config ARM
>> select ARCH_HAVE_CUSTOM_GPIO_H
>> select ARCH_HAS_GCOV_PROFILE_ALL
>> select ARCH_MIGHT_HAVE_PC_PARPORT
>> + select ARCH_NO_SYSCALL_VERIFY_PRE_USERMODE_STATE
>> select ARCH_OPTIONAL_KERNEL_RWX if ARCH_HAS_STRICT_KERNEL_RWX
>> select ARCH_OPTIONAL_KERNEL_RWX_DEFAULT if CPU_V7
>> select ARCH_SUPPORTS_ATOMIC_RMW
>> diff --git a/arch/arm/kernel/entry-common.S b/arch/arm/kernel/entry-common.S
>> index eb5cd77bf1d8..80cfdc7fabde 100644
>> --- a/arch/arm/kernel/entry-common.S
>> +++ b/arch/arm/kernel/entry-common.S
>> @@ -39,6 +39,9 @@
>> ret_fast_syscall:
>> UNWIND(.fnstart )
>> UNWIND(.cantunwind )
>> + push {r0} @ save returned r0
>> + bl verify_pre_usermode_state
>> + pop {r0} @ restore r0
>> disable_irq_notrace @ disable interrupts
>> ldr r1, [tsk, #TI_FLAGS] @ re-check for syscall tracing
>> tst r1, #_TIF_SYSCALL_WORK | _TIF_WORK_MASK
>
> This feature is configurable, right?
>
> Here the branch overhead is imposed even if the feature is configured
> out. You should consider conditionally defining a macro like some other
> features do.
>
> Furthermore I think we still support old toolchains that don't know what
> push and pop mean. You should use the legacy syntax instead.
>
>
> Nicolas
--
Thomas
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v1 1/4] syscalls: Restore address limit after a syscall
2017-03-08 21:57 ` [PATCH v1 1/4] syscalls: Restore address limit after a syscall Kees Cook
@ 2017-03-09 1:13 ` Thomas Garnier
0 siblings, 0 replies; 12+ messages in thread
From: Thomas Garnier @ 2017-03-09 1:13 UTC (permalink / raw)
To: Kees Cook
Cc: Arnd Bergmann, David Howells, Al Viro, Dave Hansen,
René Nyffenegger, Andrew Morton, Paul E . McKenney,
Petr Mladek, Andy Lutomirski, Ard Biesheuvel, Nicolas Pitre,
Sebastian Andrzej Siewior, Sergey Senozhatsky, Helge Deller,
Rik van Riel, Ingo Molnar, John Stultz, Thomas Gleixner,
Oleg Nesterov, Stanislav Kinsburskiy, Pavel Tikhomirov,
Stephen Smalley
On Wed, Mar 8, 2017 at 1:57 PM, Kees Cook <keescook@chromium.org> wrote:
> On Wed, Mar 8, 2017 at 1:38 PM, Thomas Garnier <thgarnie@google.com> wrote:
>> This patch prevents a syscall to modify the address limit of the
>> caller. The address limit is kept by the syscall wrapper and restored
>> just after the syscall ends.
>>
>> For example, it would mitigation this bug:
>>
>> - https://bugs.chromium.org/p/project-zero/issues/detail?id=990
>>
>> By default, this change warns if the segment is incorrect while
>> returning to user-mode and fix it. The
>> CONFIG_VERIFY_PRE_USERMODE_STATE_BUG option can be enabled to halt
>> instead if needed.
>
> Instead of this new config, please reuse the CHECK_DATA_CORRUPTION
> test instead, which already controls very similar WARN vs BUG
> behavior. Example below...
>
>>
>> The CONFIG_ARCH_NO_SYSCALL_VERIFY_PRE_USERMODE_STATE option is also
>> added so each architecture can optimize how the
>> verify_pre_usermode_state function is called.
>>
>> Signed-off-by: Thomas Garnier <thgarnie@google.com>
>> ---
>> Based on next-20170308
>> ---
>> include/linux/syscalls.h | 19 +++++++++++++++++++
>> init/Kconfig | 16 ++++++++++++++++
>> kernel/sys.c | 11 +++++++++++
>> 3 files changed, 46 insertions(+)
>>
>> diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
>> index 980c3c9b06f8..78a2268ecd6e 100644
>> --- a/include/linux/syscalls.h
>> +++ b/include/linux/syscalls.h
>> @@ -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
>> + barrier();
>> + return ret;
>> +}
>> +#else
>> +static inline bool has_user_ds(void) {
>> + return false;
>> +}
>> +#endif
>> +
>> +
>> #define __PROTECT(...) asmlinkage_protect(__VA_ARGS__)
>> #define __SYSCALL_DEFINEx(x, name, ...) \
>> asmlinkage long sys##name(__MAP(x,__SC_DECL,__VA_ARGS__)) \
>> @@ -199,7 +215,10 @@ extern struct trace_event_functions exit_syscall_print_funcs;
>> asmlinkage long SyS##name(__MAP(x,__SC_LONG,__VA_ARGS__)); \
>> asmlinkage long SyS##name(__MAP(x,__SC_LONG,__VA_ARGS__)) \
>> { \
>> + bool user_caller = has_user_ds(); \
>> long ret = SYSC##name(__MAP(x,__SC_CAST,__VA_ARGS__)); \
>> + if (user_caller) \
>> + verify_pre_usermode_state(); \
>> __MAP(x,__SC_TEST,__VA_ARGS__); \
>> __PROTECT(x, ret,__MAP(x,__SC_ARGS,__VA_ARGS__)); \
>> return ret; \
>> diff --git a/init/Kconfig b/init/Kconfig
>> index c859c993c26f..ab958b59063f 100644
>> --- a/init/Kconfig
>> +++ b/init/Kconfig
>> @@ -1929,6 +1929,22 @@ config PROFILING
>> config TRACEPOINTS
>> bool
>>
>> +#
>> +# Set by each architecture that want to optimize how verify_pre_usermode_state
>> +# is called.
>> +#
>> +config ARCH_NO_SYSCALL_VERIFY_PRE_USERMODE_STATE
>> + bool
>> +
>> +config VERIFY_PRE_USERMODE_STATE_BUG
>> + bool "Halt on incorrect state on returning to user-mode"
>> + default n
>> + help
>> + By default a warning is logged and the state is fixed. This option
>> + crashes the kernel instead.
>> +
>> + If unsure, say Y.
>> +
>> source "arch/Kconfig"
>>
>> endmenu # General setup
>> diff --git a/kernel/sys.c b/kernel/sys.c
>> index 196c7134bee6..cc2ebf7fae55 100644
>> --- a/kernel/sys.c
>> +++ b/kernel/sys.c
>> @@ -2459,3 +2459,14 @@ COMPAT_SYSCALL_DEFINE1(sysinfo, struct compat_sysinfo __user *, info)
>> return 0;
>> }
>> #endif /* CONFIG_COMPAT */
>> +
>> +/* Called before coming back to user-mode */
>> +asmlinkage void verify_pre_usermode_state(void)
>> +{
>> +#ifdef CONFIG_VERIFY_PRE_USERMODE_STATE_BUG
>> + BUG_ON(!segment_eq(get_fs(), USER_DS));
>> +#else
>> + if (WARN_ON(!segment_eq(get_fs(), USER_DS)))
>> + set_fs(USER_DS);
>> +#endif
>
> I would just make this:
>
> if (CHECK_DATA_CORRUPTION(!segment_eq(get_fs(), USER_DS))
> set_fs(USER_DS);
>
Make sense, I will remove my custom CONFIG and use that one instead
(still doing inline assembly if not set).
> -Kees
>
>
> --
> Kees Cook
> Pixel Security
--
Thomas
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2017-03-09 1:13 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-08 21:38 [PATCH v1 1/4] syscalls: Restore address limit after a syscall Thomas Garnier
2017-03-08 21:38 ` [PATCH v1 2/4] x86/syscalls: Specific usage of verify_pre_usermode_state Thomas Garnier
2017-03-08 21:38 ` [PATCH v1 3/4] arm/syscalls: " Thomas Garnier
2017-03-08 21:49 ` Russell King - ARM Linux
2017-03-08 22:05 ` Nicolas Pitre
2017-03-08 22:33 ` Thomas Garnier
2017-03-08 21:38 ` [PATCH v1 4/4] arm64/syscalls: " Thomas Garnier
2017-03-08 21:57 ` [PATCH v1 1/4] syscalls: Restore address limit after a syscall Kees Cook
2017-03-09 1:13 ` Thomas Garnier
2017-03-08 21:58 ` Russell King - ARM Linux
2017-03-08 22:20 ` Andy Lutomirski
2017-03-08 22:27 ` Thomas Garnier
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).