[PATCH] x86: unify copy_from_user() checking

[PATCH] x86: unify copy_from_user() 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.

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.

It also puzzles me that similar checking isn't done for copy_to_user():
While not resulting in (kernel) buffer overflows, size mistakes there
would still lead to information leaks.

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    |   25 +++++++++++++++++++++++++
 arch/x86/include/asm/uaccess_32.h |   23 -----------------------
 arch/x86/include/asm/uaccess_64.h |   16 ----------------
 3 files changed, 25 insertions(+), 39 deletions(-)

--- 3.12-rc5/arch/x86/include/asm/uaccess.h
+++ 3.12-rc5-x86-copy_from_user-overflow/arch/x86/include/asm/uaccess.h
@@ -542,5 +542,30 @@ extern struct movsl_mask {
 # include <asm/uaccess_64.h>
 #endif
 
+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);
+
+	might_fault();
+	if (likely(sz == -1 || sz >= n))
+		n = _copy_from_user(to, from, n);
+	else if(__builtin_constant_p(n))
+		copy_from_user_overflow();
+	else
+		WARN(1, "Buffer overflow detected (%d < %lu)!\n", sz, n);
+
+	return n;
+}
+
 #endif /* _ASM_X86_UACCESS_H */
 
--- 3.12-rc5/arch/x86/include/asm/uaccess_32.h
+++ 3.12-rc5-x86-copy_from_user-overflow/arch/x86/include/asm/uaccess_32.h
@@ -190,27 +190,4 @@ unsigned long __must_check _copy_from_us
 					  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-rc5/arch/x86/include/asm/uaccess_64.h
+++ 3.12-rc5-x86-copy_from_user-overflow/arch/x86/include/asm/uaccess_64.h
@@ -52,22 +52,6 @@ _copy_from_user(void *to, const void __u
 __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)
 {

Re: [PATCH] x86: unify copy_from_user() checking

[Arjan van de Ven]

>
> It also puzzles me that similar checking isn't done for copy_to_user():
> While not resulting in (kernel) buffer overflows, size mistakes there
> would still lead to information leaks.

at the time I went for the highest payoff only; e.g. the mistake of copying
to a fixed size kernel buffer.

Adding the other direction is a good idea of course.


>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Cc: Arjan van de Ven <arjan@linux.intel.com>
> Cc: Guenter Roeck <linux@roeck-us.net>


> +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);
> +	else if(__builtin_constant_p(n))
> +		copy_from_user_overflow();


this part I am not so sure about.
the original intent was that even if n is not constant, the compiler must still be able
to prove that it is smaller than sz using the range tracking feature in gcc!
In fact, that was the whole point.
The code (at the time, they're all fixed) found cases where the checks done to "n" were off by one
etc...

by requiring "n" to be constant for these checks you remove that layer of checking.

if you have found cases where this matters... maybe you found a new security issue...


so while I like the cleanup part of your patch.... not convinced on this part yet

Re: [PATCH] x86: unify copy_from_user() checking

[Jan Beulich]

>>> On 16.10.13 at 16:00, Arjan van de Ven <arjan@linux.intel.com> wrote:
>> +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);
>> +	else if(__builtin_constant_p(n))
>> +		copy_from_user_overflow();
> 
> 
> this part I am not so sure about.
> the original intent was that even if n is not constant, the compiler must 
> still be able
> to prove that it is smaller than sz using the range tracking feature in gcc!

I had pointed out cases in the patch description where I was getting
a warning when I think I shouldn't, and since I pay attention to
warnings this keeps me going back to the sources whenever I didn't
look at the reason long enough and forgot whether it's safe to ignore
these warnings. Warnings are nice and useful, but especially when
they sound dangerous having false positives isn't helpful at all.

> In fact, that was the whole point.
> The code (at the time, they're all fixed) found cases where the checks done 
> to "n" were off by one
> etc...
> 
> by requiring "n" to be constant for these checks you remove that layer of 
> checking.
> 
> if you have found cases where this matters... maybe you found a new security 
> issue...

Iirc I could convince myself that in the cited cases the warnings
were there for no reason.

Jan

Re: [PATCH] x86: unify copy_from_user() checking

[Arjan van de Ven]

>> if you have found cases where this matters... maybe you found a new security
>> issue...
>
> Iirc I could convince myself that in the cited cases the warnings
> were there for no reason.

can you pick one on a source level ?
(and the gcc 4.6 had a few false positives which is why it got disabled there)

Re: [PATCH] x86: unify copy_from_user() checking

[Jan Beulich]

>>> On 16.10.13 at 19:05, Arjan van de Ven <arjan@linux.intel.com> wrote:

>>> if you have found cases where this matters... maybe you found a new security
>>> issue...
>>
>> Iirc I could convince myself that in the cited cases the warnings
>> were there for no reason.
> 
> can you pick one on a source level ?
> (and the gcc 4.6 had a few false positives which is why it got disabled 
> there)

Sure: Let's take __tun_chr_ioctl(): While a static function, it gets
called with two different values for ifreq_len, both of which are
provably (for a human) correct. I don't think, however, that the
compiler can be expected to do so on its own in all cases - I would
expect it to be able to when it decides to inline the function in
both callers, but the larger that function grows, the more likely
it'll become that the compiler chooses to keep it separate (and it
surely would when CONFIG_CC_OPTIMIZE_FOR_SIZE).

Otoh one would expect a modern compiler to be able to do the
checking in the case of aer_inject_write(). Yet not everyone is
using most recent compiler versions, but I personally expect a
warning free compilation in that case too (at least outside the
staging sub-tree, which I avoid to build as much as possible). And
I know that I had seen the warning there (or else it wouldn't have
caught my attention, and I wouldn't have quoted it in the patch
description).

Jan

Re: [PATCH] x86: unify copy_from_user() checking

[Arjan van de Ven]

On 10/17/2013 2:45 AM, Jan Beulich wrote:
> Sure: Let's take __tun_chr_ioctl(): While a static function, it gets
> called with two different values for ifreq_len, both of which are
> provably (for a human) correct. I don't think, however, that the
> compiler can be expected to do so on its own in all cases - I would
> expect it to be able to when it decides to inline the function in
> both callers, but the larger that function grows, the more likely
> it'll become that the compiler chooses to keep it separate (and it
> surely would when CONFIG_CC_OPTIMIZE_FOR_SIZE).

with multiple callers.... I would feel safer if there was a check inside the function.
but this is a fair point (the function is large so indeed it is unlikely to get inlined)




> Otoh one would expect a modern compiler to be able to do the
> checking in the case of aer_inject_write(). Yet not everyone is
> using most recent compiler versions, but I personally expect a
> warning free compilation in that case too (at least outside the
> staging sub-tree, which I avoid to build as much as possible). And
> I know that I had seen the warning there (or else it wouldn't have
> caught my attention, and I wouldn't have quoted it in the patch
> description).

if gcc doesn't find this one then arguably that's a gcc bug.
(which is the thing that has been plaguing this feature unfortunately. in theory gcc
should be able to cope with many of these.... in practice...)


for me, the value of the feature overall is this range checking, not the fixed size part.
for fixed size... the chance of the programmer getting it wrong is near zero.
the chance of getting one of the checks wrong is much higher
(we've had cases of wrong sign in the checks, off by ones in the checks etc)
and that is what it was supposed to find.
If that's not possible due practical issues (like the inline case above but more
the compiler practicalities).... removing the warning part entirely is likely just better.

Having a runtime check for the case where the argument is not constant but we know the buffer
size... is likely still clear value... cheap (perfect branch prediction unless disaster hits!)
and the failure case is obviously the disaster case.

Re: [PATCH] x86: unify copy_from_user() checking

[Jan Beulich]

>>> On 17.10.13 at 17:45, Arjan van de Ven <arjan@linux.intel.com> wrote:
> for me, the value of the feature overall is this range checking, not the 
> fixed size part.
> for fixed size... the chance of the programmer getting it wrong is near 
> zero.
> the chance of getting one of the checks wrong is much higher
> (we've had cases of wrong sign in the checks, off by ones in the checks etc)
> and that is what it was supposed to find.
> If that's not possible due practical issues (like the inline case above but 
> more
> the compiler practicalities).... removing the warning part entirely is 
> likely just better.

But it would at least cover the case where, for some pointer,
someone mixes up sizeof(ptr) and sizeof(*ptr). So I think - it
being cheap - the current constant size check could stay, ...

> Having a runtime check for the case where the argument is not constant but 
> we know the buffer
> size... is likely still clear value... cheap (perfect branch prediction 
> unless disaster hits!)
> and the failure case is obviously the disaster case.

... and the non-constant case be taken care of at run time.
That's precisely what the patch does.

Jan

Re: [PATCH] x86: unify copy_from_user() checking

[Arjan van de Ven]

>
> ... and the non-constant case be taken care of at run time.
> That's precisely what the patch does.

fair enough.

I would like to see a comment above the code to describe this reasoning
and the objective and what the desired behavior is... so that we don't
have to reverse engineer this again 2 years from now ;-)

Re: [PATCH] x86: unify copy_from_user() checking

[Jan Beulich]

>>> On 17.10.13 at 18:04, Arjan van de Ven <arjan@linux.intel.com> wrote:
>> 
>> ... and the non-constant case be taken care of at run time.
>> That's precisely what the patch does.
> 
> fair enough.
> 
> I would like to see a comment above the code to describe this reasoning
> and the objective and what the desired behavior is... so that we don't
> have to reverse engineer this again 2 years from now ;-)

Will do, and then perhaps mirror the whole behavior to
copy_to_user().

Jan

Re: [PATCH] x86: unify copy_from_user() checking

[Arjan van de Ven]

On 10/17/2013 9:08 AM, Jan Beulich wrote:
>>>> On 17.10.13 at 18:04, Arjan van de Ven <arjan@linux.intel.com> wrote:
>>>
>>> ... and the non-constant case be taken care of at run time.
>>> That's precisely what the patch does.
>>
>> fair enough.
>>
>> I would like to see a comment above the code to describe this reasoning
>> and the objective and what the desired behavior is... so that we don't
>> have to reverse engineer this again 2 years from now ;-)
>
> Will do, and then perhaps mirror the whole behavior to
> copy_to_user().

yeah that certainly makes sense



in hindsight we should have made the copy_*_user prototype take a "type" argument,
so that the sizeof/etc are done inside the macro.
Or at least have this available as an option with a "advanced" version with just a size.
but that's a 20 year old thing at this point...