[PATCH 1/2] x86: unify copy_from_user() size checking

[PATCH 1/2] x86: unify copy_from_user() size checking

[Jan Beulich]

Commits 4a3127693001c61a21d1ce680db6340623f52e93 ("x86: Turn the
copy_from_user check into an (optional) compile time warning") and
63312b6a6faae3f2e5577f2b001e3b504f10a2aa ("x86: Add a Kconfig option to
turn the copy_from_user warnings into errors") touched only the 32-bit
variant of copy_from_user(), whereas the original commit
9f0cf4adb6aa0bfccf675c938124e68f7f06349d ("x86: Use
__builtin_object_size() to validate the buffer size for
copy_from_user()") also added the same code to the 64-bit one.

Further the earlier conversion from an inline WARN() to the call to
copy_from_user_overflow() went a little too far: When the number of
bytes to be copied is not a constant (e.g. [looking at 3.11] in
drivers/net/tun.c:__tun_chr_ioctl() or
drivers/pci/pcie/aer/aer_inject.c:aer_inject_write()), the compiler
will always have to keep the funtion call, and hence there will always
be a warning. By using __builtin_constant_p() we can avoid this.

And then this slightly extends the effect of
CONFIG_DEBUG_STRICT_USER_COPY_CHECKS in that apart from converting
warnings to errors in the constant size case, it retains the (possibly
wrong) warnings in the non-constant size case, such that if someone is
prepared to get a few false positives, (s)he'll be able to recover the
current behavior (except that these diagnostics now will never be
converted to errors).

Since the 32-bit variant (intentionally) didn't call might_fault(), the
unification results in this being called twice now. Adding a suitable
#ifdef would be the alternative if that's a problem.

I'd like to point out though that with __compiletime_object_size()
being restricted to gcc before 4.6, the whole construct is going to
become more and more pointless going forward. I would question
however that commit 2fb0815c9ee6b9ac50e15dd8360ec76d9fa46a2 ("gcc4:
disable __compiletime_object_size for GCC 4.6+") was really necessary,
and instead this should have been dealt with as is done here from the
beginning.

Cc: Arjan van de Ven <arjan@linux.intel.com>
Cc: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
 arch/x86/include/asm/uaccess.h    |   68 ++++++++++++++++++++++++++++++++++++++
 arch/x86/include/asm/uaccess_32.h |   26 --------------
 arch/x86/include/asm/uaccess_64.h |   18 ----------
 arch/x86/lib/usercopy_32.c        |    3 -
 4 files changed, 69 insertions(+), 46 deletions(-)

--- 3.12-rc6-x86.orig/arch/x86/include/asm/uaccess.h
+++ 3.12-rc6-x86/arch/x86/include/asm/uaccess.h
@@ -542,5 +542,73 @@ extern struct movsl_mask {
 # include <asm/uaccess_64.h>
 #endif
 
+unsigned long __must_check _copy_from_user(void *to, const void __user *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);
+
+#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()
+
+#else
+
+static inline void
+__copy_from_user_overflow(int size, unsigned long count)
+{
+	WARN(1, "Buffer overflow detected (%d < %lu)!\n", size, count);
+}
+
+#endif
+
+static inline unsigned long __must_check
+copy_from_user(void *to, const void __user *from, unsigned long n)
+{
+	int sz = __compiletime_object_size(to);
+
+	might_fault();
+
+	/*
+	 * 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))
+		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;
+}
+
+#undef __copy_from_user_overflow
+
 #endif /* _ASM_X86_UACCESS_H */
 
--- 3.12-rc6-x86.orig/arch/x86/include/asm/uaccess_32.h
+++ 3.12-rc6-x86/arch/x86/include/asm/uaccess_32.h
@@ -186,31 +186,5 @@ __copy_from_user_inatomic_nocache(void *
 
 unsigned long __must_check copy_to_user(void __user *to,
 					const void *from, unsigned long n);
-unsigned long __must_check _copy_from_user(void *to,
-					  const void __user *from,
-					  unsigned long n);
-
-
-extern void copy_from_user_overflow(void)
-#ifdef CONFIG_DEBUG_STRICT_USER_COPY_CHECKS
-	__compiletime_error("copy_from_user() buffer size is not provably correct")
-#else
-	__compiletime_warning("copy_from_user() buffer size is not provably correct")
-#endif
-;
-
-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 (likely(sz == -1 || sz >= n))
-		n = _copy_from_user(to, from, n);
-	else
-		copy_from_user_overflow();
-
-	return n;
-}
 
 #endif /* _ASM_X86_UACCESS_32_H */
--- 3.12-rc6-x86.orig/arch/x86/include/asm/uaccess_64.h
+++ 3.12-rc6-x86/arch/x86/include/asm/uaccess_64.h
@@ -48,26 +48,8 @@ copy_user_generic(void *to, const void *
 __must_check unsigned long
 _copy_to_user(void __user *to, const void *from, unsigned len);
 __must_check unsigned long
-_copy_from_user(void *to, const void __user *from, unsigned len);
-__must_check unsigned long
 copy_in_user(void __user *to, const void __user *from, unsigned len);
 
-static inline unsigned long __must_check copy_from_user(void *to,
-					  const void __user *from,
-					  unsigned long n)
-{
-	int sz = __compiletime_object_size(to);
-
-	might_fault();
-	if (likely(sz == -1 || sz >= n))
-		n = _copy_from_user(to, from, n);
-#ifdef CONFIG_DEBUG_VM
-	else
-		WARN(1, "Buffer overflow detected!\n");
-#endif
-	return n;
-}
-
 static __always_inline __must_check
 int copy_to_user(void __user *dst, const void *src, unsigned size)
 {
--- 3.12-rc6-x86.orig/arch/x86/lib/usercopy_32.c
+++ 3.12-rc6-x86/arch/x86/lib/usercopy_32.c
@@ -679,8 +679,7 @@ EXPORT_SYMBOL(copy_to_user);
  * If some data could not be copied, this function will pad the copied
  * data to the requested size using zero bytes.
  */
-unsigned long
-_copy_from_user(void *to, const void __user *from, unsigned long n)
+unsigned long _copy_from_user(void *to, const void __user *from, unsigned n)
 {
 	if (access_ok(VERIFY_READ, from, n))
 		n = __copy_from_user(to, from, n);

Re: [PATCH 1/2] x86: unify copy_from_user() size checking

[Ingo Molnar]

* Jan Beulich <JBeulich@suse.com> wrote:

> Commits 4a3127693001c61a21d1ce680db6340623f52e93 ("x86: Turn the
> copy_from_user check into an (optional) compile time warning") and
> 63312b6a6faae3f2e5577f2b001e3b504f10a2aa ("x86: Add a Kconfig option to
> turn the copy_from_user warnings into errors") touched only the 32-bit
> variant of copy_from_user(), whereas the original commit
> 9f0cf4adb6aa0bfccf675c938124e68f7f06349d ("x86: Use
> __builtin_object_size() to validate the buffer size for
> copy_from_user()") also added the same code to the 64-bit one.
> 
> Further the earlier conversion from an inline WARN() to the call to
> copy_from_user_overflow() went a little too far: When the number of
> bytes to be copied is not a constant (e.g. [looking at 3.11] in
> drivers/net/tun.c:__tun_chr_ioctl() or
> drivers/pci/pcie/aer/aer_inject.c:aer_inject_write()), the compiler
> will always have to keep the funtion call, and hence there will always
> be a warning. By using __builtin_constant_p() we can avoid this.
> 
> And then this slightly extends the effect of
> CONFIG_DEBUG_STRICT_USER_COPY_CHECKS in that apart from converting
> warnings to errors in the constant size case, it retains the (possibly
> wrong) warnings in the non-constant size case, such that if someone is
> prepared to get a few false positives, (s)he'll be able to recover the
> current behavior (except that these diagnostics now will never be
> converted to errors).
> 
> Since the 32-bit variant (intentionally) didn't call might_fault(), the
> unification results in this being called twice now. Adding a suitable
> #ifdef would be the alternative if that's a problem.

Addressing that sanely would be nice.

> I'd like to point out though that with __compiletime_object_size()
> being restricted to gcc before 4.6, the whole construct is going to
> become more and more pointless going forward. I would question
> however that commit 2fb0815c9ee6b9ac50e15dd8360ec76d9fa46a2 ("gcc4:
> disable __compiletime_object_size for GCC 4.6+") was really necessary,
> and instead this should have been dealt with as is done here from the
> beginning.

Can we now revert 2fb0815c9ee6?

Thanks,

	Ingo

Re: [PATCH 1/2] x86: unify copy_from_user() size checking

[Guenter Roeck]

On 10/26/2013 03:31 AM, Ingo Molnar wrote:
>
> * Jan Beulich <JBeulich@suse.com> wrote:
>
>> Commits 4a3127693001c61a21d1ce680db6340623f52e93 ("x86: Turn the
>> copy_from_user check into an (optional) compile time warning") and
>> 63312b6a6faae3f2e5577f2b001e3b504f10a2aa ("x86: Add a Kconfig option to
>> turn the copy_from_user warnings into errors") touched only the 32-bit
>> variant of copy_from_user(), whereas the original commit
>> 9f0cf4adb6aa0bfccf675c938124e68f7f06349d ("x86: Use
>> __builtin_object_size() to validate the buffer size for
>> copy_from_user()") also added the same code to the 64-bit one.
>>
>> Further the earlier conversion from an inline WARN() to the call to
>> copy_from_user_overflow() went a little too far: When the number of
>> bytes to be copied is not a constant (e.g. [looking at 3.11] in
>> drivers/net/tun.c:__tun_chr_ioctl() or
>> drivers/pci/pcie/aer/aer_inject.c:aer_inject_write()), the compiler
>> will always have to keep the funtion call, and hence there will always
>> be a warning. By using __builtin_constant_p() we can avoid this.
>>
>> And then this slightly extends the effect of
>> CONFIG_DEBUG_STRICT_USER_COPY_CHECKS in that apart from converting
>> warnings to errors in the constant size case, it retains the (possibly
>> wrong) warnings in the non-constant size case, such that if someone is
>> prepared to get a few false positives, (s)he'll be able to recover the
>> current behavior (except that these diagnostics now will never be
>> converted to errors).
>>
>> Since the 32-bit variant (intentionally) didn't call might_fault(), the
>> unification results in this being called twice now. Adding a suitable
>> #ifdef would be the alternative if that's a problem.
>
> Addressing that sanely would be nice.
>
>> I'd like to point out though that with __compiletime_object_size()
>> being restricted to gcc before 4.6, the whole construct is going to
>> become more and more pointless going forward. I would question
>> however that commit 2fb0815c9ee6b9ac50e15dd8360ec76d9fa46a2 ("gcc4:
>> disable __compiletime_object_size for GCC 4.6+") was really necessary,
>> and instead this should have been dealt with as is done here from the
>> beginning.
>
> Can we now revert 2fb0815c9ee6?
>

I sure don't mind if there is a different and better solution.

Guenter

Re: [PATCH 1/2] x86: unify copy_from_user() size checking

[Jan Beulich]

>>> On 26.10.13 at 12:31, Ingo Molnar <mingo@kernel.org> wrote:
> * Jan Beulich <JBeulich@suse.com> wrote:
>> I'd like to point out though that with __compiletime_object_size()
>> being restricted to gcc before 4.6, the whole construct is going to
>> become more and more pointless going forward. I would question
>> however that commit 2fb0815c9ee6b9ac50e15dd8360ec76d9fa46a2 ("gcc4:
>> disable __compiletime_object_size for GCC 4.6+") was really necessary,
>> and instead this should have been dealt with as is done here from the
>> beginning.
> 
> Can we now revert 2fb0815c9ee6?

Afaict yes, but I certainly didn't test with all possible gcc versions.
So minimally everyone involved in getting that change in would be
good to be in agreement.

Jan

Re: [PATCH 1/2] x86: unify copy_from_user() size checking

[Ingo Molnar]

* Jan Beulich <JBeulich@suse.com> wrote:

> >>> On 26.10.13 at 12:31, Ingo Molnar <mingo@kernel.org> wrote:
> > * Jan Beulich <JBeulich@suse.com> wrote:
> >> I'd like to point out though that with __compiletime_object_size()
> >> being restricted to gcc before 4.6, the whole construct is going to
> >> become more and more pointless going forward. I would question
> >> however that commit 2fb0815c9ee6b9ac50e15dd8360ec76d9fa46a2 ("gcc4:
> >> disable __compiletime_object_size for GCC 4.6+") was really necessary,
> >> and instead this should have been dealt with as is done here from the
> >> beginning.
> > 
> > Can we now revert 2fb0815c9ee6?
> 
> Afaict yes, but I certainly didn't test with all possible gcc 
> versions. So minimally everyone involved in getting that change in 
> would be good to be in agreement.

Mind submitting a patch? I can keep it pending until v3.14, that 
should give time for compiler dependencies to be caught. Even if it 
breaks anything it shouldn't be too hard to revert again ;-)

Thanks,

	Ingo

Re: [PATCH 1/2] x86: unify copy_from_user() size checking

[Jan Beulich]

>>> On 26.10.13 at 12:31, Ingo Molnar <mingo@kernel.org> wrote:
> * Jan Beulich <JBeulich@suse.com> wrote:
>> I'd like to point out though that with __compiletime_object_size()
>> being restricted to gcc before 4.6, the whole construct is going to
>> become more and more pointless going forward. I would question
>> however that commit 2fb0815c9ee6b9ac50e15dd8360ec76d9fa46a2 ("gcc4:
>> disable __compiletime_object_size for GCC 4.6+") was really necessary,
>> and instead this should have been dealt with as is done here from the
>> beginning.
> 
> Can we now revert 2fb0815c9ee6?

Actually I'm afraid parisc would first need to follow the changes
done on x86 here, or else they'd run into (compile time) issues
(s390 and tile only emit warnings, i.e. would at worst suffer
cosmetically unless subtrees putting -Werror in place are
affected).

Jan

Re: [PATCH 1/2] x86: unify copy_from_user() size checking

[Ingo Molnar]

* Jan Beulich <JBeulich@suse.com> wrote:

> >>> On 26.10.13 at 12:31, Ingo Molnar <mingo@kernel.org> wrote:
> > * Jan Beulich <JBeulich@suse.com> wrote:
> >> I'd like to point out though that with __compiletime_object_size()
> >> being restricted to gcc before 4.6, the whole construct is going to
> >> become more and more pointless going forward. I would question
> >> however that commit 2fb0815c9ee6b9ac50e15dd8360ec76d9fa46a2 ("gcc4:
> >> disable __compiletime_object_size for GCC 4.6+") was really necessary,
> >> and instead this should have been dealt with as is done here from the
> >> beginning.
> > 
> > Can we now revert 2fb0815c9ee6?
> 
> Actually I'm afraid parisc would first need to follow the changes 
> done on x86 here, or else they'd run into (compile time) issues 
> (s390 and tile only emit warnings, i.e. would at worst suffer 
> cosmetically unless subtrees putting -Werror in place are 
> affected).

Given how trivial __compiletime_object_size() is, we could replicate 
a (differently named) copy of that in x86 uaccess.h?

This is something that would be pretty platform dependent anyway.

Thanks,

	Ingo

Re: [PATCH 1/2] x86: unify copy_from_user() size checking

[Jan Beulich]

>>> On 29.10.13 at 10:54, Ingo Molnar <mingo@kernel.org> wrote:

> * Jan Beulich <JBeulich@suse.com> wrote:
> 
>> >>> On 26.10.13 at 12:31, Ingo Molnar <mingo@kernel.org> wrote:
>> > * Jan Beulich <JBeulich@suse.com> wrote:
>> >> I'd like to point out though that with __compiletime_object_size()
>> >> being restricted to gcc before 4.6, the whole construct is going to
>> >> become more and more pointless going forward. I would question
>> >> however that commit 2fb0815c9ee6b9ac50e15dd8360ec76d9fa46a2 ("gcc4:
>> >> disable __compiletime_object_size for GCC 4.6+") was really necessary,
>> >> and instead this should have been dealt with as is done here from the
>> >> beginning.
>> > 
>> > Can we now revert 2fb0815c9ee6?
>> 
>> Actually I'm afraid parisc would first need to follow the changes 
>> done on x86 here, or else they'd run into (compile time) issues 
>> (s390 and tile only emit warnings, i.e. would at worst suffer 
>> cosmetically unless subtrees putting -Werror in place are 
>> affected).
> 
> Given how trivial __compiletime_object_size() is, we could replicate 
> a (differently named) copy of that in x86 uaccess.h?

I would never have dared to suggest something like that...

But if you're fine with that, I can certainly do so. I'd then
even wonder whether we shouldn't re-use the same name,
#undef-ing the one we got from compiler*.h - after all the
goal would be for compiler-gcc4.h to change in exactly that
way.

> This is something that would be pretty platform dependent anyway.

Why do you think so? That's entirely a compiler construct.

Jan

Re: [PATCH 1/2] x86: unify copy_from_user() size checking

[Ingo Molnar]

* Jan Beulich <JBeulich@suse.com> wrote:

> >>> On 29.10.13 at 10:54, Ingo Molnar <mingo@kernel.org> wrote:
> 
> > * Jan Beulich <JBeulich@suse.com> wrote:
> > 
> >> >>> On 26.10.13 at 12:31, Ingo Molnar <mingo@kernel.org> wrote:
> >> > * Jan Beulich <JBeulich@suse.com> wrote:
> >> >> I'd like to point out though that with __compiletime_object_size()
> >> >> being restricted to gcc before 4.6, the whole construct is going to
> >> >> become more and more pointless going forward. I would question
> >> >> however that commit 2fb0815c9ee6b9ac50e15dd8360ec76d9fa46a2 ("gcc4:
> >> >> disable __compiletime_object_size for GCC 4.6+") was really necessary,
> >> >> and instead this should have been dealt with as is done here from the
> >> >> beginning.
> >> > 
> >> > Can we now revert 2fb0815c9ee6?
> >> 
> >> Actually I'm afraid parisc would first need to follow the changes 
> >> done on x86 here, or else they'd run into (compile time) issues 
> >> (s390 and tile only emit warnings, i.e. would at worst suffer 
> >> cosmetically unless subtrees putting -Werror in place are 
> >> affected).
> > 
> > Given how trivial __compiletime_object_size() is, we could replicate 
> > a (differently named) copy of that in x86 uaccess.h?
> 
> I would never have dared to suggest something like that...
> 
> But if you're fine with that, I can certainly do so. I'd then even 
> wonder whether we shouldn't re-use the same name,
> #undef-ing the one we got from compiler*.h - after all the
> goal would be for compiler-gcc4.h to change in exactly that way.

Yeah, I think that would work.

> > This is something that would be pretty platform dependent 
> > anyway.
> 
> Why do you think so? That's entirely a compiler construct.

Yeah, what I mean is that the progress of kernel support here is 
done on a per platform manner anyway, the 'generic' piece in 
compiler.h and compiler-gcc4.h is tiny and more of a hindrance than 
a useful generalization/sharing of code...

Thanks,

	Ingo

[PATCH] x86: override __compiletime_object_size()

[Jan Beulich]

As discussed in the context of commits 3df7b41a ("x86: Unify
copy_from_user() size checking") and 7a3d9b0f ("x86: Unify
copy_to_user() and add size checking to it"), we want to leverage
__builtin_object_size() also on newer gcc versions, but with other
architectures still using another model of copy_*_user() verification
we can't replace the global definition. Do it in the (only) header
needing the construct for now.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Cc: Arjan van de Ven <arjan@linux.intel.com>
Cc: Guenter Roeck <linux@roeck-us.net>
---
 arch/x86/include/asm/uaccess.h |    6 ++++++
 1 file changed, 6 insertions(+)

--- 3.13-rc1/arch/x86/include/asm/uaccess.h
+++ 3.13-rc1-x86-compiletime-object-size/arch/x86/include/asm/uaccess.h
@@ -584,6 +584,12 @@ __copy_from_user_overflow(int size, unsi
 
 #endif
 
+/* linux/compiler-gcc4.h restricts this to gcc < 4.6, which doesn't suit us. */
+#if defined(__GNUC__) && GCC_VERSION >= 40100
+# undef __compiletime_object_size
+# define __compiletime_object_size(obj) __builtin_object_size(obj, 0)
+#endif
+
 static inline unsigned long __must_check
 copy_from_user(void *to, const void __user *from, unsigned long n)
 {

Re: [PATCH] x86: override __compiletime_object_size()

[Ingo Molnar]

* Jan Beulich <JBeulich@suse.com> wrote:

> As discussed in the context of commits 3df7b41a ("x86: Unify
> copy_from_user() size checking") and 7a3d9b0f ("x86: Unify
> copy_to_user() and add size checking to it"), we want to leverage
> __builtin_object_size() also on newer gcc versions, but with other
> architectures still using another model of copy_*_user() verification
> we can't replace the global definition. Do it in the (only) header
> needing the construct for now.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Cc: Arjan van de Ven <arjan@linux.intel.com>
> Cc: Guenter Roeck <linux@roeck-us.net>
> ---
>  arch/x86/include/asm/uaccess.h |    6 ++++++
>  1 file changed, 6 insertions(+)
> 
> --- 3.13-rc1/arch/x86/include/asm/uaccess.h
> +++ 3.13-rc1-x86-compiletime-object-size/arch/x86/include/asm/uaccess.h
> @@ -584,6 +584,12 @@ __copy_from_user_overflow(int size, unsi
>  
>  #endif
>  
> +/* linux/compiler-gcc4.h restricts this to gcc < 4.6, which doesn't suit us. */
> +#if defined(__GNUC__) && GCC_VERSION >= 40100
> +# undef __compiletime_object_size
> +# define __compiletime_object_size(obj) __builtin_object_size(obj, 0)
> +#endif
> +

Would be nice to have a more verbose changelog that explains what 
benefits this brings us.

Thanks,

	Ingo

Re: [PATCH] x86: override __compiletime_object_size()

[Jan Beulich]

>>> On 27.11.13 at 15:03, Ingo Molnar <mingo@kernel.org> wrote:

> * Jan Beulich <JBeulich@suse.com> wrote:
> 
>> As discussed in the context of commits 3df7b41a ("x86: Unify
>> copy_from_user() size checking") and 7a3d9b0f ("x86: Unify
>> copy_to_user() and add size checking to it"), we want to leverage
>> __builtin_object_size() also on newer gcc versions, but with other
>> architectures still using another model of copy_*_user() verification
>> we can't replace the global definition. Do it in the (only) header
>> needing the construct for now.
>> 
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> Cc: Arjan van de Ven <arjan@linux.intel.com>
>> Cc: Guenter Roeck <linux@roeck-us.net>
>> ---
>>  arch/x86/include/asm/uaccess.h |    6 ++++++
>>  1 file changed, 6 insertions(+)
>> 
>> --- 3.13-rc1/arch/x86/include/asm/uaccess.h
>> +++ 3.13-rc1-x86-compiletime-object-size/arch/x86/include/asm/uaccess.h
>> @@ -584,6 +584,12 @@ __copy_from_user_overflow(int size, unsi
>>  
>>  #endif
>>  
>> +/* linux/compiler-gcc4.h restricts this to gcc < 4.6, which doesn't suit us. */
>> +#if defined(__GNUC__) && GCC_VERSION >= 40100
>> +# undef __compiletime_object_size
>> +# define __compiletime_object_size(obj) __builtin_object_size(obj, 0)
>> +#endif
>> +
> 
> Would be nice to have a more verbose changelog that explains what 
> benefits this brings us.

That makes no sense to me - the benefits of this construct should
have been explained with its introduction; the change here just
makes it being used under wider range of compilers (and the code
comment already says exactly that).

Jan

Re: [PATCH] x86: override __compiletime_object_size()

[Ingo Molnar]

* Jan Beulich <JBeulich@suse.com> wrote:

> >>> On 27.11.13 at 15:03, Ingo Molnar <mingo@kernel.org> wrote:
> 
> > * Jan Beulich <JBeulich@suse.com> wrote:
> > 
> >> As discussed in the context of commits 3df7b41a ("x86: Unify
> >> copy_from_user() size checking") and 7a3d9b0f ("x86: Unify
> >> copy_to_user() and add size checking to it"), we want to leverage
> >> __builtin_object_size() also on newer gcc versions, but with other
> >> architectures still using another model of copy_*_user() verification
> >> we can't replace the global definition. Do it in the (only) header
> >> needing the construct for now.
> >> 
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >> Cc: Arjan van de Ven <arjan@linux.intel.com>
> >> Cc: Guenter Roeck <linux@roeck-us.net>
> >> ---
> >>  arch/x86/include/asm/uaccess.h |    6 ++++++
> >>  1 file changed, 6 insertions(+)
> >> 
> >> --- 3.13-rc1/arch/x86/include/asm/uaccess.h
> >> +++ 3.13-rc1-x86-compiletime-object-size/arch/x86/include/asm/uaccess.h
> >> @@ -584,6 +584,12 @@ __copy_from_user_overflow(int size, unsi
> >>  
> >>  #endif
> >>  
> >> +/* linux/compiler-gcc4.h restricts this to gcc < 4.6, which doesn't suit us. */
> >> +#if defined(__GNUC__) && GCC_VERSION >= 40100
> >> +# undef __compiletime_object_size
> >> +# define __compiletime_object_size(obj) __builtin_object_size(obj, 0)
> >> +#endif
> >> +
> > 
> > Would be nice to have a more verbose changelog that explains what 
> > benefits this brings us.
> 
> That makes no sense to me - the benefits of this construct should 
> have been explained with its introduction; the change here just 
> makes it being used under wider range of compilers (and the code 
> comment already says exactly that).

Yes indeed - I should have read the referenced changelog...

Thanks, will apply it as-is, if there are no objections from others.

Thanks,

	Ingo

[tip:x86/asm] x86: Unify copy_from_user() size checking

[tip-bot for Jan Beulich]

Commit-ID:  3df7b41aa5e7797f391d0a41f8b0dce1fe366a09
Gitweb:     http://git.kernel.org/tip/3df7b41aa5e7797f391d0a41f8b0dce1fe366a09
Author:     Jan Beulich <JBeulich@suse.com>
AuthorDate: Mon, 21 Oct 2013 09:43:57 +0100
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Sat, 26 Oct 2013 12:27:36 +0200

x86: Unify copy_from_user() size checking

Commits 4a3127693001c61a21d1ce680db6340623f52e93 ("x86: Turn the
copy_from_user check into an (optional) compile time warning")
and 63312b6a6faae3f2e5577f2b001e3b504f10a2aa ("x86: Add a
Kconfig option to turn the copy_from_user warnings into errors")
touched only the 32-bit variant of copy_from_user(), whereas the
original commit 9f0cf4adb6aa0bfccf675c938124e68f7f06349d ("x86:
Use __builtin_object_size() to validate the buffer size for
copy_from_user()") also added the same code to the 64-bit one.

Further the earlier conversion from an inline WARN() to the call
to copy_from_user_overflow() went a little too far: When the
number of bytes to be copied is not a constant (e.g. [looking at
3.11] in drivers/net/tun.c:__tun_chr_ioctl() or
drivers/pci/pcie/aer/aer_inject.c:aer_inject_write()), the
compiler will always have to keep the funtion call, and hence
there will always be a warning. By using __builtin_constant_p()
we can avoid this.

And then this slightly extends the effect of
CONFIG_DEBUG_STRICT_USER_COPY_CHECKS in that apart from
converting warnings to errors in the constant size case, it
retains the (possibly wrong) warnings in the non-constant size
case, such that if someone is prepared to get a few false
positives, (s)he'll be able to recover the current behavior
(except that these diagnostics now will never be converted to
errors).

Since the 32-bit variant (intentionally) didn't call
might_fault(), the unification results in this being called
twice now. Adding a suitable #ifdef would be the alternative if
that's a problem.

I'd like to point out though that with
__compiletime_object_size() being restricted to gcc before 4.6,
the whole construct is going to become more and more pointless
going forward. I would question however that commit
2fb0815c9ee6b9ac50e15dd8360ec76d9fa46a2 ("gcc4: disable
__compiletime_object_size for GCC 4.6+") was really necessary,
and instead this should have been dealt with as is done here
from the beginning.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Cc: Arjan van de Ven <arjan@linux.intel.com>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Thomas Gleixner <tglx@linutronix.de>
Link: http://lkml.kernel.org/r/5265056D02000078000FC4F3@nat28.tlf.novell.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/include/asm/uaccess.h    | 68 +++++++++++++++++++++++++++++++++++++++
 arch/x86/include/asm/uaccess_32.h | 26 ---------------
 arch/x86/include/asm/uaccess_64.h | 18 -----------
 arch/x86/lib/usercopy_32.c        |  3 +-
 4 files changed, 69 insertions(+), 46 deletions(-)

diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index 5838fa9..c9799ed 100644
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -542,5 +542,73 @@ extern struct movsl_mask {
 # include <asm/uaccess_64.h>
 #endif
 
+unsigned long __must_check _copy_from_user(void *to, const void __user *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);
+
+#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()
+
+#else
+
+static inline void
+__copy_from_user_overflow(int size, unsigned long count)
+{
+	WARN(1, "Buffer overflow detected (%d < %lu)!\n", size, count);
+}
+
+#endif
+
+static inline unsigned long __must_check
+copy_from_user(void *to, const void __user *from, unsigned long n)
+{
+	int sz = __compiletime_object_size(to);
+
+	might_fault();
+
+	/*
+	 * 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))
+		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;
+}
+
+#undef __copy_from_user_overflow
+
 #endif /* _ASM_X86_UACCESS_H */
 
diff --git a/arch/x86/include/asm/uaccess_32.h b/arch/x86/include/asm/uaccess_32.h
index 7f760a9..c348c87 100644
--- a/arch/x86/include/asm/uaccess_32.h
+++ b/arch/x86/include/asm/uaccess_32.h
@@ -186,31 +186,5 @@ __copy_from_user_inatomic_nocache(void *to, const void __user *from,
 
 unsigned long __must_check copy_to_user(void __user *to,
 					const void *from, unsigned long n);
-unsigned long __must_check _copy_from_user(void *to,
-					  const void __user *from,
-					  unsigned long n);
-
-
-extern void copy_from_user_overflow(void)
-#ifdef CONFIG_DEBUG_STRICT_USER_COPY_CHECKS
-	__compiletime_error("copy_from_user() buffer size is not provably correct")
-#else
-	__compiletime_warning("copy_from_user() buffer size is not provably correct")
-#endif
-;
-
-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 (likely(sz == -1 || sz >= n))
-		n = _copy_from_user(to, from, n);
-	else
-		copy_from_user_overflow();
-
-	return n;
-}
 
 #endif /* _ASM_X86_UACCESS_32_H */
diff --git a/arch/x86/include/asm/uaccess_64.h b/arch/x86/include/asm/uaccess_64.h
index 4f7923d..4df93c4 100644
--- a/arch/x86/include/asm/uaccess_64.h
+++ b/arch/x86/include/asm/uaccess_64.h
@@ -48,26 +48,8 @@ copy_user_generic(void *to, const void *from, unsigned len)
 __must_check unsigned long
 _copy_to_user(void __user *to, const void *from, unsigned len);
 __must_check unsigned long
-_copy_from_user(void *to, const void __user *from, unsigned len);
-__must_check unsigned long
 copy_in_user(void __user *to, const void __user *from, unsigned len);
 
-static inline unsigned long __must_check copy_from_user(void *to,
-					  const void __user *from,
-					  unsigned long n)
-{
-	int sz = __compiletime_object_size(to);
-
-	might_fault();
-	if (likely(sz == -1 || sz >= n))
-		n = _copy_from_user(to, from, n);
-#ifdef CONFIG_DEBUG_VM
-	else
-		WARN(1, "Buffer overflow detected!\n");
-#endif
-	return n;
-}
-
 static __always_inline __must_check
 int copy_to_user(void __user *dst, const void *src, unsigned size)
 {
diff --git a/arch/x86/lib/usercopy_32.c b/arch/x86/lib/usercopy_32.c
index 3eb18ac..c408d02 100644
--- a/arch/x86/lib/usercopy_32.c
+++ b/arch/x86/lib/usercopy_32.c
@@ -679,8 +679,7 @@ EXPORT_SYMBOL(copy_to_user);
  * If some data could not be copied, this function will pad the copied
  * data to the requested size using zero bytes.
  */
-unsigned long
-_copy_from_user(void *to, const void __user *from, unsigned long n)
+unsigned long _copy_from_user(void *to, const void __user *from, unsigned n)
 {
 	if (access_ok(VERIFY_READ, from, n))
 		n = __copy_from_user(to, from, n);