* [PATCH] fix generic get_user and put_user
@ 2011-05-17 18:35 Mark Salter
2011-05-17 20:26 ` Arnd Bergmann
0 siblings, 1 reply; 3+ messages in thread
From: Mark Salter @ 2011-05-17 18:35 UTC (permalink / raw)
To: arnd; +Cc: linux-arch
The generic uaccess.h implements get_user() and put_user() as macros. The
current version of these do not properly handle pointers passed in with
post-increment and the like. In the case of put_user(0, ptr++), ptr gets
incremented twice. Once for the call to access_ok() and once in __put_user().
This patch creates a local copy of the pointer so that it is safe to use
post/pre increment/decrement on the pointer arg.
Signed-off-by: Mark Salter <msalter@redhat.com>
---
include/asm-generic/uaccess.h | 10 ++++++----
1 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/include/asm-generic/uaccess.h b/include/asm-generic/uaccess.h
index 1d0fdf8..5079335 100644
--- a/include/asm-generic/uaccess.h
+++ b/include/asm-generic/uaccess.h
@@ -162,9 +162,10 @@ static inline __must_check long __copy_to_user(void __user *to,
#define put_user(x, ptr) \
({ \
+ __typeof__(*(ptr)) *__pu_ptr = (ptr); \
might_sleep(); \
- access_ok(VERIFY_WRITE, ptr, sizeof(*ptr)) ? \
- __put_user(x, ptr) : \
+ access_ok(VERIFY_WRITE, __pu_ptr, sizeof(*ptr)) ? \
+ __put_user(x, __pu_ptr) : \
-EFAULT; \
})
@@ -218,9 +219,10 @@ extern int __put_user_bad(void) __attribute__((noreturn));
#define get_user(x, ptr) \
({ \
+ __typeof__(*(ptr)) *__gu_ptr = (ptr); \
might_sleep(); \
- access_ok(VERIFY_READ, ptr, sizeof(*ptr)) ? \
- __get_user(x, ptr) : \
+ access_ok(VERIFY_READ, __gu_ptr, sizeof(*ptr)) ? \
+ __get_user(x, __gu_ptr) : \
-EFAULT; \
})
--
1.6.2.5
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] fix generic get_user and put_user
2011-05-17 18:35 [PATCH] fix generic get_user and put_user Mark Salter
@ 2011-05-17 20:26 ` Arnd Bergmann
2011-05-17 21:25 ` Mark Salter
0 siblings, 1 reply; 3+ messages in thread
From: Arnd Bergmann @ 2011-05-17 20:26 UTC (permalink / raw)
To: Mark Salter; +Cc: linux-arch
On Tuesday 17 May 2011, Mark Salter wrote:
> diff --git a/include/asm-generic/uaccess.h b/include/asm-generic/uaccess.h
> index 1d0fdf8..5079335 100644
> --- a/include/asm-generic/uaccess.h
> +++ b/include/asm-generic/uaccess.h
> @@ -162,9 +162,10 @@ static inline __must_check long __copy_to_user(void __user *to,
>
> #define put_user(x, ptr) \
> ({ \
> + __typeof__(*(ptr)) *__pu_ptr = (ptr); \
> might_sleep(); \
> - access_ok(VERIFY_WRITE, ptr, sizeof(*ptr)) ? \
> - __put_user(x, ptr) : \
> + access_ok(VERIFY_WRITE, __pu_ptr, sizeof(*ptr)) ? \
> + __put_user(x, __pu_ptr) : \
> -EFAULT; \
> })
>
> @@ -218,9 +219,10 @@ extern int __put_user_bad(void) __attribute__((noreturn));
>
> #define get_user(x, ptr) \
> ({ \
> + __typeof__(*(ptr)) *__gu_ptr = (ptr); \
> might_sleep(); \
> - access_ok(VERIFY_READ, ptr, sizeof(*ptr)) ? \
> - __get_user(x, ptr) : \
> + access_ok(VERIFY_READ, __gu_ptr, sizeof(*ptr)) ? \
> + __get_user(x, __gu_ptr) : \
> -EFAULT; \
> })
>
IIRC, this doesn't work for get_user if the pointer is marked const.
Do you see a real problem with the current definitions, or are you
just trying to improve them genrally?
Arnd
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] fix generic get_user and put_user
2011-05-17 20:26 ` Arnd Bergmann
@ 2011-05-17 21:25 ` Mark Salter
0 siblings, 0 replies; 3+ messages in thread
From: Mark Salter @ 2011-05-17 21:25 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: linux-arch
On Tue, 2011-05-17 at 22:26 +0200, Arnd Bergmann wrote:
> On Tuesday 17 May 2011, Mark Salter wrote:
> > diff --git a/include/asm-generic/uaccess.h b/include/asm-generic/uaccess.h
> > index 1d0fdf8..5079335 100644
> > --- a/include/asm-generic/uaccess.h
> > +++ b/include/asm-generic/uaccess.h
> > @@ -162,9 +162,10 @@ static inline __must_check long __copy_to_user(void __user *to,
> >
> > #define put_user(x, ptr) \
> > ({ \
> > + __typeof__(*(ptr)) *__pu_ptr = (ptr); \
> > might_sleep(); \
> > - access_ok(VERIFY_WRITE, ptr, sizeof(*ptr)) ? \
> > - __put_user(x, ptr) : \
> > + access_ok(VERIFY_WRITE, __pu_ptr, sizeof(*ptr)) ? \
> > + __put_user(x, __pu_ptr) : \
> > -EFAULT; \
> > })
> >
> > @@ -218,9 +219,10 @@ extern int __put_user_bad(void) __attribute__((noreturn));
> >
> > #define get_user(x, ptr) \
> > ({ \
> > + __typeof__(*(ptr)) *__gu_ptr = (ptr); \
> > might_sleep(); \
> > - access_ok(VERIFY_READ, ptr, sizeof(*ptr)) ? \
> > - __get_user(x, ptr) : \
> > + access_ok(VERIFY_READ, __gu_ptr, sizeof(*ptr)) ? \
> > + __get_user(x, __gu_ptr) : \
> > -EFAULT; \
> > })
> >
>
> IIRC, this doesn't work for get_user if the pointer is marked const.
> Do you see a real problem with the current definitions, or are you
> just trying to improve them genrally?
>
I think you're thinking of doing this:
__typeof__(*(ptr)) __tmp_x;
then assigning to __tmp_x wouldn't work if ptr was a pointer to a const
value.
But that does make me think there's a problem with my patch if the ptr
itself is const. In that case you might see a warning about losing the
const attribute with the assignment. So maybe those should be:
__typeof__(*(ptr)) *const __xx_ptr = (ptr);
And yes, I am seeing an actual runtime failure due to put_user double
incrementing a ptr. A grep of the kernel shows a few dozen cases where
this can bite. Mostly in arch code where arch-specific implentations of
put_user/get_user are being used, but also in driver and network code.
--Mark
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2011-05-17 21:25 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-05-17 18:35 [PATCH] fix generic get_user and put_user Mark Salter
2011-05-17 20:26 ` Arnd Bergmann
2011-05-17 21:25 ` Mark Salter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).