Re: [PATCH 2/2] mm/usercopy: enable usercopy size checking for modern versions of gcc

[Linus Torvalds <torvalds@linux-foundation.org>]

[-- Attachment #1: Type: text/plain, Size: 2408 bytes --]

On Fri, Aug 26, 2016 at 1:56 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>
> There's one problem with that though.  It's going to annoy a lot of
> people who do allyesconfig/allmodconfig builds because
> DEBUG_STRICT_USER_COPY_CHECKS adds several fake warnings.

How bad is it?

In particular, we've definitely had issues with the "warning"
attribute before. Because as you pointed out somewhere elsewhere, the
warrning can happen before the call is actually optimized away by a
later compiler phase.

In particular, we _have_ occasionally fixed this by turning it into a
link-time error instead (that's in fact the really traditional model).
That makes the errior happen much later, and the error message isn't
nearly as nice (you get something like "undefined reference to unknown
symbol '__copy_to_user_failed' in function xyz" without line numbers
etc nice things). But it cuts down on the false positives that come
from warnings triggering before the final code has actually been
generated.

So one option *might* be to make the copy_to_user checks do an
explicitly constant and static check like


     if (__builtin_constant_p(n) && sz >= 0 && n > sz)
          __copy_to_user_failed();

with the "__copy_to_user_failed()" function declared but never
defined. That way, at link time, if something still references it, you
get a link error and you'll know it's bad.

So something like the attached patch *might* work. As mentioned, it
makes the error messages much less legible if they happen, and it
delays them to link time, so it's not perfect. But it certainly has
the potential of avoiding bogus warnings.

It *seemed* to work in my quick allmodconfig build test, but that may
be because I screwed something up. So take that with a large pinch of
salt.

What do people think? The static built-time errors - if they happen -
really should be pretty exceptional and unusual. So maybe it's ok that
they then would be somewhat cryptic, and you'd have to maybe hunt
(possibly through several layers of inline functions) where the actual
offending user copy then ends up being..

So I'm not happy with this patch, but I also think that the false
positives make the *current* code simply unworkable with current gcc
versions.

Of course, somebody might be able to come up with a better approach
that still gets the nice error messages and avoids the false
positives.

                          Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/plain, Size: 4891 bytes --]

 arch/x86/include/asm/uaccess.h | 99 ++++++++++++------------------------------
 include/linux/compiler-gcc.h   |  2 +-
 2 files changed, 28 insertions(+), 73 deletions(-)

diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index a0ae610b9280..f1fe3740b46b 100644
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -697,105 +697,60 @@ unsigned long __must_check _copy_from_user(void *to, const void __user *from,
 unsigned long __must_check _copy_to_user(void __user *to, const void *from,
 					 unsigned n);
 
-#ifdef CONFIG_DEBUG_STRICT_USER_COPY_CHECKS
-# define copy_user_diag __compiletime_error
-#else
-# define copy_user_diag __compiletime_warning
-#endif
-
-extern void copy_user_diag("copy_from_user() buffer size is too small")
-copy_from_user_overflow(void);
-extern void copy_user_diag("copy_to_user() buffer size is too small")
-copy_to_user_overflow(void) __asm__("copy_from_user_overflow");
-
-#undef copy_user_diag
-
-#ifdef CONFIG_DEBUG_STRICT_USER_COPY_CHECKS
-
-extern void
-__compiletime_warning("copy_from_user() buffer size is not provably correct")
-__copy_from_user_overflow(void) __asm__("copy_from_user_overflow");
-#define __copy_from_user_overflow(size, count) __copy_from_user_overflow()
-
-extern void
-__compiletime_warning("copy_to_user() buffer size is not provably correct")
-__copy_to_user_overflow(void) __asm__("copy_from_user_overflow");
-#define __copy_to_user_overflow(size, count) __copy_to_user_overflow()
-
-#else
-
 static inline void
-__copy_from_user_overflow(int size, unsigned long count)
+copy_user_overflow(int size, unsigned long count)
 {
 	WARN(1, "Buffer overflow detected (%d < %lu)!\n", size, count);
 }
 
-#define __copy_to_user_overflow __copy_from_user_overflow
-
-#endif
+extern unsigned long copy_user_bad(void);
+extern unsigned long copy_user_bad(void);
 
 static inline unsigned long __must_check
 copy_from_user(void *to, const void __user *from, unsigned long n)
 {
 	int sz = __compiletime_object_size(to);
 
+	if (sz >= 0 && n > sz) {
+		if (__builtin_constant_p(n))
+			return copy_user_bad();
+#ifdef CONFIG_DEBUG_STRICT_USER_COPY_CHECKS
+		copy_user_overflow(sz, n);
+		memset(to, 0, n);
+		return n;
+#endif
+	}
+
 	might_fault();
 
 	kasan_check_write(to, n);
 
-	/*
-	 * While we would like to have the compiler do the checking for us
-	 * even in the non-constant size case, any false positives there are
-	 * a problem (especially when DEBUG_STRICT_USER_COPY_CHECKS, but even
-	 * without - the [hopefully] dangerous looking nature of the warning
-	 * would make people go look at the respecitive call sites over and
-	 * over again just to find that there's no problem).
-	 *
-	 * And there are cases where it's just not realistic for the compiler
-	 * to prove the count to be in range. For example when multiple call
-	 * sites of a helper function - perhaps in different source files -
-	 * all doing proper range checking, yet the helper function not doing
-	 * so again.
-	 *
-	 * Therefore limit the compile time checking to the constant size
-	 * case, and do only runtime checking for non-constant sizes.
-	 */
-
-	if (likely(sz < 0 || sz >= n)) {
-		check_object_size(to, n, false);
-		n = _copy_from_user(to, from, n);
-	} else if (__builtin_constant_p(n))
-		copy_from_user_overflow();
-	else
-		__copy_from_user_overflow(sz, n);
-
-	return n;
+	check_object_size(to, n, false);
+	return copy_from_user(to, from, n);
 }
 
 static inline unsigned long __must_check
 copy_to_user(void __user *to, const void *from, unsigned long n)
 {
-	int sz = __compiletime_object_size(from);
+	int sz = __compiletime_object_size(to);
+
+	if (sz >= 0 && n > sz) {
+		if (__builtin_constant_p(n))
+			return copy_user_bad();
+#ifdef CONFIG_DEBUG_STRICT_USER_COPY_CHECKS
+		copy_user_overflow(sz, n);
+		return n;
+#endif
+	}
 
 	kasan_check_read(from, n);
 
 	might_fault();
 
-	/* See the comment in copy_from_user() above. */
-	if (likely(sz < 0 || sz >= n)) {
-		check_object_size(from, n, true);
-		n = _copy_to_user(to, from, n);
-	} else if (__builtin_constant_p(n))
-		copy_to_user_overflow();
-	else
-		__copy_to_user_overflow(sz, n);
-
-	return n;
+	check_object_size(from, n, true);
+	return _copy_to_user(to, from, n);
 }
 
-#undef __copy_from_user_overflow
-#undef __copy_to_user_overflow
-
 /*
  * We rely on the nested NMI work to allow atomic faults from the NMI path; the
  * nested NMI paths are careful to preserve CR2.
diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
index e2949397c19b..e7f7a689ef09 100644
--- a/include/linux/compiler-gcc.h
+++ b/include/linux/compiler-gcc.h
@@ -158,7 +158,7 @@
 #define __compiler_offsetof(a, b)					\
 	__builtin_offsetof(a, b)
 
-#if GCC_VERSION >= 40100 && GCC_VERSION < 40600
+#if GCC_VERSION >= 40100
 # define __compiletime_object_size(obj) __builtin_object_size(obj, 0)
 #endif