linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Re: x86, x32: Correct invalid use of user timespec in the kernel
       [not found] <20140131025453.B594B660CA3@gitolite.kernel.org>
@ 2014-01-31 17:50 ` Dave Jones
  2014-01-31 18:06   ` H. Peter Anvin
  0 siblings, 1 reply; 7+ messages in thread
From: Dave Jones @ 2014-01-31 17:50 UTC (permalink / raw)
  To: Linux Kernel Mailing List; +Cc: Linus Torvalds, hpa

On Fri, Jan 31, 2014 at 02:54:53AM +0000, Linux Kernel wrote:

 > Commit:     2def2ef2ae5f3990aabdbe8a755911902707d268
 > 
 > ...
 >  
 > -	if (get_compat_timespec(&ktspec, timeout))
 > +	if (compat_get_timespec(&ktspec, timeout))
 >  		return -EFAULT;
 >  
 >  	datagrams = __sys_recvmmsg(fd, (struct mmsghdr __user *)mmsg, vlen,
 >  				   flags | MSG_CMSG_COMPAT, &ktspec);
 > -	if (datagrams > 0 && put_compat_timespec(&ktspec, timeout))
 > +	if (datagrams > 0 && compat_put_timespec(&ktspec, timeout))
 >  		datagrams = -EFAULT;
 >  

Can we rename one of each of those functions ?
It's not really surprising they got mixed up given they look so alike.

It looks like an accident just waiting to happen again.

	Dave


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: x86, x32: Correct invalid use of user timespec in the kernel
  2014-01-31 17:50 ` x86, x32: Correct invalid use of user timespec in the kernel Dave Jones
@ 2014-01-31 18:06   ` H. Peter Anvin
  2014-01-31 18:45     ` Linus Torvalds
  0 siblings, 1 reply; 7+ messages in thread
From: H. Peter Anvin @ 2014-01-31 18:06 UTC (permalink / raw)
  To: Dave Jones, Linux Kernel Mailing List, Linus Torvalds

On 01/31/2014 09:50 AM, Dave Jones wrote:
> On Fri, Jan 31, 2014 at 02:54:53AM +0000, Linux Kernel wrote:
> 
>  > Commit:     2def2ef2ae5f3990aabdbe8a755911902707d268
>  > 
>  > ...
>  >  
>  > -	if (get_compat_timespec(&ktspec, timeout))
>  > +	if (compat_get_timespec(&ktspec, timeout))
>  >  		return -EFAULT;
>  >  
>  >  	datagrams = __sys_recvmmsg(fd, (struct mmsghdr __user *)mmsg, vlen,
>  >  				   flags | MSG_CMSG_COMPAT, &ktspec);
>  > -	if (datagrams > 0 && put_compat_timespec(&ktspec, timeout))
>  > +	if (datagrams > 0 && compat_put_timespec(&ktspec, timeout))
>  >  		datagrams = -EFAULT;
>  >  
> 
> Can we rename one of each of those functions ?
> It's not really surprising they got mixed up given they look so alike.
> 
> It looks like an accident just waiting to happen again.
> 

Very much so, I made the same comment.

My feeling is that {get,put}_compat_timespec() should at the very least
have leading underscores to flag it as a low-level function, but better
suggestions would be appreciated.

	-hpa



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: x86, x32: Correct invalid use of user timespec in the kernel
  2014-01-31 18:06   ` H. Peter Anvin
@ 2014-01-31 18:45     ` Linus Torvalds
  2014-01-31 19:00       ` H. Peter Anvin
                         ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Linus Torvalds @ 2014-01-31 18:45 UTC (permalink / raw)
  To: H. Peter Anvin; +Cc: Dave Jones, Linux Kernel Mailing List

On Fri, Jan 31, 2014 at 10:06 AM, H. Peter Anvin <hpa@linux.intel.com> wrote:
>
> My feeling is that {get,put}_compat_timespec() should at the very least
> have leading underscores to flag it as a low-level function, but better
> suggestions would be appreciated.

Why not just remove it entirely, and change all users to
compat_[get|set]_timespec (same for timeval etc, of course).

After all, compat_*_time*() does fall back cleanly for non-x32 cases.
And sure, maybe that particular code is never *needed* for x32
support, but the overhead is generally zero (since in most cases X32
isn't even configured), or very low anyway. So the upside of having
two subtly incompatible interfaces is very dubious, no?

                Linus

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: x86, x32: Correct invalid use of user timespec in the kernel
  2014-01-31 18:45     ` Linus Torvalds
@ 2014-01-31 19:00       ` H. Peter Anvin
  2014-01-31 19:13       ` H. Peter Anvin
  2014-01-31 22:37       ` H. Peter Anvin
  2 siblings, 0 replies; 7+ messages in thread
From: H. Peter Anvin @ 2014-01-31 19:00 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: Dave Jones, Linux Kernel Mailing List

On 01/31/2014 10:45 AM, Linus Torvalds wrote:
> On Fri, Jan 31, 2014 at 10:06 AM, H. Peter Anvin <hpa@linux.intel.com> wrote:
>>
>> My feeling is that {get,put}_compat_timespec() should at the very least
>> have leading underscores to flag it as a low-level function, but better
>> suggestions would be appreciated.
> 
> Why not just remove it entirely, and change all users to
> compat_[get|set]_timespec (same for timeval etc, of course).
> 
> After all, compat_*_time*() does fall back cleanly for non-x32 cases.
> And sure, maybe that particular code is never *needed* for x32
> support, but the overhead is generally zero (since in most cases X32
> isn't even configured), or very low anyway. So the upside of having
> two subtly incompatible interfaces is very dubious, no?
> 

As they both seem to be out of line, I would think so.  More than half
of the use cases are in kernel/compat.c where we could use a
double-underscore inline version if we really care -- it would probably
be a net win in terms of performance.

There are only 25 call sites in the kernel of
'(get|put)_compat_time(val|spec)' and that includes the ones inside the
larger functions.

	-hpa


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: x86, x32: Correct invalid use of user timespec in the kernel
  2014-01-31 18:45     ` Linus Torvalds
  2014-01-31 19:00       ` H. Peter Anvin
@ 2014-01-31 19:13       ` H. Peter Anvin
  2014-01-31 22:37       ` H. Peter Anvin
  2 siblings, 0 replies; 7+ messages in thread
From: H. Peter Anvin @ 2014-01-31 19:13 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: Dave Jones, Linux Kernel Mailing List, H.J. Lu

On 01/31/2014 10:45 AM, Linus Torvalds wrote:
> On Fri, Jan 31, 2014 at 10:06 AM, H. Peter Anvin <hpa@linux.intel.com> wrote:
>>
>> My feeling is that {get,put}_compat_timespec() should at the very least
>> have leading underscores to flag it as a low-level function, but better
>> suggestions would be appreciated.
> 
> Why not just remove it entirely, and change all users to
> compat_[get|set]_timespec (same for timeval etc, of course).
> 
> After all, compat_*_time*() does fall back cleanly for non-x32 cases.
> And sure, maybe that particular code is never *needed* for x32
> support, but the overhead is generally zero (since in most cases X32
> isn't even configured), or very low anyway. So the upside of having
> two subtly incompatible interfaces is very dubious, no?
> 

Hmmm... it ends up being a bit weird even so.  Some of the interfaces
ought to be revamped at a higher level.

Consider this bit in ipc/compat.c:

long compat_sys_semtimedop(int semid, struct sembuf __user *tsems,
                unsigned nsops, const struct compat_timespec __user
*timeout)
{
        struct timespec __user *ts64 = NULL;
        if (timeout) {
                struct timespec ts;
                ts64 = compat_alloc_user_space(sizeof(*ts64));
                if (get_compat_timespec(&ts, timeout))
                        return -EFAULT;
                if (copy_to_user(ts64, &ts, sizeof(ts)))
                        return -EFAULT;
        }
        return sys_semtimedop(semid, tsems, nsops, ts64);
}

This is most definitely broken if COMPAT_USE_64BIT_TIME, even with
get_compat_timespec() is replaced by compat_get_timespec().  However,
what is *really* going on here is that we want to provide a user space
pointer to a kernel-format timespec, so we could have an interface like
this:

int compat_convert_timespec_user(struct compat_timespec **ts64p,
const struct compat_timespec __user *ts)
{
        struct timespec __user *ts64;
        struct timespec ts;

	/* If the compat timespec is 64 bits, no conversion is needed */
	if (!ts || COMPAT_USE_64BIT_TIME) {
		*ts64p = (struct timespec __user *)ts;
		return 0;
	}

	*ts64p = ts64 = compat_alloc_user_space(sizeof(*ts64));
        if (__get_compat_timespec(&ts, timeout))
		return -EFAULT;
	if (copy_to_user(ts64, &ts, sizeof(ts)))
        	return -EFAULT;

	return 0;
}

Now one can argue we have a potential problem with type safety here, but
I'm not sure there is any way to avoid that.

	-hpa


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: x86, x32: Correct invalid use of user timespec in the kernel
  2014-01-31 18:45     ` Linus Torvalds
  2014-01-31 19:00       ` H. Peter Anvin
  2014-01-31 19:13       ` H. Peter Anvin
@ 2014-01-31 22:37       ` H. Peter Anvin
  2014-02-01 19:07         ` Linus Torvalds
  2 siblings, 1 reply; 7+ messages in thread
From: H. Peter Anvin @ 2014-01-31 22:37 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: Dave Jones, Linux Kernel Mailing List

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

On 01/31/2014 10:45 AM, Linus Torvalds wrote:
> On Fri, Jan 31, 2014 at 10:06 AM, H. Peter Anvin <hpa@linux.intel.com> wrote:
>>
>> My feeling is that {get,put}_compat_timespec() should at the very least
>> have leading underscores to flag it as a low-level function, but better
>> suggestions would be appreciated.
> 
> Why not just remove it entirely, and change all users to
> compat_[get|set]_timespec (same for timeval etc, of course).
> 
> After all, compat_*_time*() does fall back cleanly for non-x32 cases.
> And sure, maybe that particular code is never *needed* for x32
> support, but the overhead is generally zero (since in most cases X32
> isn't even configured), or very low anyway. So the upside of having
> two subtly incompatible interfaces is very dubious, no?
> 

Here is a patch for comments/review/opinions... it has only been
compile-tested.

	-hpa


[-- Attachment #2: diff --]
[-- Type: text/plain, Size: 15240 bytes --]

diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
index 8f7a6a4..6191968 100644
--- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
+++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
@@ -733,7 +733,7 @@ static int put_v4l2_event32(struct v4l2_event *kp, struct v4l2_event32 __user *u
 		copy_to_user(&up->u, &kp->u, sizeof(kp->u)) ||
 		put_user(kp->pending, &up->pending) ||
 		put_user(kp->sequence, &up->sequence) ||
-		put_compat_timespec(&kp->timestamp, &up->timestamp) ||
+		compat_put_timespec(&kp->timestamp, &up->timestamp) ||
 		put_user(kp->id, &up->id) ||
 		copy_to_user(up->reserved, kp->reserved, 8 * sizeof(__u32)))
 			return -EFAULT;
diff --git a/fs/compat.c b/fs/compat.c
index 6af20de..62092f4 100644
--- a/fs/compat.c
+++ b/fs/compat.c
@@ -92,8 +92,8 @@ asmlinkage long compat_sys_utimensat(unsigned int dfd, const char __user *filena
 	struct timespec tv[2];
 
 	if  (t) {
-		if (get_compat_timespec(&tv[0], &t[0]) ||
-		    get_compat_timespec(&tv[1], &t[1]))
+		if (compat_get_timespec(&tv[0], &t[0]) ||
+		    compat_get_timespec(&tv[1], &t[1]))
 			return -EFAULT;
 
 		if (tv[0].tv_nsec == UTIME_OMIT && tv[1].tv_nsec == UTIME_OMIT)
@@ -512,7 +512,7 @@ compat_sys_io_getevents(aio_context_t ctx_id,
 				nr * sizeof(struct io_event))))
 		goto out;
 	if (timeout) {
-		if (get_compat_timespec(&t, timeout))
+		if (compat_get_timespec(&t, timeout))
 			goto out;
 
 		ut = compat_alloc_user_space(sizeof(*ut));
diff --git a/include/linux/compat.h b/include/linux/compat.h
index 19f6003..db467cf 100644
--- a/include/linux/compat.h
+++ b/include/linux/compat.h
@@ -141,26 +141,23 @@ struct compat_sigaction {
 };
 
 /*
- * These functions operate strictly on struct compat_time*
- */
-extern int get_compat_timespec(struct timespec *,
-			       const struct compat_timespec __user *);
-extern int put_compat_timespec(const struct timespec *,
-			       struct compat_timespec __user *);
-extern int get_compat_timeval(struct timeval *,
-			      const struct compat_timeval __user *);
-extern int put_compat_timeval(const struct timeval *,
-			      struct compat_timeval __user *);
-/*
  * These functions operate on 32- or 64-bit specs depending on
- * COMPAT_USE_64BIT_TIME, hence the void user pointer arguments and the
- * naming as compat_get/put_ rather than get/put_compat_.
+ * COMPAT_USE_64BIT_TIME, hence the void user pointer arguments.
  */
 extern int compat_get_timespec(struct timespec *, const void __user *);
 extern int compat_put_timespec(const struct timespec *, void __user *);
 extern int compat_get_timeval(struct timeval *, const void __user *);
 extern int compat_put_timeval(const struct timeval *, void __user *);
 
+/*
+ * These functions convert a timeval/timespec if necessary and return
+ * a user space pointer.
+ */
+extern int compat_convert_timespec(const struct timespec __user **,
+				   const void __user *);
+extern int compat_convert_timeval(const struct timeval __user **,
+				  const void __user *);
+
 struct compat_iovec {
 	compat_uptr_t	iov_base;
 	compat_size_t	iov_len;
diff --git a/ipc/compat.c b/ipc/compat.c
index f486b00..1048522 100644
--- a/ipc/compat.c
+++ b/ipc/compat.c
@@ -752,14 +752,8 @@ long compat_sys_shmctl(int first, int second, void __user *uptr)
 long compat_sys_semtimedop(int semid, struct sembuf __user *tsems,
 		unsigned nsops, const struct compat_timespec __user *timeout)
 {
-	struct timespec __user *ts64 = NULL;
-	if (timeout) {
-		struct timespec ts;
-		ts64 = compat_alloc_user_space(sizeof(*ts64));
-		if (get_compat_timespec(&ts, timeout))
-			return -EFAULT;
-		if (copy_to_user(ts64, &ts, sizeof(ts)))
-			return -EFAULT;
-	}
+	struct timespec __user *ts64;
+	if (compat_convert_timespec(&ts64, timeout))
+		return -EFAULT;
 	return sys_semtimedop(semid, tsems, nsops, ts64);
 }
diff --git a/ipc/compat_mq.c b/ipc/compat_mq.c
index 63d7c6de..a9cf163 100644
--- a/ipc/compat_mq.c
+++ b/ipc/compat_mq.c
@@ -64,20 +64,6 @@ asmlinkage long compat_sys_mq_open(const char __user *u_name,
 	return sys_mq_open(u_name, oflag, mode, p);
 }
 
-static int compat_prepare_timeout(struct timespec __user **p,
-				  const struct compat_timespec __user *u)
-{
-	struct timespec ts;
-	if (!u) {
-		*p = NULL;
-		return 0;
-	}
-	*p = compat_alloc_user_space(sizeof(ts));
-	if (get_compat_timespec(&ts, u) || copy_to_user(*p, &ts, sizeof(ts)))
-		return -EFAULT;
-	return 0;
-}
-
 asmlinkage long compat_sys_mq_timedsend(mqd_t mqdes,
 			const char __user *u_msg_ptr,
 			size_t msg_len, unsigned int msg_prio,
@@ -85,7 +71,7 @@ asmlinkage long compat_sys_mq_timedsend(mqd_t mqdes,
 {
 	struct timespec __user *u_ts;
 
-	if (compat_prepare_timeout(&u_ts, u_abs_timeout))
+	if (compat_convert_timespec(&u_ts, u_abs_timeout))
 		return -EFAULT;
 
 	return sys_mq_timedsend(mqdes, u_msg_ptr, msg_len,
@@ -98,7 +84,8 @@ asmlinkage ssize_t compat_sys_mq_timedreceive(mqd_t mqdes,
 			const struct compat_timespec __user *u_abs_timeout)
 {
 	struct timespec __user *u_ts;
-	if (compat_prepare_timeout(&u_ts, u_abs_timeout))
+
+	if (compat_convert_timespec(&u_ts, u_abs_timeout))
 		return -EFAULT;
 
 	return sys_mq_timedreceive(mqdes, u_msg_ptr, msg_len,
diff --git a/kernel/compat.c b/kernel/compat.c
index 0a09e48..9533ba6 100644
--- a/kernel/compat.c
+++ b/kernel/compat.c
@@ -30,28 +30,6 @@
 
 #include <asm/uaccess.h>
 
-/*
- * Get/set struct timeval with struct timespec on the native side
- */
-static int compat_get_timeval_convert(struct timespec *o,
-				      struct compat_timeval __user *i)
-{
-	long usec;
-
-	if (get_user(o->tv_sec, &i->tv_sec) ||
-	    get_user(usec, &i->tv_usec))
-		return -EFAULT;
-	o->tv_nsec = usec * 1000;
-	return 0;
-}
-
-static int compat_put_timeval_convert(struct compat_timeval __user *o,
-				      struct timeval *i)
-{
-	return (put_user(i->tv_sec, &o->tv_sec) ||
-		put_user(i->tv_usec, &o->tv_usec)) ? -EFAULT : 0;
-}
-
 static int compat_get_timex(struct timex *txc, struct compat_timex __user *utp)
 {
 	memset(txc, 0, sizeof(struct timex));
@@ -116,7 +94,7 @@ asmlinkage long compat_sys_gettimeofday(struct compat_timeval __user *tv,
 	if (tv) {
 		struct timeval ktv;
 		do_gettimeofday(&ktv);
-		if (compat_put_timeval_convert(tv, &ktv))
+		if (compat_put_timeval(&ktv, tv))
 			return -EFAULT;
 	}
 	if (tz) {
@@ -130,59 +108,58 @@ asmlinkage long compat_sys_gettimeofday(struct compat_timeval __user *tv,
 asmlinkage long compat_sys_settimeofday(struct compat_timeval __user *tv,
 		struct timezone __user *tz)
 {
-	struct timespec kts;
-	struct timezone ktz;
+	struct timeval user_tv;
+	struct timespec	new_ts;
+	struct timezone new_tz;
 
 	if (tv) {
-		if (compat_get_timeval_convert(&kts, tv))
+		if (compat_get_timeval(&user_tv, tv))
 			return -EFAULT;
+		new_ts.tv_sec = user_tv.tv_sec;
+		new_ts.tv_nsec = user_tv.tv_usec * NSEC_PER_USEC;
 	}
 	if (tz) {
-		if (copy_from_user(&ktz, tz, sizeof(ktz)))
+		if (copy_from_user(&new_tz, tz, sizeof(*tz)))
 			return -EFAULT;
 	}
 
-	return do_sys_settimeofday(tv ? &kts : NULL, tz ? &ktz : NULL);
+	return do_sys_settimeofday(tv ? &new_ts : NULL, tz ? &new_tz : NULL);
 }
 
-int get_compat_timeval(struct timeval *tv, const struct compat_timeval __user *ctv)
+static int __compat_get_timeval(struct timeval *tv, const struct compat_timeval __user *ctv)
 {
 	return (!access_ok(VERIFY_READ, ctv, sizeof(*ctv)) ||
 			__get_user(tv->tv_sec, &ctv->tv_sec) ||
 			__get_user(tv->tv_usec, &ctv->tv_usec)) ? -EFAULT : 0;
 }
-EXPORT_SYMBOL_GPL(get_compat_timeval);
 
-int put_compat_timeval(const struct timeval *tv, struct compat_timeval __user *ctv)
+static int __compat_put_timeval(const struct timeval *tv, struct compat_timeval __user *ctv)
 {
 	return (!access_ok(VERIFY_WRITE, ctv, sizeof(*ctv)) ||
 			__put_user(tv->tv_sec, &ctv->tv_sec) ||
 			__put_user(tv->tv_usec, &ctv->tv_usec)) ? -EFAULT : 0;
 }
-EXPORT_SYMBOL_GPL(put_compat_timeval);
 
-int get_compat_timespec(struct timespec *ts, const struct compat_timespec __user *cts)
+static int __compat_get_timespec(struct timespec *ts, const struct compat_timespec __user *cts)
 {
 	return (!access_ok(VERIFY_READ, cts, sizeof(*cts)) ||
 			__get_user(ts->tv_sec, &cts->tv_sec) ||
 			__get_user(ts->tv_nsec, &cts->tv_nsec)) ? -EFAULT : 0;
 }
-EXPORT_SYMBOL_GPL(get_compat_timespec);
 
-int put_compat_timespec(const struct timespec *ts, struct compat_timespec __user *cts)
+static int __compat_put_timespec(const struct timespec *ts, struct compat_timespec __user *cts)
 {
 	return (!access_ok(VERIFY_WRITE, cts, sizeof(*cts)) ||
 			__put_user(ts->tv_sec, &cts->tv_sec) ||
 			__put_user(ts->tv_nsec, &cts->tv_nsec)) ? -EFAULT : 0;
 }
-EXPORT_SYMBOL_GPL(put_compat_timespec);
 
 int compat_get_timeval(struct timeval *tv, const void __user *utv)
 {
 	if (COMPAT_USE_64BIT_TIME)
 		return copy_from_user(tv, utv, sizeof *tv) ? -EFAULT : 0;
 	else
-		return get_compat_timeval(tv, utv);
+		return __compat_get_timeval(tv, utv);
 }
 EXPORT_SYMBOL_GPL(compat_get_timeval);
 
@@ -191,7 +168,7 @@ int compat_put_timeval(const struct timeval *tv, void __user *utv)
 	if (COMPAT_USE_64BIT_TIME)
 		return copy_to_user(utv, tv, sizeof *tv) ? -EFAULT : 0;
 	else
-		return put_compat_timeval(tv, utv);
+		return __compat_put_timeval(tv, utv);
 }
 EXPORT_SYMBOL_GPL(compat_put_timeval);
 
@@ -200,7 +177,7 @@ int compat_get_timespec(struct timespec *ts, const void __user *uts)
 	if (COMPAT_USE_64BIT_TIME)
 		return copy_from_user(ts, uts, sizeof *ts) ? -EFAULT : 0;
 	else
-		return get_compat_timespec(ts, uts);
+		return __compat_get_timespec(ts, uts);
 }
 EXPORT_SYMBOL_GPL(compat_get_timespec);
 
@@ -209,10 +186,56 @@ int compat_put_timespec(const struct timespec *ts, void __user *uts)
 	if (COMPAT_USE_64BIT_TIME)
 		return copy_to_user(uts, ts, sizeof *ts) ? -EFAULT : 0;
 	else
-		return put_compat_timespec(ts, uts);
+		return __compat_put_timespec(ts, uts);
 }
 EXPORT_SYMBOL_GPL(compat_put_timespec);
 
+int compat_convert_timespec(const struct timespec __user **kts,
+			    const void __user *cts)
+{
+	struct timespec ts;
+	struct timespec __user *uts;
+
+	if (!cts || COMPAT_USE_64BIT_TIME) {
+		*kts = cts;
+		return 0;
+	}
+
+	uts = compat_alloc_user_space(sizeof(ts));
+	if (!uts)
+		return -EFAULT;
+	if (compat_get_timespec(&ts, cts))
+		return -EFAULT;
+	if (copy_to_user(uts, &ts, sizeof(ts)))
+		return -EFAULT;
+
+	*kts = uts;
+	return 0;
+}
+
+int compat_convert_timeval(const struct timeval __user **ktv,
+			    const void __user *ctv)
+{
+	struct timeval tv;
+	struct timespec __user *utv;
+
+	if (!ctv || COMPAT_USE_64BIT_TIME) {
+		*ktv = ctv;
+		return 0;
+	}
+
+	utv = compat_alloc_user_space(sizeof(tv));
+	if (!utv)
+		return -EFAULT;
+	if (compat_get_timeval(&tv, ctv))
+		return -EFAULT;
+	if (copy_to_user(utv, &tv, sizeof(tv)))
+		return -EFAULT;
+
+	*ktv = utv;
+	return 0;
+}
+
 static long compat_nanosleep_restart(struct restart_block *restart)
 {
 	struct compat_timespec __user *rmtp;
@@ -229,7 +252,7 @@ static long compat_nanosleep_restart(struct restart_block *restart)
 	if (ret) {
 		rmtp = restart->nanosleep.compat_rmtp;
 
-		if (rmtp && put_compat_timespec(&rmt, rmtp))
+		if (rmtp && compat_put_timespec(&rmt, rmtp))
 			return -EFAULT;
 	}
 
@@ -243,7 +266,7 @@ asmlinkage long compat_sys_nanosleep(struct compat_timespec __user *rqtp,
 	mm_segment_t oldfs;
 	long ret;
 
-	if (get_compat_timespec(&tu, rqtp))
+	if (compat_get_timespec(&tu, rqtp))
 		return -EFAULT;
 
 	if (!timespec_valid(&tu))
@@ -263,7 +286,7 @@ asmlinkage long compat_sys_nanosleep(struct compat_timespec __user *rqtp,
 		restart->fn = compat_nanosleep_restart;
 		restart->nanosleep.compat_rmtp = rmtp;
 
-		if (rmtp && put_compat_timespec(&rmt, rmtp))
+		if (rmtp && compat_put_timespec(&rmt, rmtp))
 			return -EFAULT;
 	}
 
@@ -647,8 +670,8 @@ asmlinkage long compat_sys_sched_getaffinity(compat_pid_t pid, unsigned int len,
 int get_compat_itimerspec(struct itimerspec *dst,
 			  const struct compat_itimerspec __user *src)
 {
-	if (get_compat_timespec(&dst->it_interval, &src->it_interval) ||
-	    get_compat_timespec(&dst->it_value, &src->it_value))
+	if (__compat_get_timespec(&dst->it_interval, &src->it_interval) ||
+	    __compat_get_timespec(&dst->it_value, &src->it_value))
 		return -EFAULT;
 	return 0;
 }
@@ -656,8 +679,8 @@ int get_compat_itimerspec(struct itimerspec *dst,
 int put_compat_itimerspec(struct compat_itimerspec __user *dst,
 			  const struct itimerspec *src)
 {
-	if (put_compat_timespec(&src->it_interval, &dst->it_interval) ||
-	    put_compat_timespec(&src->it_value, &dst->it_value))
+	if (__compat_put_timespec(&src->it_interval, &dst->it_interval) ||
+	    __compat_put_timespec(&src->it_value, &dst->it_value))
 		return -EFAULT;
 	return 0;
 }
@@ -727,7 +750,7 @@ long compat_sys_clock_settime(clockid_t which_clock,
 	mm_segment_t oldfs;
 	struct timespec ts;
 
-	if (get_compat_timespec(&ts, tp))
+	if (compat_get_timespec(&ts, tp))
 		return -EFAULT;
 	oldfs = get_fs();
 	set_fs(KERNEL_DS);
@@ -749,7 +772,7 @@ long compat_sys_clock_gettime(clockid_t which_clock,
 	err = sys_clock_gettime(which_clock,
 				(struct timespec __user *) &ts);
 	set_fs(oldfs);
-	if (!err && put_compat_timespec(&ts, tp))
+	if (!err && compat_put_timespec(&ts, tp))
 		return -EFAULT;
 	return err;
 }
@@ -789,7 +812,7 @@ long compat_sys_clock_getres(clockid_t which_clock,
 	err = sys_clock_getres(which_clock,
 			       (struct timespec __user *) &ts);
 	set_fs(oldfs);
-	if (!err && tp && put_compat_timespec(&ts, tp))
+	if (!err && tp && compat_put_timespec(&ts, tp))
 		return -EFAULT;
 	return err;
 }
@@ -808,7 +831,7 @@ static long compat_clock_nanosleep_restart(struct restart_block *restart)
 	set_fs(oldfs);
 
 	if ((err == -ERESTART_RESTARTBLOCK) && rmtp &&
-	    put_compat_timespec(&tu, rmtp))
+	    compat_put_timespec(&tu, rmtp))
 		return -EFAULT;
 
 	if (err == -ERESTART_RESTARTBLOCK) {
@@ -827,7 +850,7 @@ long compat_sys_clock_nanosleep(clockid_t which_clock, int flags,
 	struct timespec in, out;
 	struct restart_block *restart;
 
-	if (get_compat_timespec(&in, rqtp))
+	if (compat_get_timespec(&in, rqtp))
 		return -EFAULT;
 
 	oldfs = get_fs();
@@ -838,7 +861,7 @@ long compat_sys_clock_nanosleep(clockid_t which_clock, int flags,
 	set_fs(oldfs);
 
 	if ((err == -ERESTART_RESTARTBLOCK) && rmtp &&
-	    put_compat_timespec(&out, rmtp))
+	    compat_put_timespec(&out, rmtp))
 		return -EFAULT;
 
 	if (err == -ERESTART_RESTARTBLOCK) {
@@ -1130,7 +1153,7 @@ COMPAT_SYSCALL_DEFINE2(sched_rr_get_interval,
 	set_fs(KERNEL_DS);
 	ret = sys_sched_rr_get_interval(pid, (struct timespec __user *)&t);
 	set_fs(old_fs);
-	if (put_compat_timespec(&t, interval))
+	if (compat_put_timespec(&t, interval))
 		return -EFAULT;
 	return ret;
 }
diff --git a/kernel/futex_compat.c b/kernel/futex_compat.c
index f9f44fd..55c8c93 100644
--- a/kernel/futex_compat.c
+++ b/kernel/futex_compat.c
@@ -183,7 +183,7 @@ COMPAT_SYSCALL_DEFINE6(futex, u32 __user *, uaddr, int, op, u32, val,
 	if (utime && (cmd == FUTEX_WAIT || cmd == FUTEX_LOCK_PI ||
 		      cmd == FUTEX_WAIT_BITSET ||
 		      cmd == FUTEX_WAIT_REQUEUE_PI)) {
-		if (get_compat_timespec(&ts, utime))
+		if (compat_get_timespec(&ts, utime))
 			return -EFAULT;
 		if (!timespec_valid(&ts))
 			return -EINVAL;

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: x86, x32: Correct invalid use of user timespec in the kernel
  2014-01-31 22:37       ` H. Peter Anvin
@ 2014-02-01 19:07         ` Linus Torvalds
  0 siblings, 0 replies; 7+ messages in thread
From: Linus Torvalds @ 2014-02-01 19:07 UTC (permalink / raw)
  To: H. Peter Anvin; +Cc: Dave Jones, Linux Kernel Mailing List

On Fri, Jan 31, 2014 at 2:37 PM, H. Peter Anvin <hpa@linux.intel.com> wrote:
>
> Here is a patch for comments/review/opinions... it has only been
> compile-tested.

Looks sane to me from a superficial quick read-through.

             Linus

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2014-02-01 19:07 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20140131025453.B594B660CA3@gitolite.kernel.org>
2014-01-31 17:50 ` x86, x32: Correct invalid use of user timespec in the kernel Dave Jones
2014-01-31 18:06   ` H. Peter Anvin
2014-01-31 18:45     ` Linus Torvalds
2014-01-31 19:00       ` H. Peter Anvin
2014-01-31 19:13       ` H. Peter Anvin
2014-01-31 22:37       ` H. Peter Anvin
2014-02-01 19:07         ` Linus Torvalds

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).