Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1632979 > unrolled thread
| Started by | Thomas Garnier <thgarnie@google.com> |
|---|---|
| First post | 2017-04-28 17:40 +0200 |
| Last post | 2017-05-09 08:40 +0200 |
| Articles | 20 on this page of 84 — 18 participants |
Back to article view | Back to linux.kernel
[PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Thomas Garnier <thgarnie@google.com> - 2017-04-28 17:40 +0200
Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Thomas Garnier <thgarnie@google.com> - 2017-05-06 00:20 +0200
Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Ingo Molnar <mingo@kernel.org> - 2017-05-08 09:40 +0200
Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Ingo Molnar <mingo@kernel.org> - 2017-05-08 10:00 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Daniel Micay <danielmicay@gmail.com> - 2017-05-08 17:30 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Kees Cook <keescook@chromium.org> - 2017-05-08 17:30 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Thomas Garnier <thgarnie@google.com> - 2017-05-08 22:00 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Ingo Molnar <mingo@kernel.org> - 2017-05-09 09:00 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Greg KH <greg@kroah.com> - 2017-05-09 13:20 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Thomas Garnier <thgarnie@google.com> - 2017-05-09 16:40 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Thomas Garnier <thgarnie@google.com> - 2017-05-12 01:20 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Linus Torvalds <torvalds@linux-foundation.org> - 2017-05-12 01:50 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Martin Schwidefsky <schwidefsky@de.ibm.com> - 2017-05-12 07:30 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Kees Cook <keescook@chromium.org> - 2017-05-12 07:40 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Martin Schwidefsky <schwidefsky@de.ibm.com> - 2017-05-12 08:00 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Linus Torvalds <torvalds@linux-foundation.org> - 2017-05-12 21:10 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Kees Cook <keescook@chromium.org> - 2017-05-12 21:40 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-05-12 22:30 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Peter Zijlstra <peterz@infradead.org> - 2017-05-12 22:40 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-05-12 22:50 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Kees Cook <keescook@chromium.org> - 2017-05-12 23:10 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Kees Cook <keescook@chromium.org> - 2017-05-12 23:10 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Christoph Hellwig <hch@infradead.org> - 2017-05-13 09:30 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Al Viro <viro@ZenIV.linux.org.uk> - 2017-05-12 23:10 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Daniel Micay <danielmicay@gmail.com> - 2017-05-12 23:20 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Kees Cook <keescook@chromium.org> - 2017-05-12 23:20 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Daniel Micay <danielmicay@gmail.com> - 2017-05-12 23:30 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Al Viro <viro@ZenIV.linux.org.uk> - 2017-05-12 23:50 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Rik van Riel <riel@redhat.com> - 2017-05-12 23:50 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Al Viro <viro@ZenIV.linux.org.uk> - 2017-05-13 01:00 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Kees Cook <keescook@chromium.org> - 2017-05-13 00:00 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Kees Cook <keescook@chromium.org> - 2017-05-12 21:10 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-05-12 21:20 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Ingo Molnar <mingo@kernel.org> - 2017-05-12 09:00 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Andy Lutomirski <luto@kernel.org> - 2017-05-12 08:20 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Ingo Molnar <mingo@kernel.org> - 2017-05-12 09:00 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Thomas Garnier <thgarnie@google.com> - 2017-05-12 19:10 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Kees Cook <keescook@chromium.org> - 2017-05-09 18:40 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Greg KH <greg@kroah.com> - 2017-05-08 14:50 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Ingo Molnar <mingo@kernel.org> - 2017-05-09 08:50 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Christoph Hellwig <hch@infradead.org> - 2017-05-09 11:00 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Andy Lutomirski <luto@kernel.org> - 2017-05-09 15:10 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Christoph Hellwig <hch@infradead.org> - 2017-05-09 15:10 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Christoph Hellwig <hch@infradead.org> - 2017-05-09 18:10 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Kees Cook <keescook@chromium.org> - 2017-05-09 19:00 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Andy Lutomirski <luto@amacapital.net> - 2017-05-10 01:00 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Kees Cook <keescook@chromium.org> - 2017-05-10 01:40 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Andy Lutomirski <luto@kernel.org> - 2017-05-10 04:10 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Christoph Hellwig <hch@infradead.org> - 2017-05-10 09:20 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Borislav Petkov <bp@alien8.de> - 2017-05-11 13:30 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Christoph Hellwig <hch@infradead.org> - 2017-05-10 08:50 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Al Viro <viro@ZenIV.linux.org.uk> - 2017-05-10 04:20 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Al Viro <viro@ZenIV.linux.org.uk> - 2017-05-10 04:50 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Al Viro <viro@ZenIV.linux.org.uk> - 2017-05-10 05:20 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Al Viro <viro@ZenIV.linux.org.uk> - 2017-05-10 05:30 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Al Viro <viro@ZenIV.linux.org.uk> - 2017-05-10 05:50 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Christoph Hellwig <hch@infradead.org> - 2017-05-10 09:00 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Christoph Hellwig <hch@infradead.org> - 2017-05-10 09:00 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Al Viro <viro@ZenIV.linux.org.uk> - 2017-05-10 09:30 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Christoph Hellwig <hch@infradead.org> - 2017-05-10 09:40 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Christoph Hellwig <hch@infradead.org> - 2017-05-10 09:00 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Arnd Bergmann <arnd@arndb.de> - 2017-05-10 09:30 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Christoph Hellwig <hch@infradead.org> - 2017-05-10 09:40 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Brian Gerst <brgerst@gmail.com> - 2017-05-09 18:10 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Arnd Bergmann <arnd@arndb.de> - 2017-05-10 09:40 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Al Viro <viro@ZenIV.linux.org.uk> - 2017-05-10 10:10 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Christoph Hellwig <hch@infradead.org> - 2017-05-10 10:20 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Andy Lutomirski <luto@kernel.org> - 2017-05-11 02:20 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Ingo Molnar <mingo@kernel.org> - 2017-05-12 09:10 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Al Viro <viro@ZenIV.linux.org.uk> - 2017-05-12 09:20 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Christoph Hellwig <hch@infradead.org> - 2017-05-12 09:40 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Arnd Bergmann <arnd@arndb.de> - 2017-05-12 09:50 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Christoph Hellwig <hch@infradead.org> - 2017-05-12 10:20 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Al Viro <viro@ZenIV.linux.org.uk> - 2017-05-12 10:20 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Al Viro <viro@ZenIV.linux.org.uk> - 2017-05-12 10:20 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Arnd Bergmann <arnd@arndb.de> - 2017-05-12 10:30 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Andy Lutomirski <luto@kernel.org> - 2017-05-13 01:30 +0200
Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Kees Cook <keescook@chromium.org> - 2017-05-08 15:20 +0200
Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Jann Horn <jannh@google.com> - 2017-05-08 16:10 +0200
Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Al Viro <viro@ZenIV.linux.org.uk> - 2017-05-08 22:50 +0200
Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Andy Lutomirski <luto@kernel.org> - 2017-05-13 01:20 +0200
Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Ingo Molnar <mingo@kernel.org> - 2017-05-08 16:10 +0200
Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Kees Cook <keescook@chromium.org> - 2017-05-08 17:30 +0200
Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Ingo Molnar <mingo@kernel.org> - 2017-05-09 08:40 +0200
Page 1 of 5 [1] 2 3 4 5 Next page →
| From | Thomas Garnier <thgarnie@google.com> |
|---|---|
| Date | 2017-04-28 17:40 +0200 |
| Subject | [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode |
| Message-ID | <tBg9X-Qc-7@gated-at.bofh.it> |
Ensure that a syscall does not return to user-mode with a kernel address
limit. If that happens, a process can corrupt kernel-mode memory and
elevate privileges [1].
The CONFIG_ADDR_LIMIT_CHECK option disables the generic check so each
architecture can create optimized versions. This option is enabled by
default on s390 because a similar feature already exists.
[1] https://bugs.chromium.org/p/project-zero/issues/detail?id=990
Signed-off-by: Thomas Garnier <thgarnie@google.com>
Tested-by: Kees Cook <keescook@chromium.org>
---
Based on next-20170426
---
arch/s390/Kconfig | 1 +
include/linux/syscalls.h | 27 ++++++++++++++++++++++++++-
init/Kconfig | 6 ++++++
kernel/sys.c | 13 +++++++++++++
4 files changed, 46 insertions(+), 1 deletion(-)
diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
index d25435d94b6e..3d2ec084d5fc 100644
--- a/arch/s390/Kconfig
+++ b/arch/s390/Kconfig
@@ -64,6 +64,7 @@ config ARCH_SUPPORTS_UPROBES
config S390
def_bool y
+ select ADDR_LIMIT_CHECK
select ARCH_HAS_DEVMEM_IS_ALLOWED
select ARCH_HAS_ELF_RANDOMIZE
select ARCH_HAS_GCOV_PROFILE_ALL
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index 980c3c9b06f8..e534b93ce43a 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -191,6 +191,28 @@ extern struct trace_event_functions exit_syscall_print_funcs;
SYSCALL_METADATA(sname, x, __VA_ARGS__) \
__SYSCALL_DEFINEx(x, sname, __VA_ARGS__)
+
+/*
+ * Called before coming back to user-mode. Returning to user-mode with an
+ * address limit different than USER_DS can allow to overwrite kernel memory.
+ */
+static inline void addr_limit_check_syscall(void)
+{
+ BUG_ON(!segment_eq(get_fs(), USER_DS));
+}
+
+#ifndef CONFIG_ADDR_LIMIT_CHECK
+#define ADDR_LIMIT_CHECK_PRE() \
+ bool user_caller = segment_eq(get_fs(), USER_DS)
+#define ADDR_LIMIT_CHECK_POST() \
+ if (user_caller) addr_limit_check_syscall()
+#else
+#define ADDR_LIMIT_CHECK_PRE()
+#define ADDR_LIMIT_CHECK_POST()
+asmlinkage void addr_limit_check_failed(void) __noreturn;
+#endif
+
+
#define __PROTECT(...) asmlinkage_protect(__VA_ARGS__)
#define __SYSCALL_DEFINEx(x, name, ...) \
asmlinkage long sys##name(__MAP(x,__SC_DECL,__VA_ARGS__)) \
@@ -199,7 +221,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__)) \
{ \
- long ret = SYSC##name(__MAP(x,__SC_CAST,__VA_ARGS__)); \
+ long ret; \
+ ADDR_LIMIT_CHECK_PRE(); \
+ ret = SYSC##name(__MAP(x,__SC_CAST,__VA_ARGS__)); \
+ ADDR_LIMIT_CHECK_POST(); \
__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 42a346b0df43..599d9fe30703 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1961,6 +1961,12 @@ config PROFILING
config TRACEPOINTS
bool
+config ADDR_LIMIT_CHECK
+ bool
+ help
+ Disable the generic address limit check. Allow each architecture to
+ optimize how and when the verification is done.
+
source "arch/Kconfig"
endmenu # General setup
diff --git a/kernel/sys.c b/kernel/sys.c
index 8a94b4eabcaa..a1cbcd715d62 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -2458,3 +2458,16 @@ COMPAT_SYSCALL_DEFINE1(sysinfo, struct compat_sysinfo __user *, info)
return 0;
}
#endif /* CONFIG_COMPAT */
+
+#ifdef CONFIG_ADDR_LIMIT_CHECK
+/*
+ * Used when an architecture specific implementation detects an invalid address
+ * limit. This function does not return.
+ */
+asmlinkage void addr_limit_check_failed(void)
+{
+ /* Try to fail on the generic address limit check */
+ addr_limit_check_syscall();
+ panic("Invalid address limit before returning to user-mode");
+}
+#endif
--
2.13.0.rc0.306.g87b477812d-goog
[toc] | [next] | [standalone]
| From | Thomas Garnier <thgarnie@google.com> |
|---|---|
| Date | 2017-05-06 00:20 +0200 |
| Message-ID | <tDTJT-7Nn-3@gated-at.bofh.it> |
| In reply to | #1632979 |
On Fri, Apr 28, 2017 at 8:32 AM, Thomas Garnier <thgarnie@google.com> wrote:
> Ensure that a syscall does not return to user-mode with a kernel address
> limit. If that happens, a process can corrupt kernel-mode memory and
> elevate privileges [1].
>
> The CONFIG_ADDR_LIMIT_CHECK option disables the generic check so each
> architecture can create optimized versions. This option is enabled by
> default on s390 because a similar feature already exists.
>
> [1] https://bugs.chromium.org/p/project-zero/issues/detail?id=990
>
> Signed-off-by: Thomas Garnier <thgarnie@google.com>
> Tested-by: Kees Cook <keescook@chromium.org>
Ingo: Do you want to take the set?
> ---
> Based on next-20170426
> ---
> arch/s390/Kconfig | 1 +
> include/linux/syscalls.h | 27 ++++++++++++++++++++++++++-
> init/Kconfig | 6 ++++++
> kernel/sys.c | 13 +++++++++++++
> 4 files changed, 46 insertions(+), 1 deletion(-)
>
> diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
> index d25435d94b6e..3d2ec084d5fc 100644
> --- a/arch/s390/Kconfig
> +++ b/arch/s390/Kconfig
> @@ -64,6 +64,7 @@ config ARCH_SUPPORTS_UPROBES
>
> config S390
> def_bool y
> + select ADDR_LIMIT_CHECK
> select ARCH_HAS_DEVMEM_IS_ALLOWED
> select ARCH_HAS_ELF_RANDOMIZE
> select ARCH_HAS_GCOV_PROFILE_ALL
> diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
> index 980c3c9b06f8..e534b93ce43a 100644
> --- a/include/linux/syscalls.h
> +++ b/include/linux/syscalls.h
> @@ -191,6 +191,28 @@ extern struct trace_event_functions exit_syscall_print_funcs;
> SYSCALL_METADATA(sname, x, __VA_ARGS__) \
> __SYSCALL_DEFINEx(x, sname, __VA_ARGS__)
>
> +
> +/*
> + * Called before coming back to user-mode. Returning to user-mode with an
> + * address limit different than USER_DS can allow to overwrite kernel memory.
> + */
> +static inline void addr_limit_check_syscall(void)
> +{
> + BUG_ON(!segment_eq(get_fs(), USER_DS));
> +}
> +
> +#ifndef CONFIG_ADDR_LIMIT_CHECK
> +#define ADDR_LIMIT_CHECK_PRE() \
> + bool user_caller = segment_eq(get_fs(), USER_DS)
> +#define ADDR_LIMIT_CHECK_POST() \
> + if (user_caller) addr_limit_check_syscall()
> +#else
> +#define ADDR_LIMIT_CHECK_PRE()
> +#define ADDR_LIMIT_CHECK_POST()
> +asmlinkage void addr_limit_check_failed(void) __noreturn;
> +#endif
> +
> +
> #define __PROTECT(...) asmlinkage_protect(__VA_ARGS__)
> #define __SYSCALL_DEFINEx(x, name, ...) \
> asmlinkage long sys##name(__MAP(x,__SC_DECL,__VA_ARGS__)) \
> @@ -199,7 +221,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__)) \
> { \
> - long ret = SYSC##name(__MAP(x,__SC_CAST,__VA_ARGS__)); \
> + long ret; \
> + ADDR_LIMIT_CHECK_PRE(); \
> + ret = SYSC##name(__MAP(x,__SC_CAST,__VA_ARGS__)); \
> + ADDR_LIMIT_CHECK_POST(); \
> __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 42a346b0df43..599d9fe30703 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -1961,6 +1961,12 @@ config PROFILING
> config TRACEPOINTS
> bool
>
> +config ADDR_LIMIT_CHECK
> + bool
> + help
> + Disable the generic address limit check. Allow each architecture to
> + optimize how and when the verification is done.
> +
> source "arch/Kconfig"
>
> endmenu # General setup
> diff --git a/kernel/sys.c b/kernel/sys.c
> index 8a94b4eabcaa..a1cbcd715d62 100644
> --- a/kernel/sys.c
> +++ b/kernel/sys.c
> @@ -2458,3 +2458,16 @@ COMPAT_SYSCALL_DEFINE1(sysinfo, struct compat_sysinfo __user *, info)
> return 0;
> }
> #endif /* CONFIG_COMPAT */
> +
> +#ifdef CONFIG_ADDR_LIMIT_CHECK
> +/*
> + * Used when an architecture specific implementation detects an invalid address
> + * limit. This function does not return.
> + */
> +asmlinkage void addr_limit_check_failed(void)
> +{
> + /* Try to fail on the generic address limit check */
> + addr_limit_check_syscall();
> + panic("Invalid address limit before returning to user-mode");
> +}
> +#endif
> --
> 2.13.0.rc0.306.g87b477812d-goog
>
--
Thomas
[toc] | [prev] | [next] | [standalone]
| From | Ingo Molnar <mingo@kernel.org> |
|---|---|
| Date | 2017-05-08 09:40 +0200 |
| Subject | Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode |
| Message-ID | <tELqV-VP-1@gated-at.bofh.it> |
| In reply to | #1636763 |
(added more Cc:s)
* Thomas Garnier <thgarnie@google.com> wrote:
> On Fri, Apr 28, 2017 at 8:32 AM, Thomas Garnier <thgarnie@google.com> wrote:
> > Ensure that a syscall does not return to user-mode with a kernel address
> > limit. If that happens, a process can corrupt kernel-mode memory and
> > elevate privileges [1].
> >
> > The CONFIG_ADDR_LIMIT_CHECK option disables the generic check so each
> > architecture can create optimized versions. This option is enabled by
> > default on s390 because a similar feature already exists.
> >
> > [1] https://bugs.chromium.org/p/project-zero/issues/detail?id=990
> >
> > Signed-off-by: Thomas Garnier <thgarnie@google.com>
> > Tested-by: Kees Cook <keescook@chromium.org>
>
> Ingo: Do you want to take the set?
Yeah, so now I'm questioning the whole premise of the feature, sorry :-/
A big disavantage is that the "security check" will add 2-5 instructions to the
system call fast path. Every one of them, and essentially forever. Just to handle
a CVE that was caused by a buggy touch-screen driver helper function leaking
KERNEL_DS and which was fixed long ago ...
And yes, I realize that there were other such bugs and that such bugs might occur
in the future - but why not push the overhead of the security check to the kernel
build phase? I.e. I'm wondering how well we could do static analysis during kernel
build - would a limited mode of Sparse be good enough for that? Or we could add a
new static checker to tools/, built from first principles and used primarily for
extended syntactical checking.
For example I'd consider it a good practice to mandate that if a kernel function
sets KERNEL_DS then it must restore it as well. Any function that does not do
that, or is too complex for the static analysis to prove correctness for sure
should be considered buggy!
Are there any common kernel APIs outside set_fs() that set KERNEL_DS
intentionally? The overwhelming pattern ought to be:
orig_fs = get_fs();
set_fs(KERNEL_DS);
...
set_fs(orig_fs);
... and even a relatively simple static analysis tool ought to be able to see
through that.
I'd even suggest we do it not like Sparse builds are done today, but in a more
integrated fashion: do static analysis as part of a typical kernel defconfig build
and not tolerate warnings but go for a 'zero warnings' policy like Linus uses for
modconfig builds.
_That_ solution I'd feel very, very good about - it would be so much better than
any runtime checks...
Not to mention that such an integrated static analysis facility would allow many
other things to be checked during build time, which we couldn't possibly check
runtime.
Thanks,
Ingo
[toc] | [prev] | [next] | [standalone]
| From | Ingo Molnar <mingo@kernel.org> |
|---|---|
| Date | 2017-05-08 10:00 +0200 |
| Subject | Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode |
| Message-ID | <tELKi-14K-19@gated-at.bofh.it> |
| In reply to | #1637273 |
* Ingo Molnar <mingo@kernel.org> wrote:
> ... and even a relatively simple static analysis tool ought to be able to see
> through that.
>
> I'd even suggest we do it not like Sparse builds are done today, but in a more
> integrated fashion: do static analysis as part of a typical kernel defconfig
> build and not tolerate warnings but go for a 'zero warnings' policy like Linus
> uses for modconfig builds.
>
> _That_ solution I'd feel very, very good about - it would be so much better than
> any runtime checks...
So the problem I have with Sparse is that it is very spammy. For example:
make C=1 kernel/sched/
... produces:
kernel/sched/core.c:792:6: warning: symbol 'sched_set_stop_task' was not declared. Should it be static?
kernel/sched/core.c:1298:5: warning: symbol 'migrate_swap' was not declared. Should it be static?
kernel/sched/core.c:3648:35: warning: symbol 'preempt_schedule_irq' was not declared. Should it be static?
./include/linux/uaccess.h:166:18: warning: incorrect type in argument 1 (different modifiers)
./include/linux/uaccess.h:166:18: expected void *<noident>
./include/linux/uaccess.h:166:18: got void const *from
./include/linux/uaccess.h:166:18: warning: incorrect type in argument 1 (different modifiers)
./include/linux/uaccess.h:166:18: expected void *<noident>
./include/linux/uaccess.h:166:18: got void const *from
./include/linux/uaccess.h:166:18: warning: incorrect type in argument 1 (different modifiers)
./include/linux/uaccess.h:166:18: expected void *<noident>
./include/linux/uaccess.h:166:18: got void const *from
./include/linux/uaccess.h:166:18: warning: incorrect type in argument 1 (different modifiers)
./include/linux/uaccess.h:166:18: expected void *<noident>
./include/linux/uaccess.h:166:18: got void const *from
kernel/sched/clock.c:80:19: warning: symbol 'sched_clock_running' was not declared. Should it be static?
./include/linux/sched/cputime.h:83:14: warning: expression using sizeof bool
./include/linux/sched/cputime.h:83:14: warning: expression using sizeof bool
./include/linux/sched/cputime.h:83:14: warning: expression using sizeof bool
./include/linux/sched/cputime.h:83:14: warning: expression using sizeof bool
./include/linux/sched/cputime.h:83:14: warning: expression using sizeof bool
./include/linux/sched/cputime.h:83:14: warning: expression using sizeof bool
kernel/sched/cputime.c:335:33: warning: context imbalance in 'thread_group_cputime' - different lock contexts for basic block
kernel/sched/fair.c:54:14: warning: symbol 'normalized_sysctl_sched_latency' was not declared. Should it be static?
kernel/sched/fair.c:75:14: warning: symbol 'normalized_sysctl_sched_min_granularity' was not declared. Should it be static?
kernel/sched/fair.c:98:14: warning: symbol 'normalized_sysctl_sched_wakeup_granularity' was not declared. Should it be static?
kernel/sched/fair.c:132:14: warning: symbol 'capacity_margin' was not declared. Should it be static?
kernel/sched/sched.h:1988:16: error: cannot dereference this type
kernel/sched/sched.h:1988:16: error: cannot dereference this type
kernel/sched/sched.h:1988:16: error: internal error: bad type in derived(11)
kernel/sched/sched.h:1988:16: error: cannot dereference this type
kernel/sched/sched.h:1988:16: error: incompatible types in comparison expression (different base types)
kernel/sched/sched.h:1988:16: error: cannot dereference this type
kernel/sched/fair.c:4688:35: error: marked inline, but without a definition
kernel/sched/sched.h:1988:16: error: internal error: bad type in derived(11)
kernel/sched/sched.h:1988:16: error: incompatible types in comparison expression (different base types)
kernel/sched/sched.h:1988:16: error: internal error: bad type in derived(11)
kernel/sched/sched.h:1988:16: error: incompatible types in comparison expression (different base types)
kernel/sched/sched.h:1988:16: error: internal error: bad type in derived(11)
kernel/sched/sched.h:1988:16: error: incompatible types in comparison expression (different base types)
kernel/sched/sched.h:1988:16: error: internal error: bad type in derived(11)
kernel/sched/sched.h:1988:16: error: incompatible types in comparison expression (different base types)
kernel/sched/sched.h:1988:16: error: internal error: bad type in derived(11)
kernel/sched/sched.h:1988:16: error: incompatible types in comparison expression (different base types)
kernel/sched/sched.h:1988:16: error: internal error: bad type in derived(11)
kernel/sched/sched.h:1988:16: error: incompatible types in comparison expression (different base types)
kernel/sched/sched.h:1988:16: error: internal error: bad type in derived(11)
kernel/sched/sched.h:1988:16: error: incompatible types in comparison expression (different base types)
kernel/sched/sched.h:1988:16: error: internal error: bad type in derived(11)
kernel/sched/sched.h:1988:16: error: incompatible types in comparison expression (different base types)
kernel/sched/sched.h:1988:16: error: internal error: bad type in derived(11)
kernel/sched/sched.h:1988:16: error: incompatible types in comparison expression (different base types)
kernel/sched/sched.h:1988:16: error: internal error: bad type in derived(11)
kernel/sched/sched.h:1988:16: error: incompatible types in comparison expression (different base types)
kernel/sched/fair.c:5817:19: error: cannot dereference this type
kernel/sched/fair.c:5817:19: error: cannot dereference this type
kernel/sched/fair.c:5817:19: error: internal error: bad type in derived(11)
kernel/sched/fair.c:5817:19: error: cannot dereference this type
kernel/sched/fair.c:5817:19: error: incompatible types in comparison expression (different base types)
kernel/sched/fair.c:5817:19: error: cannot dereference this type
kernel/sched/sched.h:1988:16: error: internal error: bad type in derived(11)
kernel/sched/sched.h:1988:16: error: incompatible types in comparison expression (different base types)
kernel/sched/sched.h:1988:16: error: internal error: bad type in derived(11)
kernel/sched/sched.h:1988:16: error: incompatible types in comparison expression (different base types)
kernel/sched/sched.h:1988:16: error: internal error: bad type in derived(11)
kernel/sched/sched.h:1988:16: error: incompatible types in comparison expression (different base types)
kernel/sched/sched.h:1988:16: error: internal error: bad type in derived(11)
kernel/sched/sched.h:1988:16: error: incompatible types in comparison expression (different base types)
kernel/sched/sched.h:1988:16: error: internal error: bad type in derived(11)
kernel/sched/sched.h:1988:16: error: incompatible types in comparison expression (different base types)
kernel/sched/rt.c:635:6: warning: symbol 'sched_rt_bandwidth_account' was not declared. Should it be static?
kernel/sched/sched.h:1988:16: error: cannot dereference this type
kernel/sched/sched.h:1988:16: error: cannot dereference this type
kernel/sched/sched.h:1988:16: error: internal error: bad type in derived(11)
kernel/sched/sched.h:1988:16: error: cannot dereference this type
kernel/sched/sched.h:1988:16: error: incompatible types in comparison expression (different base types)
kernel/sched/sched.h:1988:16: error: cannot dereference this type
kernel/sched/sched.h:1988:16: error: cannot dereference this type
kernel/sched/sched.h:1988:16: error: cannot dereference this type
kernel/sched/sched.h:1988:16: error: internal error: bad type in derived(11)
kernel/sched/sched.h:1988:16: error: cannot dereference this type
kernel/sched/sched.h:1988:16: error: incompatible types in comparison expression (different base types)
kernel/sched/sched.h:1988:16: error: cannot dereference this type
./include/linux/sched/cputime.h:83:14: warning: expression using sizeof bool
./include/linux/sched/cputime.h:83:14: warning: expression using sizeof bool
kernel/sched/topology.c:499:28: warning: incorrect type in initializer (different address spaces)
kernel/sched/topology.c:499:28: expected void const [noderef] <asn:3>*__vpp_verify
kernel/sched/topology.c:499:28: got struct sched_domain **<noident>
kernel/sched/topology.c:534:28: warning: incorrect type in initializer (different address spaces)
kernel/sched/topology.c:534:28: expected void const [noderef] <asn:3>*__vpp_verify
kernel/sched/topology.c:534:28: got struct sched_domain **<noident>
kernel/sched/topology.c:554:28: warning: incorrect type in initializer (different address spaces)
kernel/sched/topology.c:554:28: expected void const [noderef] <asn:3>*__vpp_verify
kernel/sched/topology.c:554:28: got struct sched_group_capacity **<noident>
kernel/sched/topology.c:594:36: warning: incorrect type in initializer (different address spaces)
kernel/sched/topology.c:594:36: expected void const [noderef] <asn:3>*__vpp_verify
kernel/sched/topology.c:594:36: got struct sched_domain **<noident>
kernel/sched/topology.c:601:24: warning: incorrect type in initializer (different address spaces)
kernel/sched/topology.c:601:24: expected void const [noderef] <asn:3>*__vpp_verify
kernel/sched/topology.c:601:24: got struct sched_group **<noident>
kernel/sched/topology.c:602:31: warning: incorrect type in initializer (different address spaces)
kernel/sched/topology.c:602:31: expected void const [noderef] <asn:3>*__vpp_verify
kernel/sched/topology.c:602:31: got struct sched_group_capacity **<noident>
kernel/sched/topology.c:1330:39: warning: incorrect type in initializer (different address spaces)
kernel/sched/topology.c:1330:39: expected void const [noderef] <asn:3>*__vpp_verify
kernel/sched/topology.c:1330:39: got struct sched_domain **<noident>
kernel/sched/topology.c:1333:40: warning: incorrect type in initializer (different address spaces)
kernel/sched/topology.c:1333:40: expected void const [noderef] <asn:3>*__vpp_verify
kernel/sched/topology.c:1333:40: got struct sched_domain **<noident>
kernel/sched/topology.c:1337:40: warning: incorrect type in initializer (different address spaces)
kernel/sched/topology.c:1337:40: expected void const [noderef] <asn:3>*__vpp_verify
kernel/sched/topology.c:1337:40: got struct sched_domain_shared **<noident>
kernel/sched/topology.c:1339:40: warning: incorrect type in initializer (different address spaces)
kernel/sched/topology.c:1339:40: expected void const [noderef] <asn:3>*__vpp_verify
kernel/sched/topology.c:1339:40: got struct sched_group **<noident>
kernel/sched/topology.c:1341:40: warning: incorrect type in initializer (different address spaces)
kernel/sched/topology.c:1341:40: expected void const [noderef] <asn:3>*__vpp_verify
kernel/sched/topology.c:1341:40: got struct sched_group_capacity **<noident>
kernel/sched/topology.c:1343:32: warning: incorrect type in argument 1 (different address spaces)
kernel/sched/topology.c:1343:32: expected void [noderef] <asn:3>*__pdata
kernel/sched/topology.c:1343:32: got struct sched_domain **[noderef] sd
kernel/sched/topology.c:1345:32: warning: incorrect type in argument 1 (different address spaces)
kernel/sched/topology.c:1345:32: expected void [noderef] <asn:3>*__pdata
kernel/sched/topology.c:1345:32: got struct sched_domain_shared **[noderef] sds
kernel/sched/topology.c:1347:32: warning: incorrect type in argument 1 (different address spaces)
kernel/sched/topology.c:1347:32: expected void [noderef] <asn:3>*__pdata
kernel/sched/topology.c:1347:32: got struct sched_group **[noderef] sg
kernel/sched/topology.c:1349:32: warning: incorrect type in argument 1 (different address spaces)
kernel/sched/topology.c:1349:32: expected void [noderef] <asn:3>*__pdata
kernel/sched/topology.c:1349:32: got struct sched_group_capacity **[noderef] sgc
kernel/sched/topology.c:1261:25: warning: incorrect type in assignment (different address spaces)
kernel/sched/topology.c:1261:25: expected struct sched_domain **[noderef] sd
kernel/sched/topology.c:1261:25: got struct sched_domain *[noderef] <asn:3>*<noident>
kernel/sched/topology.c:1265:26: warning: incorrect type in assignment (different address spaces)
kernel/sched/topology.c:1265:26: expected struct sched_domain_shared **[noderef] sds
kernel/sched/topology.c:1265:26: got struct sched_domain_shared *[noderef] <asn:3>*<noident>
kernel/sched/topology.c:1269:25: warning: incorrect type in assignment (different address spaces)
kernel/sched/topology.c:1269:25: expected struct sched_group **[noderef] sg
kernel/sched/topology.c:1269:25: got struct sched_group *[noderef] <asn:3>*<noident>
kernel/sched/topology.c:1273:26: warning: incorrect type in assignment (different address spaces)
kernel/sched/topology.c:1273:26: expected struct sched_group_capacity **[noderef] sgc
kernel/sched/topology.c:1273:26: got struct sched_group_capacity *[noderef] <asn:3>*<noident>
kernel/sched/topology.c:1288:26: warning: incorrect type in initializer (different address spaces)
kernel/sched/topology.c:1288:26: expected void const [noderef] <asn:3>*__vpp_verify
kernel/sched/topology.c:1288:26: got struct sched_domain **<noident>
kernel/sched/topology.c:1295:26: warning: incorrect type in initializer (different address spaces)
kernel/sched/topology.c:1295:26: expected void const [noderef] <asn:3>*__vpp_verify
kernel/sched/topology.c:1295:26: got struct sched_domain_shared **<noident>
kernel/sched/topology.c:1304:26: warning: incorrect type in initializer (different address spaces)
kernel/sched/topology.c:1304:26: expected void const [noderef] <asn:3>*__vpp_verify
kernel/sched/topology.c:1304:26: got struct sched_group **<noident>
kernel/sched/topology.c:1311:26: warning: incorrect type in initializer (different address spaces)
kernel/sched/topology.c:1311:26: expected void const [noderef] <asn:3>*__vpp_verify
kernel/sched/topology.c:1311:26: got struct sched_group_capacity **<noident>
kernel/sched/topology.c:759:30: warning: incorrect type in argument 1 (different address spaces)
kernel/sched/topology.c:759:30: expected void [noderef] <asn:3>*__pdata
kernel/sched/topology.c:759:30: got struct sched_domain **[noderef] sd
kernel/sched/topology.c:776:15: warning: incorrect type in assignment (different address spaces)
kernel/sched/topology.c:776:15: expected struct sched_domain **[noderef] sd
kernel/sched/topology.c:776:15: got struct sched_domain *[noderef] <asn:3>*<noident>
kernel/sched/topology.c:794:9: warning: incorrect type in initializer (different address spaces)
kernel/sched/topology.c:794:9: expected void const [noderef] <asn:3>*__vpp_verify
kernel/sched/topology.c:794:9: got struct sched_domain **<noident>
kernel/sched/topology.c:795:10: warning: incorrect type in initializer (different address spaces)
kernel/sched/topology.c:795:10: expected void const [noderef] <asn:3>*__vpp_verify
kernel/sched/topology.c:795:10: got struct sched_domain **<noident>
kernel/sched/topology.c:797:28: warning: incorrect type in initializer (different address spaces)
kernel/sched/topology.c:797:28: expected void const [noderef] <asn:3>*__vpp_verify
kernel/sched/topology.c:797:28: got struct sched_domain_shared **<noident>
kernel/sched/topology.c:798:18: warning: incorrect type in initializer (different address spaces)
kernel/sched/topology.c:798:18: expected void const [noderef] <asn:3>*__vpp_verify
kernel/sched/topology.c:798:18: got struct sched_domain_shared **<noident>
kernel/sched/topology.c:800:28: warning: incorrect type in initializer (different address spaces)
kernel/sched/topology.c:800:28: expected void const [noderef] <asn:3>*__vpp_verify
kernel/sched/topology.c:800:28: got struct sched_group **<noident>
kernel/sched/topology.c:801:18: warning: incorrect type in initializer (different address spaces)
kernel/sched/topology.c:801:18: expected void const [noderef] <asn:3>*__vpp_verify
kernel/sched/topology.c:801:18: got struct sched_group **<noident>
kernel/sched/topology.c:803:28: warning: incorrect type in initializer (different address spaces)
kernel/sched/topology.c:803:28: expected void const [noderef] <asn:3>*__vpp_verify
kernel/sched/topology.c:803:28: got struct sched_group_capacity **<noident>
kernel/sched/topology.c:804:18: warning: incorrect type in initializer (different address spaces)
kernel/sched/topology.c:804:18: expected void const [noderef] <asn:3>*__vpp_verify
kernel/sched/topology.c:804:18: got struct sched_group_capacity **<noident>
kernel/sched/topology.c:848:36: warning: incorrect type in initializer (different address spaces)
kernel/sched/topology.c:848:36: expected void const [noderef] <asn:3>*__vpp_verify
kernel/sched/topology.c:848:36: got struct sched_domain **<noident>
kernel/sched/topology.c:954:31: warning: incorrect type in initializer (different address spaces)
kernel/sched/topology.c:954:31: expected void const [noderef] <asn:3>*__vpp_verify
kernel/sched/topology.c:954:31: got struct sched_domain_shared **<noident>
kernel/sched/topology.c:1354:21: warning: symbol 'build_sched_domain' was not declared. Should it be static?
kernel/sched/topology.c:1409:34: warning: incorrect type in initializer (different address spaces)
kernel/sched/topology.c:1409:34: expected void const [noderef] <asn:3>*__vpp_verify
kernel/sched/topology.c:1409:34: got struct sched_domain **<noident>
kernel/sched/topology.c:1419:28: warning: incorrect type in initializer (different address spaces)
kernel/sched/topology.c:1419:28: expected void const [noderef] <asn:3>*__vpp_verify
kernel/sched/topology.c:1419:28: got struct sched_domain **<noident>
kernel/sched/topology.c:1436:28: warning: incorrect type in initializer (different address spaces)
kernel/sched/topology.c:1436:28: expected void const [noderef] <asn:3>*__vpp_verify
kernel/sched/topology.c:1436:28: got struct sched_domain **<noident>
kernel/sched/topology.c:1446:23: warning: incorrect type in initializer (different address spaces)
kernel/sched/topology.c:1446:23: expected void const [noderef] <asn:3>*__vpp_verify
kernel/sched/topology.c:1446:23: got struct sched_domain **<noident>
kernel/sched/topology.c:759:29: warning: dereference of noderef expression
kernel/sched/topology.c:777:14: warning: dereference of noderef expression
kernel/sched/topology.c:1262:22: warning: dereference of noderef expression
kernel/sched/topology.c:1266:22: warning: dereference of noderef expression
kernel/sched/topology.c:1270:22: warning: dereference of noderef expression
kernel/sched/topology.c:1274:22: warning: dereference of noderef expression
kernel/sched/topology.c:1329:29: warning: dereference of noderef expression
kernel/sched/topology.c:1336:29: warning: dereference of noderef expression
kernel/sched/topology.c:1338:29: warning: dereference of noderef expression
kernel/sched/topology.c:1340:29: warning: dereference of noderef expression
kernel/sched/topology.c:1343:29: warning: dereference of noderef expression
kernel/sched/topology.c:1344:17: warning: dereference of noderef expression
kernel/sched/topology.c:1345:29: warning: dereference of noderef expression
kernel/sched/topology.c:1346:17: warning: dereference of noderef expression
kernel/sched/topology.c:1347:29: warning: dereference of noderef expression
kernel/sched/topology.c:1348:17: warning: dereference of noderef expression
kernel/sched/topology.c:1349:29: warning: dereference of noderef expression
kernel/sched/topology.c:1350:17: warning: dereference of noderef expression
... it's just not usable in that form for a regular maintenance flow.
So what would be more useful is to add a specific Sparse check that only checks
KERNEL_DS, to add it as a regular (.config driven) build option and make sure the
kernel build has zero warnings.
From that point on we can declare that this kind of bug won't occur anymore, if
the Sparse implementation of the check is correct.
But there's a (big) problem with that development model: Sparse is not part of the
kernel tree and adding a feature to it while making the kernel depend on that
brand new feature is a logistical nightmare. The overhead is quite similar to
adding new features to a compiler - it happens at a glacial pace and is only done
for major features really, at considerable expense. I don't think this is an
adequate model for 'extended syntax checking' of the kernel, especially when it
comes to correctness that has such obvious security impact.
Thanks,
Ingo
[toc] | [prev] | [next] | [standalone]
| From | Daniel Micay <danielmicay@gmail.com> |
|---|---|
| Date | 2017-05-08 17:30 +0200 |
| Subject | Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode |
| Message-ID | <tESLN-5F6-35@gated-at.bofh.it> |
| In reply to | #1637285 |
On Mon, 2017-05-08 at 09:52 +0200, Ingo Molnar wrote: > > ... it's just not usable in that form for a regular maintenance flow. > > So what would be more useful is to add a specific Sparse check that > only checks > KERNEL_DS, to add it as a regular (.config driven) build option and > make sure the > kernel build has zero warnings. > > From that point on we can declare that this kind of bug won't occur > anymore, if > the Sparse implementation of the check is correct. > > But there's a (big) problem with that development model: Sparse is not > part of the > kernel tree and adding a feature to it while making the kernel depend > on that > brand new feature is a logistical nightmare. The overhead is quite > similar to > adding new features to a compiler - it happens at a glacial pace and > is only done > for major features really, at considerable expense. I don't think this > is an > adequate model for 'extended syntax checking' of the kernel, > especially when it > comes to correctness that has such obvious security impact. > > Thanks, > > Ingo There's the option of using GCC plugins now that the infrastructure was upstreamed from grsecurity. It can be used as part of the regular build process and as long as the analysis is pretty simple it shouldn't hurt compile time much. The problem with doing that is I don't think there are people with much experience with GCC contributing upstream and it's going to be more work to develop/maintain than some kind of specialized DSL for analysis. I think a few static analysis plugins are used as part of maintaining grsecurity for solving issues like finding false positives for the REFCOUNT overflow checking feature, so it's something that's already being done in practice elsewhere.
[toc] | [prev] | [next] | [standalone]
| From | Kees Cook <keescook@chromium.org> |
|---|---|
| Date | 2017-05-08 17:30 +0200 |
| Subject | Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode |
| Message-ID | <tESLN-5F6-43@gated-at.bofh.it> |
| In reply to | #1637515 |
On Mon, May 8, 2017 at 8:22 AM, Daniel Micay <danielmicay@gmail.com> wrote: > On Mon, 2017-05-08 at 09:52 +0200, Ingo Molnar wrote: >> >> ... it's just not usable in that form for a regular maintenance flow. >> >> So what would be more useful is to add a specific Sparse check that >> only checks >> KERNEL_DS, to add it as a regular (.config driven) build option and >> make sure the >> kernel build has zero warnings. >> >> From that point on we can declare that this kind of bug won't occur >> anymore, if >> the Sparse implementation of the check is correct. >> >> But there's a (big) problem with that development model: Sparse is not >> part of the >> kernel tree and adding a feature to it while making the kernel depend >> on that >> brand new feature is a logistical nightmare. The overhead is quite >> similar to >> adding new features to a compiler - it happens at a glacial pace and >> is only done >> for major features really, at considerable expense. I don't think this >> is an >> adequate model for 'extended syntax checking' of the kernel, >> especially when it >> comes to correctness that has such obvious security impact. >> >> Thanks, >> >> Ingo > > There's the option of using GCC plugins now that the infrastructure was > upstreamed from grsecurity. It can be used as part of the regular build > process and as long as the analysis is pretty simple it shouldn't hurt > compile time much. Well, and that the situation may arise due to memory corruption, not from poorly-matched set_fs() calls, which static analysis won't help solve. We need to catch this bad kernel state because it is a very bad state to run in. -Kees -- Kees Cook Pixel Security
[toc] | [prev] | [next] | [standalone]
| From | Thomas Garnier <thgarnie@google.com> |
|---|---|
| Date | 2017-05-08 22:00 +0200 |
| Subject | Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode |
| Message-ID | <tEWZ3-8eF-1@gated-at.bofh.it> |
| In reply to | #1637519 |
On Mon, May 8, 2017 at 8:26 AM, Kees Cook <keescook@chromium.org> wrote: > On Mon, May 8, 2017 at 8:22 AM, Daniel Micay <danielmicay@gmail.com> wrote: >> On Mon, 2017-05-08 at 09:52 +0200, Ingo Molnar wrote: >>> >>> ... it's just not usable in that form for a regular maintenance flow. >>> >>> So what would be more useful is to add a specific Sparse check that >>> only checks >>> KERNEL_DS, to add it as a regular (.config driven) build option and >>> make sure the >>> kernel build has zero warnings. >>> >>> From that point on we can declare that this kind of bug won't occur >>> anymore, if >>> the Sparse implementation of the check is correct. >>> >>> But there's a (big) problem with that development model: Sparse is not >>> part of the >>> kernel tree and adding a feature to it while making the kernel depend >>> on that >>> brand new feature is a logistical nightmare. The overhead is quite >>> similar to >>> adding new features to a compiler - it happens at a glacial pace and >>> is only done >>> for major features really, at considerable expense. I don't think this >>> is an >>> adequate model for 'extended syntax checking' of the kernel, >>> especially when it >>> comes to correctness that has such obvious security impact. >>> >>> Thanks, >>> >>> Ingo >> >> There's the option of using GCC plugins now that the infrastructure was >> upstreamed from grsecurity. It can be used as part of the regular build >> process and as long as the analysis is pretty simple it shouldn't hurt >> compile time much. > > Well, and that the situation may arise due to memory corruption, not > from poorly-matched set_fs() calls, which static analysis won't help > solve. We need to catch this bad kernel state because it is a very bad > state to run in. Of course, I agree with Kees points on this and previous emails. A static analysis solution is hard to scale across functions and build time can suffer. I don't think the coverage will be good enough to consider this change and static analysis as similar. > > -Kees > > -- > Kees Cook > Pixel Security -- Thomas
[toc] | [prev] | [next] | [standalone]
| From | Ingo Molnar <mingo@kernel.org> |
|---|---|
| Date | 2017-05-09 09:00 +0200 |
| Subject | Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode |
| Message-ID | <tF7hM-6w8-15@gated-at.bofh.it> |
| In reply to | #1637519 |
* Kees Cook <keescook@chromium.org> wrote: > > There's the option of using GCC plugins now that the infrastructure was > > upstreamed from grsecurity. It can be used as part of the regular build > > process and as long as the analysis is pretty simple it shouldn't hurt compile > > time much. > > Well, and that the situation may arise due to memory corruption, not from > poorly-matched set_fs() calls, which static analysis won't help solve. We need > to catch this bad kernel state because it is a very bad state to run in. If memory corruption corrupted the task state into having addr_limit set to KERNEL_DS then there's already a fair chance that it's game over: it could also have set *uid to 0, or changed a sensitive PF_ flag, or a number of other things... Furthermore, think about it: there's literally an infinite amount of corrupted task states that could be a security problem and that could be checked after every system call. Do we want to check every one of them? Thanks, Ingo
[toc] | [prev] | [next] | [standalone]
| From | Greg KH <greg@kroah.com> |
|---|---|
| Date | 2017-05-09 13:20 +0200 |
| Subject | Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode |
| Message-ID | <tFblo-WU-3@gated-at.bofh.it> |
| In reply to | #1637906 |
On Tue, May 09, 2017 at 08:56:19AM +0200, Ingo Molnar wrote:
>
> * Kees Cook <keescook@chromium.org> wrote:
>
> > > There's the option of using GCC plugins now that the infrastructure was
> > > upstreamed from grsecurity. It can be used as part of the regular build
> > > process and as long as the analysis is pretty simple it shouldn't hurt compile
> > > time much.
> >
> > Well, and that the situation may arise due to memory corruption, not from
> > poorly-matched set_fs() calls, which static analysis won't help solve. We need
> > to catch this bad kernel state because it is a very bad state to run in.
>
> If memory corruption corrupted the task state into having addr_limit set to
> KERNEL_DS then there's already a fair chance that it's game over: it could also
> have set *uid to 0, or changed a sensitive PF_ flag, or a number of other
> things...
>
> Furthermore, think about it: there's literally an infinite amount of corrupted
> task states that could be a security problem and that could be checked after every
> system call. Do we want to check every one of them?
Ok, I'm all for not checking lots of stuff all the time, just to protect
from crappy drivers that. Especially as we _can_ audit and run checks
on the source code for them in the kernel tree.
But, and here's the problem, outside of the desktop/enterprise world,
there are a ton of out-of-tree code that is crap. The number of
security/bug fixes and kernel crashes for out-of-tree code in systems
like Android phones is just so high it's laughable.
When you have a device that is running 3.2 million lines of kernel code,
yet the diffstat of the tree compared to mainline adds 3 million lines
of code, there is bound to be a ton of issues/problems there.
So this is an entirely different thing we need to try to protect
ourselves from. A long time ago I laughed when I saw that Microsoft had
to do lots of "hardening" of their kernel to protect themselves from
crappy drivers, as I knew we didn't have to do that because we had the
source for them and could fix the root issues. But that has changed and
now we don't all have that option. That code is out-of-tree because the
vendor doesn't care, and doesn't want to take any time at all to do
anything resembling a real code review[1].
So, how about options like the ones being proposed here, go behind a new
config option:
CONFIG_PROTECT_FROM_CRAPPY_DRIVERS
that device owners can enable if they do not trust their vendor-provided
code (hint, I sure don't.) That way the "normal" path that all of us
are used to running will be fine, but if you want to take the speed hit
to try to protect yourself, then you can do that as well.
Anyway, just an idea...
thanks,
greg k-h
[1] I am working really hard with lots of vendors to try to fix their
broken development model, but that is going to take years to resolve
as their device pipelines are years long, and changing their
mindsets takes a long time...
[toc] | [prev] | [next] | [standalone]
| From | Thomas Garnier <thgarnie@google.com> |
|---|---|
| Date | 2017-05-09 16:40 +0200 |
| Subject | Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode |
| Message-ID | <tFesW-2Ya-21@gated-at.bofh.it> |
| In reply to | #1638049 |
On Tue, May 9, 2017 at 4:10 AM, Greg KH <greg@kroah.com> wrote: > On Tue, May 09, 2017 at 08:56:19AM +0200, Ingo Molnar wrote: >> >> * Kees Cook <keescook@chromium.org> wrote: >> >> > > There's the option of using GCC plugins now that the infrastructure was >> > > upstreamed from grsecurity. It can be used as part of the regular build >> > > process and as long as the analysis is pretty simple it shouldn't hurt compile >> > > time much. >> > >> > Well, and that the situation may arise due to memory corruption, not from >> > poorly-matched set_fs() calls, which static analysis won't help solve. We need >> > to catch this bad kernel state because it is a very bad state to run in. >> >> If memory corruption corrupted the task state into having addr_limit set to >> KERNEL_DS then there's already a fair chance that it's game over: it could also >> have set *uid to 0, or changed a sensitive PF_ flag, or a number of other >> things... >> >> Furthermore, think about it: there's literally an infinite amount of corrupted >> task states that could be a security problem and that could be checked after every >> system call. Do we want to check every one of them? > > Ok, I'm all for not checking lots of stuff all the time, just to protect > from crappy drivers that. Especially as we _can_ audit and run checks > on the source code for them in the kernel tree. > > But, and here's the problem, outside of the desktop/enterprise world, > there are a ton of out-of-tree code that is crap. The number of > security/bug fixes and kernel crashes for out-of-tree code in systems > like Android phones is just so high it's laughable. > > When you have a device that is running 3.2 million lines of kernel code, > yet the diffstat of the tree compared to mainline adds 3 million lines > of code, there is bound to be a ton of issues/problems there. > > So this is an entirely different thing we need to try to protect > ourselves from. A long time ago I laughed when I saw that Microsoft had > to do lots of "hardening" of their kernel to protect themselves from > crappy drivers, as I knew we didn't have to do that because we had the > source for them and could fix the root issues. But that has changed and > now we don't all have that option. That code is out-of-tree because the > vendor doesn't care, and doesn't want to take any time at all to do > anything resembling a real code review[1]. That's a big part of why I thought would be useful. I am less worried about edge cases upstream right now than forks with custom codes not using set_fs correctly. > > So, how about options like the ones being proposed here, go behind a new > config option: > CONFIG_PROTECT_FROM_CRAPPY_DRIVERS > that device owners can enable if they do not trust their vendor-provided > code (hint, I sure don't.) That way the "normal" path that all of us > are used to running will be fine, but if you want to take the speed hit > to try to protect yourself, then you can do that as well. Maybe another name but why not. > > Anyway, just an idea... > > thanks, > > greg k-h > > [1] I am working really hard with lots of vendors to try to fix their > broken development model, but that is going to take years to resolve > as their device pipelines are years long, and changing their > mindsets takes a long time... -- Thomas
[toc] | [prev] | [next] | [standalone]
| From | Thomas Garnier <thgarnie@google.com> |
|---|---|
| Date | 2017-05-12 01:20 +0200 |
| Subject | Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode |
| Message-ID | <tG5xg-4mn-5@gated-at.bofh.it> |
| In reply to | #1638166 |
On Tue, May 9, 2017 at 7:29 AM, Thomas Garnier <thgarnie@google.com> wrote: > > On Tue, May 9, 2017 at 4:10 AM, Greg KH <greg@kroah.com> wrote: > > On Tue, May 09, 2017 at 08:56:19AM +0200, Ingo Molnar wrote: > >> > >> * Kees Cook <keescook@chromium.org> wrote: > >> > >> > > There's the option of using GCC plugins now that the infrastructure was > >> > > upstreamed from grsecurity. It can be used as part of the regular build > >> > > process and as long as the analysis is pretty simple it shouldn't hurt compile > >> > > time much. > >> > > >> > Well, and that the situation may arise due to memory corruption, not from > >> > poorly-matched set_fs() calls, which static analysis won't help solve. We need > >> > to catch this bad kernel state because it is a very bad state to run in. > >> > >> If memory corruption corrupted the task state into having addr_limit set to > >> KERNEL_DS then there's already a fair chance that it's game over: it could also > >> have set *uid to 0, or changed a sensitive PF_ flag, or a number of other > >> things... > >> > >> Furthermore, think about it: there's literally an infinite amount of corrupted > >> task states that could be a security problem and that could be checked after every > >> system call. Do we want to check every one of them? > > > > Ok, I'm all for not checking lots of stuff all the time, just to protect > > from crappy drivers that. Especially as we _can_ audit and run checks > > on the source code for them in the kernel tree. > > > > But, and here's the problem, outside of the desktop/enterprise world, > > there are a ton of out-of-tree code that is crap. The number of > > security/bug fixes and kernel crashes for out-of-tree code in systems > > like Android phones is just so high it's laughable. > > > > When you have a device that is running 3.2 million lines of kernel code, > > yet the diffstat of the tree compared to mainline adds 3 million lines > > of code, there is bound to be a ton of issues/problems there. > > > > So this is an entirely different thing we need to try to protect > > ourselves from. A long time ago I laughed when I saw that Microsoft had > > to do lots of "hardening" of their kernel to protect themselves from > > crappy drivers, as I knew we didn't have to do that because we had the > > source for them and could fix the root issues. But that has changed and > > now we don't all have that option. That code is out-of-tree because the > > vendor doesn't care, and doesn't want to take any time at all to do > > anything resembling a real code review[1]. > > That's a big part of why I thought would be useful. I am less worried > about edge cases upstream right now than forks with custom codes not > using set_fs correctly. > > > > > So, how about options like the ones being proposed here, go behind a new > > config option: > > CONFIG_PROTECT_FROM_CRAPPY_DRIVERS > > that device owners can enable if they do not trust their vendor-provided > > code (hint, I sure don't.) That way the "normal" path that all of us > > are used to running will be fine, but if you want to take the speed hit > > to try to protect yourself, then you can do that as well. > > Maybe another name but why not. Ingo: Do you want the change as-is? Would you like it to be optional? What do you think? > > > > > Anyway, just an idea... > > > > thanks, > > > > greg k-h > > > > [1] I am working really hard with lots of vendors to try to fix their > > broken development model, but that is going to take years to resolve > > as their device pipelines are years long, and changing their > > mindsets takes a long time... > > > > -- > Thomas -- Thomas
[toc] | [prev] | [next] | [standalone]
| From | Linus Torvalds <torvalds@linux-foundation.org> |
|---|---|
| Date | 2017-05-12 01:50 +0200 |
| Subject | Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode |
| Message-ID | <tG60h-4yv-1@gated-at.bofh.it> |
| In reply to | #1640071 |
On Thu, May 11, 2017 at 4:17 PM, Thomas Garnier <thgarnie@google.com> wrote:
>
> Ingo: Do you want the change as-is? Would you like it to be optional?
> What do you think?
I'm not ingo, but I don't like that patch. It's in the wrong place -
that system call return code is too timing-critical to add address
limit checks.
Now what I think you *could* do is:
- make "set_fs()" actually set a work flag in the current thread flags
- do the test in the slow-path (syscall_return_slowpath).
Yes, yes, that ends up being architecture-specific, but it's fairly simple.
And it only slows down the system calls that actually use "set_fs()".
Sure, it will slow those down a fair amount, but they are hopefully a
small subset of all cases.
How does that sound to people? Thats' where we currently do that
if (IS_ENABLED(CONFIG_PROVE_LOCKING) &&
WARN(irqs_disabled(), "syscall %ld left IRQs disabled",
regs->orig_ax))
local_irq_enable();
check too, which is a fairly similar issue.
Linus
[toc] | [prev] | [next] | [standalone]
| From | Martin Schwidefsky <schwidefsky@de.ibm.com> |
|---|---|
| Date | 2017-05-12 07:30 +0200 |
| Subject | Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode |
| Message-ID | <tGbjj-8cj-1@gated-at.bofh.it> |
| In reply to | #1640076 |
On Thu, 11 May 2017 16:44:07 -0700 Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Thu, May 11, 2017 at 4:17 PM, Thomas Garnier <thgarnie@google.com> wrote: > > > > Ingo: Do you want the change as-is? Would you like it to be optional? > > What do you think? > > I'm not ingo, but I don't like that patch. It's in the wrong place - > that system call return code is too timing-critical to add address > limit checks. > > Now what I think you *could* do is: > > - make "set_fs()" actually set a work flag in the current thread flags > > - do the test in the slow-path (syscall_return_slowpath). > > Yes, yes, that ends up being architecture-specific, but it's fairly simple. > > And it only slows down the system calls that actually use "set_fs()". > Sure, it will slow those down a fair amount, but they are hopefully a > small subset of all cases. > > How does that sound to people? Thats' where we currently do that > > if (IS_ENABLED(CONFIG_PROVE_LOCKING) && > WARN(irqs_disabled(), "syscall %ld left IRQs disabled", > regs->orig_ax)) > local_irq_enable(); > > check too, which is a fairly similar issue. This is exactly what Heiko did for the s390 backend as a result of this discussion. See the _CIF_ASCE_SECONDARY bit in arch/s390/kernel/entry.S, for the hot patch the check for the bit is included in the general _CIF_WORK test. Only the slow patch gets a bit slower. git commit b5a882fcf146c87cb6b67c6df353e1c042b8773d "s390: restore address space when returning to user space". -- blue skies, Martin. "Reality continues to ruin my life." - Calvin.
[toc] | [prev] | [next] | [standalone]
| From | Kees Cook <keescook@chromium.org> |
|---|---|
| Date | 2017-05-12 07:40 +0200 |
| Subject | Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode |
| Message-ID | <tGbsZ-8fz-1@gated-at.bofh.it> |
| In reply to | #1640205 |
On Thu, May 11, 2017 at 10:28 PM, Martin Schwidefsky <schwidefsky@de.ibm.com> wrote: > On Thu, 11 May 2017 16:44:07 -0700 > Linus Torvalds <torvalds@linux-foundation.org> wrote: > >> On Thu, May 11, 2017 at 4:17 PM, Thomas Garnier <thgarnie@google.com> wrote: >> > >> > Ingo: Do you want the change as-is? Would you like it to be optional? >> > What do you think? >> >> I'm not ingo, but I don't like that patch. It's in the wrong place - >> that system call return code is too timing-critical to add address >> limit checks. >> >> Now what I think you *could* do is: >> >> - make "set_fs()" actually set a work flag in the current thread flags >> >> - do the test in the slow-path (syscall_return_slowpath). >> >> Yes, yes, that ends up being architecture-specific, but it's fairly simple. >> >> And it only slows down the system calls that actually use "set_fs()". >> Sure, it will slow those down a fair amount, but they are hopefully a >> small subset of all cases. >> >> How does that sound to people? Thats' where we currently do that >> >> if (IS_ENABLED(CONFIG_PROVE_LOCKING) && >> WARN(irqs_disabled(), "syscall %ld left IRQs disabled", >> regs->orig_ax)) >> local_irq_enable(); >> >> check too, which is a fairly similar issue. > > This is exactly what Heiko did for the s390 backend as a result of this > discussion. See the _CIF_ASCE_SECONDARY bit in arch/s390/kernel/entry.S, > for the hot patch the check for the bit is included in the general > _CIF_WORK test. Only the slow patch gets a bit slower. > > git commit b5a882fcf146c87cb6b67c6df353e1c042b8773d > "s390: restore address space when returning to user space". If I'm understanding this, it won't catch corruption of addr_limit during fast-path syscalls, though (i.e. addr_limit changed without a call to set_fs()). :( This addr_limit corruption is mostly only a risk archs without THREAD_INFO_IN_TASK, but it would still be nice to catch unbalanced set_fs() code, so I like the idea. I like getting rid of addr_limit entirely even more, but that'll take some time. :) -Kees -- Kees Cook Pixel Security
[toc] | [prev] | [next] | [standalone]
| From | Martin Schwidefsky <schwidefsky@de.ibm.com> |
|---|---|
| Date | 2017-05-12 08:00 +0200 |
| Subject | Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode |
| Message-ID | <tGbMm-8mq-3@gated-at.bofh.it> |
| In reply to | #1640207 |
On Thu, 11 May 2017 22:34:31 -0700 Kees Cook <keescook@chromium.org> wrote: > On Thu, May 11, 2017 at 10:28 PM, Martin Schwidefsky > <schwidefsky@de.ibm.com> wrote: > > On Thu, 11 May 2017 16:44:07 -0700 > > Linus Torvalds <torvalds@linux-foundation.org> wrote: > > > >> On Thu, May 11, 2017 at 4:17 PM, Thomas Garnier <thgarnie@google.com> wrote: > >> > > >> > Ingo: Do you want the change as-is? Would you like it to be optional? > >> > What do you think? > >> > >> I'm not ingo, but I don't like that patch. It's in the wrong place - > >> that system call return code is too timing-critical to add address > >> limit checks. > >> > >> Now what I think you *could* do is: > >> > >> - make "set_fs()" actually set a work flag in the current thread flags > >> > >> - do the test in the slow-path (syscall_return_slowpath). > >> > >> Yes, yes, that ends up being architecture-specific, but it's fairly simple. > >> > >> And it only slows down the system calls that actually use "set_fs()". > >> Sure, it will slow those down a fair amount, but they are hopefully a > >> small subset of all cases. > >> > >> How does that sound to people? Thats' where we currently do that > >> > >> if (IS_ENABLED(CONFIG_PROVE_LOCKING) && > >> WARN(irqs_disabled(), "syscall %ld left IRQs disabled", > >> regs->orig_ax)) > >> local_irq_enable(); > >> > >> check too, which is a fairly similar issue. > > > > This is exactly what Heiko did for the s390 backend as a result of this > > discussion. See the _CIF_ASCE_SECONDARY bit in arch/s390/kernel/entry.S, > > for the hot patch the check for the bit is included in the general > > _CIF_WORK test. Only the slow patch gets a bit slower. > > > > git commit b5a882fcf146c87cb6b67c6df353e1c042b8773d > > "s390: restore address space when returning to user space". > > If I'm understanding this, it won't catch corruption of addr_limit > during fast-path syscalls, though (i.e. addr_limit changed without a > call to set_fs()). :( This addr_limit corruption is mostly only a risk > archs without THREAD_INFO_IN_TASK, but it would still be nice to catch > unbalanced set_fs() code, so I like the idea. I like getting rid of > addr_limit entirely even more, but that'll take some time. :) Well for s390 there is no addr_limit as we use two separate address space for kernel vs. user. The equivalent to the addr_limit corruption on a fast-path syscall would be changing CR7 outside of set_fs. This boils down to the question what we are protection against? Bad code with unbalanced set_fs or evil code that changes addr_limit/CR7 outside of set_fs -- blue skies, Martin. "Reality continues to ruin my life." - Calvin.
[toc] | [prev] | [next] | [standalone]
| From | Linus Torvalds <torvalds@linux-foundation.org> |
|---|---|
| Date | 2017-05-12 21:10 +0200 |
| Subject | Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode |
| Message-ID | <tGo6R-EW-3@gated-at.bofh.it> |
| In reply to | #1640213 |
On Fri, May 12, 2017 at 12:01 PM, Kees Cook <keescook@chromium.org> wrote:
>
> Yeah, the risk for "corrupted addr_limit" is mainly a concern for
> archs with addr_limit on the kernel stack. If I'm reading things
> correctly, that means, from the archs I've been paying closer
> attention to, it's an issue for arm, mips, and powerpc:
I don't understand why people are looking at addr_limit as some kind
of special thing.
If somebody is smashing the stack and corrupting thread info data, the
game is over. addr_limit is the *least* of your problems, and it's not
even all that likely that it will be increasing (it's much more likely
that it would be overwritten with a smaller value).
Quite frankly, this kind of idiotic discussion just makes me question
the whole idea of the patch.
Any "security" that is this specific is not real security, it's just
masturbatory garbage.
It may be worth checking that people use "set_fs()" properly. But stop
this idiotic crap. It just makes the kernel security people look like
the crazies.
There are enough incompetent crazy security people, don't go there.
The kinds of things it is worth protecting against are the big class
of generic issues, not the kind of "oh, but imagine if a cosmic ray
flips this particular word in memory" kind of crap that ignores all
the other words of memory.
Seriously, Kees. You are just making security people look bad. Stop it.
Linus
[toc] | [prev] | [next] | [standalone]
| From | Kees Cook <keescook@chromium.org> |
|---|---|
| Date | 2017-05-12 21:40 +0200 |
| Subject | Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode |
| Message-ID | <tGozU-Ud-5@gated-at.bofh.it> |
| In reply to | #1640698 |
On Fri, May 12, 2017 at 12:08 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Fri, May 12, 2017 at 12:01 PM, Kees Cook <keescook@chromium.org> wrote: >> Yeah, the risk for "corrupted addr_limit" is mainly a concern for >> archs with addr_limit on the kernel stack. If I'm reading things >> correctly, that means, from the archs I've been paying closer >> attention to, it's an issue for arm, mips, and powerpc: > > I don't understand why people are looking at addr_limit as some kind > of special thing. > > If somebody is smashing the stack and corrupting thread info data, the > game is over. addr_limit is the *least* of your problems, and it's not > even all that likely that it will be increasing (it's much more likely > that it would be overwritten with a smaller value). > > Quite frankly, this kind of idiotic discussion just makes me question > the whole idea of the patch. > > Any "security" that is this specific is not real security, it's just > masturbatory garbage. > > It may be worth checking that people use "set_fs()" properly. But stop > this idiotic crap. It just makes the kernel security people look like > the crazies. > > There are enough incompetent crazy security people, don't go there. > The kinds of things it is worth protecting against are the big class > of generic issues, not the kind of "oh, but imagine if a cosmic ray > flips this particular word in memory" kind of crap that ignores all > the other words of memory. > > Seriously, Kees. You are just making security people look bad. Stop it. I'm clearly not explaining things well enough. I shouldn't say "corruption", I should say "malicious manipulation". The methodology of attacks against the stack are quite different from the other kinds of attacks like use-after-free, heap overflow, etc. Being able to exhaust the kernel stack (either due to deep recursion or unbounded alloca()) means attackers can control a write to addr_limit, and then leverage that into an actual arbitrary write via subsequent calls to copy_to_user() pointed at kernel memory. This isn't theoretical, this is how those attacks are performed. It may sound crazy, but it's real. With thread_info off the stack, the whole problem goes away. It's wonderful that this has happened for x86, arm64, and s390. There are always going to be new methods of attack for everything, but while we slowly address the design weakness (set_fs()), we can fix the low hanging fruit too. -Kees -- Kees Cook Pixel Security
[toc] | [prev] | [next] | [standalone]
| From | Russell King - ARM Linux <linux@armlinux.org.uk> |
|---|---|
| Date | 2017-05-12 22:30 +0200 |
| Subject | Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode |
| Message-ID | <tGpmi-1v8-9@gated-at.bofh.it> |
| In reply to | #1640708 |
On Fri, May 12, 2017 at 12:30:02PM -0700, Kees Cook wrote: > I'm clearly not explaining things well enough. I shouldn't say > "corruption", I should say "malicious manipulation". The methodology > of attacks against the stack are quite different from the other kinds > of attacks like use-after-free, heap overflow, etc. Being able to > exhaust the kernel stack (either due to deep recursion or unbounded > alloca()) I really hope we don't have alloca() use in the kernel. Do you have evidence to support that assertion? IMHO alloca() (or similar) should not be present in any kernel code because we have a limited stack - we have kmalloc() etc for that kind of thing. -- 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.
[toc] | [prev] | [next] | [standalone]
| From | Peter Zijlstra <peterz@infradead.org> |
|---|---|
| Date | 2017-05-12 22:40 +0200 |
| Subject | Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode |
| Message-ID | <tGpvY-1zo-15@gated-at.bofh.it> |
| In reply to | #1640731 |
On Fri, May 12, 2017 at 09:21:06PM +0100, Russell King - ARM Linux wrote: > On Fri, May 12, 2017 at 12:30:02PM -0700, Kees Cook wrote: > > I'm clearly not explaining things well enough. I shouldn't say > > "corruption", I should say "malicious manipulation". The methodology > > of attacks against the stack are quite different from the other kinds > > of attacks like use-after-free, heap overflow, etc. Being able to > > exhaust the kernel stack (either due to deep recursion or unbounded > > alloca()) > > I really hope we don't have alloca() use in the kernel. Do you have > evidence to support that assertion? > > IMHO alloca() (or similar) should not be present in any kernel code > because we have a limited stack - we have kmalloc() etc for that kind > of thing. On stack variable length arrays get implemented by the compiler doing alloca(), and we sadly have a few of those around. But yes, fully agreed on the desirability of alloca() and things.
[toc] | [prev] | [next] | [standalone]
| From | Russell King - ARM Linux <linux@armlinux.org.uk> |
|---|---|
| Date | 2017-05-12 22:50 +0200 |
| Subject | Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode |
| Message-ID | <tGpFD-1DN-9@gated-at.bofh.it> |
| In reply to | #1640738 |
On Fri, May 12, 2017 at 10:30:44PM +0200, Peter Zijlstra wrote: > On Fri, May 12, 2017 at 09:21:06PM +0100, Russell King - ARM Linux wrote: > > On Fri, May 12, 2017 at 12:30:02PM -0700, Kees Cook wrote: > > > I'm clearly not explaining things well enough. I shouldn't say > > > "corruption", I should say "malicious manipulation". The methodology > > > of attacks against the stack are quite different from the other kinds > > > of attacks like use-after-free, heap overflow, etc. Being able to > > > exhaust the kernel stack (either due to deep recursion or unbounded > > > alloca()) > > > > I really hope we don't have alloca() use in the kernel. Do you have > > evidence to support that assertion? > > > > IMHO alloca() (or similar) should not be present in any kernel code > > because we have a limited stack - we have kmalloc() etc for that kind > > of thing. > > On stack variable length arrays get implemented by the compiler doing > alloca(), and we sadly have a few of those around. I hope their size is appropriately limited, but something tells me it would be foolish to assume that. > But yes, fully agreed on the desirability of alloca() and things. Hmm, I wonder if -fno-builtin-alloca would prevent those... it looks like it certainly would prevent an explicit alloca() call. -- 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.
[toc] | [prev] | [next] | [standalone]
Page 1 of 5 [1] 2 3 4 5 Next page →
Back to top | Article view | linux.kernel
csiph-web