Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1632979 > unrolled thread

[PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode

Back to article view | Back to linux.kernel

Contents

  [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 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: [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 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 3  [1] 2 3  Next page →

#1632979 — [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode

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]

#1636763

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]

#1637273 — Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode

(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]

#1637285 — Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode

* 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]

#1637515 — Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode

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]

#1637519 — Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode

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]

#1637680 — Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode

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]

#1637906 — Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode

* 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]

#1638049 — Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode

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]

#1638166 — Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode

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]

#1638276 — Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode

On Mon, May 8, 2017 at 11:56 PM, Ingo Molnar <mingo@kernel.org> 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.

[attempting some thread-merging]

> Ok, so that's CVE-2010-4258, where an oops with KERNEL_DS set was used to escalate
> privileges, due to the kernel's oops handler not cleaning up the KERNEL_DS. The
> exploit used another bug, a crash in a network protocol handler, to execute the
> oops handler with KERNEL_DS set.

Right, I didn't mean to suggest that vulnerability would be fixed by
this solution. I was trying to show how there can be some pretty
complex interaction with exceptions/interrupts/etc that would make
pure static analysis still miss things.

> 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?

Right, but this "slippery slope" argument isn't the best way to reject
security changes. Let me take a step back and describe the threat, and
where we should likely spend time:

The primary threat with addr_limit getting changed is that a
narrowly-scoped attack (traditionally stack exhaustion or
adjacent-stack large-index writes) could be leveraged into opening the
entire kernel to writes (by allowing all syscalls with a
copy_to_user() call to suddenly be able to write to kernel memory).
So, really, the flaw is having addr_limit at all. Removing set_fs()
should, I think, allow this to become a const (or at least should get
us a lot closer).

The main path to corrupting addr_limit has been via stack corruption.
On architectures with CONFIG_THREAD_INFO_IN_TASK, this risk is greatly
reduced already, but it's not universally available yet. (And as long
as we're talking about stack attacks, CONFIG_VMAP_STACK makes
cross-stack overflows go away, and cross-stack indexing harder, but
that's not really about addr_limit since currently nothing with
VMAP_STACK doesn't already have THREAD_INFO_IN_TASK.)

So, left with a still exploitable target in memory that allows such an
expansion of attack method, I still think it's worth keeping this
patch series, but if we can drop set_fs() I could probably be
convinced the benefit of the series doesn't exceed the cost on
THREAD_INFO_IN_TASK-architectures (x86, arm64, s390). But that means
at least currently keeping it on arm, for example. If we can make
addr_limit const, well, we don't need the series at all.

-Kees

-- 
Kees Cook
Pixel Security

[toc] | [prev] | [next] | [standalone]

#1637428 — Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode

On Mon, May 08, 2017 at 09:33:52AM +0200, Ingo Molnar wrote:
> 
> (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.

What about a simple coccinelle script to test for this type of thing?
We write it once, add it to the in-kernel body of tests, and then 0-day
runs it on all trees all the time.  That should catch this type of
issue, like all of the other "bad programming bus" that the tool
currently catches.

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1637898 — Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode

* Greg KH <greg@kroah.com> wrote:

> What about a simple coccinelle script to test for this type of thing?
> We write it once, add it to the in-kernel body of tests, and then 0-day
> runs it on all trees all the time.  That should catch this type of
> issue, like all of the other "bad programming bus" that the tool
> currently catches.

Yeah, that would work - but today most of our coccinelle scripts are still pretty 
verbose, and I think it's important to make this a different category of 
coccinelle script, which is .config driven where a loud warning yells at us.

I.e. force the 'zero warnings tolerated' model.

I also noticed that Coccinelle builds are pretty slow, so it would still make 
sense to have a performance oriented static checking facility that does not have 
the performance baggage of high level functional languages.

I.e. either integrate it into Sparse - or start a kernel integrated static 
analysis tooling project that would only follow control flow initially - which is 
what we need here I believe.

We only have ~115 code blocks in the kernel that set/restore KERNEL_DS, it would 
be a pity to add a runtime check to every system call ...

We could also add a runtime check to oops handling to make sure we don't leak 
KERNEL_DS through kernel crashes, to ease worries about CVE-2010-4258.

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1637991 — Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode

On Tue, May 09, 2017 at 08:45:22AM +0200, Ingo Molnar wrote:
> We only have ~115 code blocks in the kernel that set/restore KERNEL_DS, it would 
> be a pity to add a runtime check to every system call ...

I think we should simply strive to remove all of them that aren't
in core scheduler / arch code.  Basically evetyytime we do the

	oldfs = get_fs();
	set_fs(KERNEL_DS); 
	..
	set_fs(oldfs);

trick we're doing something wrong, and there should always be better
ways to archive it.  E.g. using iov_iter with a ITER_KVEC type
consistently would already remove most of them.

[toc] | [prev] | [next] | [standalone]

#1638110 — Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode

On Tue, May 9, 2017 at 1:56 AM, Christoph Hellwig <hch@infradead.org> wrote:
> On Tue, May 09, 2017 at 08:45:22AM +0200, Ingo Molnar wrote:
>> We only have ~115 code blocks in the kernel that set/restore KERNEL_DS, it would
>> be a pity to add a runtime check to every system call ...
>
> I think we should simply strive to remove all of them that aren't
> in core scheduler / arch code.  Basically evetyytime we do the
>
>         oldfs = get_fs();
>         set_fs(KERNEL_DS);
>         ..
>         set_fs(oldfs);
>
> trick we're doing something wrong, and there should always be better
> ways to archive it.  E.g. using iov_iter with a ITER_KVEC type
> consistently would already remove most of them.

How about trying to remove all of them?  If we could actually get rid
of all of them, we could drop the arch support, and we'd get faster,
simpler, shorter uaccess code throughout the kernel.

The ones in kernel/compat.c are generally garbage.  They should be
using compat_alloc_user_space().  Ditto for kernel/power/user.c.

flush_module_icache() is a potentially silly arch thing.  Does the
code in kernel/module.c that uses set_fs() actually work?

kernel/signal.c's set_fs() is laziness.

__probe_kernel_read() and __probe_kernel_write() use set_fs(), but
that usage only matters on sane arches* like s390x.  We should
arguably have a set_uaccess_address_space() or similar for this
purpose that's a nop on normal arches like x86.

fs/splice.c has some, ahem, interesting uses that have been the source
of nasty exploits in the past.  Converting them to use iov_iter
properly would be really, really nice.  Christoph, I don't suppose
you'd like to do that?

The others seem to mostly be fixable, but I haven't looked that closely.

Overall, I suspect that a big part of why mitigations like the one
being discussed in this thread were developed is because addr_limit
used to be on the stack, making it (along with restart_block) a really
nice target.  This is fixed now on x86, arm64, and s390x, I believe,
and other arches can easily opt in to the fix.

* I'm strongly in favor of arches that have totally separate user and
kernel address spaces.  Sadly, the most common arches don't do this.

[toc] | [prev] | [next] | [standalone]

#1638114 — Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode

On Tue, May 09, 2017 at 06:00:01AM -0700, Andy Lutomirski wrote:
> fs/splice.c has some, ahem, interesting uses that have been the source
> of nasty exploits in the past.  Converting them to use iov_iter
> properly would be really, really nice.  Christoph, I don't suppose
> you'd like to do that?

I can take care of all the fs code including this one.

[toc] | [prev] | [next] | [standalone]

#1638255 — Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode

On Tue, May 09, 2017 at 06:02:50AM -0700, Christoph Hellwig wrote:
> On Tue, May 09, 2017 at 06:00:01AM -0700, Andy Lutomirski wrote:
> > fs/splice.c has some, ahem, interesting uses that have been the source
> > of nasty exploits in the past.  Converting them to use iov_iter
> > properly would be really, really nice.  Christoph, I don't suppose
> > you'd like to do that?
> 
> I can take care of all the fs code including this one.

I spent the afternoon hacking up where I'd like this to head.  It's
completely untested as of now:

	http://git.infradead.org/users/hch/vfs.git/shortlog/refs/heads/setfs-elimination

[toc] | [prev] | [next] | [standalone]

#1638283 — Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode

On Tue, May 9, 2017 at 9:03 AM, Christoph Hellwig <hch@infradead.org> wrote:
> On Tue, May 09, 2017 at 06:02:50AM -0700, Christoph Hellwig wrote:
>> On Tue, May 09, 2017 at 06:00:01AM -0700, Andy Lutomirski wrote:
>> > fs/splice.c has some, ahem, interesting uses that have been the source
>> > of nasty exploits in the past.  Converting them to use iov_iter
>> > properly would be really, really nice.  Christoph, I don't suppose
>> > you'd like to do that?
>>
>> I can take care of all the fs code including this one.
>
> I spent the afternoon hacking up where I'd like this to head.  It's
> completely untested as of now:
>
>         http://git.infradead.org/users/hch/vfs.git/shortlog/refs/heads/setfs-elimination

Ooooh yes! Nice work.

I love this:
http://git.infradead.org/users/hch/vfs.git/commitdiff/51e83f50f824ca23f5584c172138e6b7c2ff786d
but I wonder what it'll cause out-of-tree code to do. I mean, I'd
rather nothing out-of-tree be calling these, but I'd hate 3rd party
hacks even more.

http://git.infradead.org/users/hch/vfs.git/commitdiff/018e0e9030777121fe87e89d43066691e7366587
This accidentally(?) removes the kernel-doc comments.

http://git.infradead.org/users/hch/vfs.git/commitdiff/78b62c730254fc39fa888cdbdca08fde6e09a798
Could this be made defensive? (Return 0 if ret wraps, for example?) I
see what the comment says, but not everyone will read that. :(

http://git.infradead.org/users/hch/vfs.git/commitdiff/a106276ca0294be054bc89ce97219933fe543df1
Perhaps unconditionally set USER_DS on exit instead of retaining
whatever was there?

-Kees

-- 
Kees Cook
Pixel Security

[toc] | [prev] | [next] | [standalone]

#1638430 — Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode

On Tue, May 9, 2017 at 9:50 AM, Kees Cook <keescook@chromium.org> wrote:
> On Tue, May 9, 2017 at 9:03 AM, Christoph Hellwig <hch@infradead.org> wrote:
>> On Tue, May 09, 2017 at 06:02:50AM -0700, Christoph Hellwig wrote:
>>> On Tue, May 09, 2017 at 06:00:01AM -0700, Andy Lutomirski wrote:
>>> > fs/splice.c has some, ahem, interesting uses that have been the source
>>> > of nasty exploits in the past.  Converting them to use iov_iter
>>> > properly would be really, really nice.  Christoph, I don't suppose
>>> > you'd like to do that?
>>>
>>> I can take care of all the fs code including this one.
>>
>> I spent the afternoon hacking up where I'd like this to head.  It's
>> completely untested as of now:
>>
>>         http://git.infradead.org/users/hch/vfs.git/shortlog/refs/heads/setfs-elimination
>
> Ooooh yes! Nice work.
>
> I love this:
> http://git.infradead.org/users/hch/vfs.git/commitdiff/51e83f50f824ca23f5584c172138e6b7c2ff786d
> but I wonder what it'll cause out-of-tree code to do. I mean, I'd
> rather nothing out-of-tree be calling these, but I'd hate 3rd party
> hacks even more.
>
> http://git.infradead.org/users/hch/vfs.git/commitdiff/018e0e9030777121fe87e89d43066691e7366587
> This accidentally(?) removes the kernel-doc comments.
>
> http://git.infradead.org/users/hch/vfs.git/commitdiff/78b62c730254fc39fa888cdbdca08fde6e09a798
> Could this be made defensive? (Return 0 if ret wraps, for example?) I
> see what the comment says, but not everyone will read that. :(
>
> http://git.infradead.org/users/hch/vfs.git/commitdiff/a106276ca0294be054bc89ce97219933fe543df1
> Perhaps unconditionally set USER_DS on exit instead of retaining
> whatever was there?

I don't like silent fixups.  If we want to do this, we should BUG or
at least WARN, not just change the addr limit.  But I'm also not
convinced it's indicative of an actual bug here.

--Andy

[toc] | [prev] | [next] | [standalone]

#1638451 — Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode

On Tue, May 9, 2017 at 3:52 PM, Andy Lutomirski <luto@amacapital.net> wrote:
> On Tue, May 9, 2017 at 9:50 AM, Kees Cook <keescook@chromium.org> wrote:
>> On Tue, May 9, 2017 at 9:03 AM, Christoph Hellwig <hch@infradead.org> wrote:
>>> On Tue, May 09, 2017 at 06:02:50AM -0700, Christoph Hellwig wrote:
>>>> On Tue, May 09, 2017 at 06:00:01AM -0700, Andy Lutomirski wrote:
>>>> > fs/splice.c has some, ahem, interesting uses that have been the source
>>>> > of nasty exploits in the past.  Converting them to use iov_iter
>>>> > properly would be really, really nice.  Christoph, I don't suppose
>>>> > you'd like to do that?
>>>>
>>>> I can take care of all the fs code including this one.
>>>
>>> I spent the afternoon hacking up where I'd like this to head.  It's
>>> completely untested as of now:
>>>
>>>         http://git.infradead.org/users/hch/vfs.git/shortlog/refs/heads/setfs-elimination
>>
>> Ooooh yes! Nice work.
>>
>> I love this:
>> http://git.infradead.org/users/hch/vfs.git/commitdiff/51e83f50f824ca23f5584c172138e6b7c2ff786d
>> but I wonder what it'll cause out-of-tree code to do. I mean, I'd
>> rather nothing out-of-tree be calling these, but I'd hate 3rd party
>> hacks even more.
>>
>> http://git.infradead.org/users/hch/vfs.git/commitdiff/018e0e9030777121fe87e89d43066691e7366587
>> This accidentally(?) removes the kernel-doc comments.
>>
>> http://git.infradead.org/users/hch/vfs.git/commitdiff/78b62c730254fc39fa888cdbdca08fde6e09a798
>> Could this be made defensive? (Return 0 if ret wraps, for example?) I
>> see what the comment says, but not everyone will read that. :(
>>
>> http://git.infradead.org/users/hch/vfs.git/commitdiff/a106276ca0294be054bc89ce97219933fe543df1
>> Perhaps unconditionally set USER_DS on exit instead of retaining
>> whatever was there?
>
> I don't like silent fixups.  If we want to do this, we should BUG or
> at least WARN, not just change the addr limit.  But I'm also not
> convinced it's indicative of an actual bug here.

Nothing should enter that function with KERNEL_DS set, right?

BUG_ON(get_fs() != USER_DS);
set_fs(KERNEL_DS);
...
set_fs(USER_DS);

-Kees

-- 
Kees Cook
Pixel Security

[toc] | [prev] | [next] | [standalone]

Page 1 of 3  [1] 2 3  Next page →

Back to top | Article view | linux.kernel

csiph-web