While it's debatable whether {get,put}_user() should be called with
arguments that have side effects, macro's should be written safely in the
first place. In this case, a slightly off version of put_user() ended up
causing random userspace corruption and these things aren't trivial to
track down.
While some of these conversions aren't strictly necessary, I think it's
better to do all of them so as to be proactive in people accidently
screwing it up in the future.
Signed-off-by: Mike Frysinger <[email protected]>
---
include/asm-generic/uaccess.h | 49 ++++++++++++++++++++++------------------
1 files changed, 27 insertions(+), 22 deletions(-)
diff --git a/include/asm-generic/uaccess.h b/include/asm-generic/uaccess.h
index 6d8cab2..cf3cb73 100644
--- a/include/asm-generic/uaccess.h
+++ b/include/asm-generic/uaccess.h
@@ -143,15 +143,16 @@ static inline __must_check long __copy_to_user(void __user *to,
#define __put_user(x, ptr) \
({ \
__typeof__(*(ptr)) __x = (x); \
+ __typeof__(*(ptr)) *__p = (ptr); \
int __pu_err = -EFAULT; \
- __chk_user_ptr(ptr); \
- switch (sizeof (*(ptr))) { \
+ __chk_user_ptr(__p); \
+ switch (sizeof(*__p)) { \
case 1: \
case 2: \
case 4: \
case 8: \
- __pu_err = __put_user_fn(sizeof (*(ptr)), \
- ptr, &__x); \
+ __pu_err = __put_user_fn(sizeof(*__p), \
+ __p, &__x); \
break; \
default: \
__put_user_bad(); \
@@ -162,9 +163,11 @@ static inline __must_check long __copy_to_user(void __user *to,
#define put_user(x, ptr) \
({ \
+ __typeof__(*(ptr)) _x = (x); \
+ __typeof__(*(ptr)) *_p = (ptr); \
might_sleep(); \
- __access_ok(ptr, sizeof (*ptr)) ? \
- __put_user(x, ptr) : \
+ __access_ok(_p, sizeof(*_p)) ? \
+ __put_user(_x, _p) : \
-EFAULT; \
})
@@ -178,35 +181,36 @@ extern int __put_user_bad(void) __attribute__((noreturn));
#define __get_user(x, ptr) \
({ \
+ __typeof__(*(ptr)) *__p = (ptr); \
int __gu_err = -EFAULT; \
- __chk_user_ptr(ptr); \
- switch (sizeof(*(ptr))) { \
+ __chk_user_ptr(__p); \
+ switch (sizeof(*__p)) { \
case 1: { \
unsigned char __x; \
- __gu_err = __get_user_fn(sizeof (*(ptr)), \
- ptr, &__x); \
- (x) = *(__force __typeof__(*(ptr)) *) &__x; \
+ __gu_err = __get_user_fn(sizeof(*__p), \
+ __p, &__x); \
+ (x) = *(__force __typeof__(*__p) *) &__x; \
break; \
}; \
case 2: { \
unsigned short __x; \
- __gu_err = __get_user_fn(sizeof (*(ptr)), \
- ptr, &__x); \
- (x) = *(__force __typeof__(*(ptr)) *) &__x; \
+ __gu_err = __get_user_fn(sizeof(*__p), \
+ __p, &__x); \
+ (x) = *(__force __typeof__(*__p) *) &__x; \
break; \
}; \
case 4: { \
unsigned int __x; \
- __gu_err = __get_user_fn(sizeof (*(ptr)), \
- ptr, &__x); \
- (x) = *(__force __typeof__(*(ptr)) *) &__x; \
+ __gu_err = __get_user_fn(sizeof(*__p), \
+ __p, &__x); \
+ (x) = *(__force __typeof__(*__p) *) &__x; \
break; \
}; \
case 8: { \
unsigned long long __x; \
- __gu_err = __get_user_fn(sizeof (*(ptr)), \
- ptr, &__x); \
- (x) = *(__force __typeof__(*(ptr)) *) &__x; \
+ __gu_err = __get_user_fn(sizeof(*__p), \
+ __p, &__x); \
+ (x) = *(__force __typeof__(*__p) *) &__x; \
break; \
}; \
default: \
@@ -218,9 +222,10 @@ extern int __put_user_bad(void) __attribute__((noreturn));
#define get_user(x, ptr) \
({ \
+ __typeof__(*(ptr)) *_p = (ptr); \
might_sleep(); \
- __access_ok(ptr, sizeof (*ptr)) ? \
- __get_user(x, ptr) : \
+ __access_ok(_p, sizeof(*_p)) ? \
+ __get_user(x, _p) : \
-EFAULT; \
})
--
1.6.3.1
The strnlen_user() function was missing a access_ok() check on the pointer
given. We've had cases on Blackfin systems where test programs caused
kernel crashes here because userspace passed up a NULL/-1 pointer and the
kernel gladly attempted to run strlen() on it.
Signed-off-by: Mike Frysinger <[email protected]>
---
include/asm-generic/uaccess.h | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/include/asm-generic/uaccess.h b/include/asm-generic/uaccess.h
index cf3cb73..d299557 100644
--- a/include/asm-generic/uaccess.h
+++ b/include/asm-generic/uaccess.h
@@ -296,6 +296,8 @@ strncpy_from_user(char *dst, const char __user *src, long count)
#ifndef strnlen_user
static inline long strnlen_user(const char __user *src, long n)
{
+ if (!access_ok(VERIFY_READ, src, 1))
+ return 0;
return strlen((void * __force)src) + 1;
}
#endif
--
1.6.3.1
There's no reason that I can see to use the short __access_ok() form
directly when the access_ok() is clearer in intent and for most people,
expands to the same C code (i.e. always specify the first field -- access
type). Not all no-mmu systems lack memory protection, so the read/write
could feasibly be checked.
Signed-off-by: Mike Frysinger <[email protected]>
---
include/asm-generic/uaccess.h | 12 ++++++------
1 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/include/asm-generic/uaccess.h b/include/asm-generic/uaccess.h
index d299557..d3a9519 100644
--- a/include/asm-generic/uaccess.h
+++ b/include/asm-generic/uaccess.h
@@ -166,7 +166,7 @@ static inline __must_check long __copy_to_user(void __user *to,
__typeof__(*(ptr)) _x = (x); \
__typeof__(*(ptr)) *_p = (ptr); \
might_sleep(); \
- __access_ok(_p, sizeof(*_p)) ? \
+ access_ok(VERIFY_WRITE, _p, sizeof(*_p)) ? \
__put_user(_x, _p) : \
-EFAULT; \
})
@@ -224,7 +224,7 @@ extern int __put_user_bad(void) __attribute__((noreturn));
({ \
__typeof__(*(ptr)) *_p = (ptr); \
might_sleep(); \
- __access_ok(_p, sizeof(*_p)) ? \
+ access_ok(VERIFY_READ, _p, sizeof(*_p)) ? \
__get_user(x, _p) : \
-EFAULT; \
})
@@ -249,7 +249,7 @@ static inline long copy_from_user(void *to,
const void __user * from, unsigned long n)
{
might_sleep();
- if (__access_ok(from, n))
+ if (access_ok(VERIFY_READ, from, n))
return __copy_from_user(to, from, n);
else
return n;
@@ -259,7 +259,7 @@ static inline long copy_to_user(void __user *to,
const void *from, unsigned long n)
{
might_sleep();
- if (__access_ok(to, n))
+ if (access_ok(VERIFY_WRITE, to, n))
return __copy_to_user(to, from, n);
else
return n;
@@ -283,7 +283,7 @@ __strncpy_from_user(char *dst, const char __user *src, long count)
static inline long
strncpy_from_user(char *dst, const char __user *src, long count)
{
- if (!__access_ok(src, 1))
+ if (!access_ok(VERIFY_READ, src, 1))
return -EFAULT;
return __strncpy_from_user(dst, src, count);
}
@@ -323,7 +323,7 @@ static inline __must_check unsigned long
clear_user(void __user *to, unsigned long n)
{
might_sleep();
- if (!__access_ok(to, n))
+ if (!access_ok(VERIFY_WRITE, to, n))
return n;
return __clear_user(to, n);
--
1.6.3.1
From: Arnd Bergmann <[email protected]>
The access_ok() function operates on a pointer, not unsigned long.
Otherwise builds result in a crazy amount of warnings such as:
CC [M] fs/smbfs/request.o
In file included from
/usr/local/src/linux/linux-2.6/arch/blackfin/include/asm/uaccess.h:17,
from include/net/checksum.h:26,
from include/linux/skbuff.h:28,
from include/linux/ip.h:109,
from include/net/ip.h:27,
from fs/smbfs/smbiod.c:23:
include/asm-generic/uaccess.h: In function ‘copy_from_user’:
include/asm-generic/uaccess.h:247: warning: passing argument 1 of
‘_access_ok’ makes integer from pointer without a cast
include/asm-generic/uaccess.h: In function ‘copy_to_user’:
include/asm-generic/uaccess.h:257: warning: passing argument 1 of
‘_access_ok’ makes integer from pointer without a cast
include/asm-generic/uaccess.h: In function ‘strncpy_from_user’:
include/asm-generic/uaccess.h:281: warning: passing argument 1 of
‘_access_ok’ makes integer from pointer without a cast
include/asm-generic/uaccess.h: In function ‘clear_user’:
include/asm-generic/uaccess.h:319: warning: passing argument 1 of
‘_access_ok’ makes integer from pointer without a cast
Signed-off-by: Arnd Bergmann <[email protected]>
Signed-off-by: Mike Frysinger <[email protected]>
---
include/asm-generic/uaccess.h | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/asm-generic/uaccess.h b/include/asm-generic/uaccess.h
index d3a9519..4b32dab 100644
--- a/include/asm-generic/uaccess.h
+++ b/include/asm-generic/uaccess.h
@@ -37,14 +37,14 @@ static inline void set_fs(mm_segment_t fs)
#define VERIFY_READ 0
#define VERIFY_WRITE 1
-#define access_ok(type, addr, size) __access_ok((unsigned long)(addr),(size))
+#define access_ok(type, addr, size) __access_ok((addr), (size))
/*
* The architecture should really override this if possible, at least
* doing a check on the get_fs()
*/
#ifndef __access_ok
-static inline int __access_ok(unsigned long addr, unsigned long size)
+static inline int __access_ok(void __user *ptr, unsigned long size)
{
return 1;
}
--
1.6.3.1
On Sunday 14 June 2009, Mike Frysinger wrote:
> While it's debatable whether {get,put}_user() should be called with
> arguments that have side effects, macro's should be written safely in the
> first place. In this case, a slightly off version of put_user() ended up
> causing random userspace corruption and these things aren't trivial to
> track down.
>
> While some of these conversions aren't strictly necessary, I think it's
> better to do all of them so as to be proactive in people accidently
> screwing it up in the future.
I've tried this and failed. This change adds an endless number of sparse
warnings in put_user and even gcc warnings in get_user. The problem
is that typeof() carries over the 'const' and '__user' modifiers, both
of which prevent you from assigning data to the new pointer that you
constructed.
I'd love to see a way to do this correctly, but this patch won't cut it.
Arnd <><
On Sunday 14 June 2009, Mike Frysinger wrote:
> @@ -37,14 +37,14 @@ static inline void set_fs(mm_segment_t fs)
> #define VERIFY_READ 0
> #define VERIFY_WRITE 1
>
> -#define access_ok(type, addr, size) __access_ok((unsigned long)(addr),(size))
> +#define access_ok(type, addr, size) __access_ok((addr), (size))
>
> /*
> * The architecture should really override this if possible, at least
Upon actually testing this change, it turns out that this causes more
trouble because of having to pass down pointers that may have 'const'
or 'volatile' modifiers. I'm sure there is a way to do this correctly,
but most architectures seem to cope well with 'unsigned long' here,
and your patch 3/4 solves the original problem nicely.
Arnd <><
On Sunday 14 June 2009, Mike Frysinger wrote:
> There's no reason that I can see to use the short __access_ok() form
> directly when the access_ok() is clearer in intent and for most people,
> expands to the same C code (i.e. always specify the first field -- access
> type). Not all no-mmu systems lack memory protection, so the read/write
> could feasibly be checked.
>
> Signed-off-by: Mike Frysinger <[email protected]>
Applied, thanks.
Arnd <><
On Sunday 14 June 2009, Mike Frysinger wrote:
> The strnlen_user() function was missing a access_ok() check on the pointer
> given. We've had cases on Blackfin systems where test programs caused
> kernel crashes here because userspace passed up a NULL/-1 pointer and the
> kernel gladly attempted to run strlen() on it.
>
> Signed-off-by: Mike Frysinger <[email protected]>
Applied, thanks.
Arnd <><
On Sun, Jun 14, 2009 at 16:17, Arnd Bergmann wrote:
> On Sunday 14 June 2009, Mike Frysinger wrote:
>> @@ -37,14 +37,14 @@ static inline void set_fs(mm_segment_t fs)
>> #define VERIFY_READ 0
>> #define VERIFY_WRITE 1
>>
>> -#define access_ok(type, addr, size) __access_ok((unsigned long)(addr),(size))
>> +#define access_ok(type, addr, size) __access_ok((addr), (size))
>>
>> /*
>> * The architecture should really override this if possible, at least
>
> Upon actually testing this change, it turns out that this causes more
> trouble because of having to pass down pointers that may have 'const'
> or 'volatile' modifiers. I'm sure there is a way to do this correctly,
> but most architectures seem to cope well with 'unsigned long' here,
> and your patch 3/4 solves the original problem nicely.
yeah, i ended up making the Blackfin access_ok replacement cast the
first argument to an unsigned long to avoid those warnings ;). i
guess we can just leave this alone for a rainy day ...
-mike
On Sun, Jun 14, 2009 at 10:14:39PM +0200, Arnd Bergmann wrote:
> On Sunday 14 June 2009, Mike Frysinger wrote:
> > While it's debatable whether {get,put}_user() should be called with
> > arguments that have side effects, macro's should be written safely in the
> > first place. In this case, a slightly off version of put_user() ended up
> > causing random userspace corruption and these things aren't trivial to
> > track down.
> >
> > While some of these conversions aren't strictly necessary, I think it's
> > better to do all of them so as to be proactive in people accidently
> > screwing it up in the future.
>
> I've tried this and failed. This change adds an endless number of sparse
> warnings in put_user and even gcc warnings in get_user. The problem
> is that typeof() carries over the 'const' and '__user' modifiers, both
> of which prevent you from assigning data to the new pointer that you
> constructed.
>
> I'd love to see a way to do this correctly, but this patch won't cut it.
Note that sizeof(*(ptr)) does *NOT* evaluate ptr, unless we are dealing
with variably-modified type. The same goes for typeof. And chk_user_ptr()
expands to (void)0 during the build. So I don't believe that existing variant
is incorrect - we do not evaluate the argument twice.
On Wed, Jun 24, 2009 at 00:38, Al Viro wrote:
> On Sun, Jun 14, 2009 at 10:14:39PM +0200, Arnd Bergmann wrote:
>> On Sunday 14 June 2009, Mike Frysinger wrote:
>> > While it's debatable whether {get,put}_user() should be called with
>> > arguments that have side effects, macro's should be written safely in the
>> > first place. In this case, a slightly off version of put_user() ended up
>> > causing random userspace corruption and these things aren't trivial to
>> > track down.
>> >
>> > While some of these conversions aren't strictly necessary, I think it's
>> > better to do all of them so as to be proactive in people accidently
>> > screwing it up in the future.
>>
>> I've tried this and failed. This change adds an endless number of sparse
>> warnings in put_user and even gcc warnings in get_user. The problem
>> is that typeof() carries over the 'const' and '__user' modifiers, both
>> of which prevent you from assigning data to the new pointer that you
>> constructed.
>>
>> I'd love to see a way to do this correctly, but this patch won't cut it.
>
> Note that sizeof(*(ptr)) does *NOT* evaluate ptr, unless we are dealing
> with variably-modified type. The same goes for typeof. And chk_user_ptr()
> expands to (void)0 during the build.
i never said it does -- i explicitly said i converted more than needed
on purpose
> So I don't believe that existing variant
> is incorrect - we do not evaluate the argument twice.
except that it does. read get_user() where the argument is expanded
once for access_ok() and twice when tailing to __get_user(). same
goes for put_user().
-mike