[PATCH 1/2] alpha: osf_sys.c: fix put_tv32 regression

[PATCH 1/2] alpha: osf_sys.c: fix put_tv32 regression

[Arnd Bergmann]

There was a typo in the new version of put_tv32() that caused
uninitialized stack data to be written back to user space, rather
than writing the actual timeval for the emulation of
gettimeofday(), wait4(), usleep_thread() and old_adjtimex().

This fixes it to write the correct data again.

Cc: stable@vger.kernel.org
Fixes: 1cc6c4635e9f ("osf_sys.c: switch handling of timeval32/itimerval32 to copy_{to,from}_user()")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 arch/alpha/kernel/osf_sys.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/alpha/kernel/osf_sys.c b/arch/alpha/kernel/osf_sys.c
index ce3a675c0c4b..75a5c35a2067 100644
--- a/arch/alpha/kernel/osf_sys.c
+++ b/arch/alpha/kernel/osf_sys.c
@@ -964,8 +964,8 @@ static inline long
 put_tv32(struct timeval32 __user *o, struct timeval *i)
 {
 	return copy_to_user(o, &(struct timeval32){
-				.tv_sec = o->tv_sec,
-				.tv_usec = o->tv_usec},
+				.tv_sec = i->tv_sec,
+				.tv_usec = i->tv_usec},
 			    sizeof(struct timeval32));
 }
 
-- 
2.9.0

[PATCH 2/2] alpha: osf_sys.c: use timespec64 where appropriate

[Arnd Bergmann]

Some of the syscall helper functions (do_utimes, poll_select_set_timeout,
core_sys_select) have changed over the past year or two to use
'timespec64' pointers rather than 'timespec'. This was fine on alpha,
since 64-bit architectures treat the two as the same type.

However, I'd like to change that behavior and make 'timespec64' a proper
type of its own even on 64-bit architectures, and that will introduce
harmless type mismatch warnings here.

Also, I'm trying to kill off the do_gettimeofday() helper in favor of
ktime_get() and related interfaces throughout the kernel.

This changes the get_tv32/put_tv32 helper functions to also take a
timespec64 argument rather than timeval, which allows us to simplify
some of the syscall helpers a bit and avoid the type warnings.

For the moment, wait4 and adjtimex are still better off with the old
behavior, so I'm adding a special put_tv_to_tv32() helper for those.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 arch/alpha/kernel/osf_sys.c | 68 ++++++++++++++++++++++-----------------------
 1 file changed, 34 insertions(+), 34 deletions(-)

diff --git a/arch/alpha/kernel/osf_sys.c b/arch/alpha/kernel/osf_sys.c
index 75a5c35a2067..6db6ec30c3de 100644
--- a/arch/alpha/kernel/osf_sys.c
+++ b/arch/alpha/kernel/osf_sys.c
@@ -950,18 +950,27 @@ struct itimerval32
 };
 
 static inline long
-get_tv32(struct timeval *o, struct timeval32 __user *i)
+get_tv32(struct timespec64 *o, struct timeval32 __user *i)
 {
 	struct timeval32 tv;
 	if (copy_from_user(&tv, i, sizeof(struct timeval32)))
 		return -EFAULT;
 	o->tv_sec = tv.tv_sec;
-	o->tv_usec = tv.tv_usec;
+	o->tv_nsec = tv.tv_usec * NSEC_PER_USEC;
 	return 0;
 }
 
 static inline long
-put_tv32(struct timeval32 __user *o, struct timeval *i)
+put_tv32(struct timeval32 __user *o, struct timespec64 *i)
+{
+	return copy_to_user(o, &(struct timeval32){
+				.tv_sec = i->tv_sec,
+				.tv_usec = i->tv_nsec / NSEC_PER_USEC},
+			    sizeof(struct timeval32));
+}
+
+static inline long
+put_tv_to_tv32(struct timeval32 __user *o, struct timeval *i)
 {
 	return copy_to_user(o, &(struct timeval32){
 				.tv_sec = i->tv_sec,
@@ -1004,9 +1013,10 @@ SYSCALL_DEFINE2(osf_gettimeofday, struct timeval32 __user *, tv,
 		struct timezone __user *, tz)
 {
 	if (tv) {
-		struct timeval ktv;
-		do_gettimeofday(&ktv);
-		if (put_tv32(tv, &ktv))
+		struct timespec64 kts;
+
+		ktime_get_ts64(&kts);
+		if (put_tv32(tv, &kts))
 			return -EFAULT;
 	}
 	if (tz) {
@@ -1019,22 +1029,19 @@ SYSCALL_DEFINE2(osf_gettimeofday, struct timeval32 __user *, tv,
 SYSCALL_DEFINE2(osf_settimeofday, struct timeval32 __user *, tv,
 		struct timezone __user *, tz)
 {
-	struct timespec64 kts64;
-	struct timespec kts;
+	struct timespec64 kts;
 	struct timezone ktz;
 
  	if (tv) {
-		if (get_tv32((struct timeval *)&kts, tv))
+		if (get_tv32(&kts, tv))
 			return -EFAULT;
-		kts.tv_nsec *= 1000;
-		kts64 = timespec_to_timespec64(kts);
 	}
 	if (tz) {
 		if (copy_from_user(&ktz, tz, sizeof(*tz)))
 			return -EFAULT;
 	}
 
-	return do_sys_settimeofday64(tv ? &kts64 : NULL, tz ? &ktz : NULL);
+	return do_sys_settimeofday64(tv ? &kts : NULL, tz ? &ktz : NULL);
 }
 
 asmlinkage long sys_ni_posix_timers(void);
@@ -1083,22 +1090,16 @@ SYSCALL_DEFINE3(osf_setitimer, int, which, struct itimerval32 __user *, in,
 SYSCALL_DEFINE2(osf_utimes, const char __user *, filename,
 		struct timeval32 __user *, tvs)
 {
-	struct timespec tv[2];
+	struct timespec64 tv[2];
 
 	if (tvs) {
-		struct timeval ktvs[2];
-		if (get_tv32(&ktvs[0], &tvs[0]) ||
-		    get_tv32(&ktvs[1], &tvs[1]))
+		if (get_tv32(&tv[0], &tvs[0]) ||
+		    get_tv32(&tv[1], &tvs[1]))
 			return -EFAULT;
 
-		if (ktvs[0].tv_usec < 0 || ktvs[0].tv_usec >= 1000000 ||
-		    ktvs[1].tv_usec < 0 || ktvs[1].tv_usec >= 1000000)
+		if (tv[0].tv_nsec < 0 || tv[0].tv_nsec >= 1000000000 ||
+		    tv[1].tv_nsec < 0 || tv[1].tv_nsec >= 1000000000)
 			return -EINVAL;
-
-		tv[0].tv_sec = ktvs[0].tv_sec;
-		tv[0].tv_nsec = 1000 * ktvs[0].tv_usec;
-		tv[1].tv_sec = ktvs[1].tv_sec;
-		tv[1].tv_nsec = 1000 * ktvs[1].tv_usec;
 	}
 
 	return do_utimes(AT_FDCWD, filename, tvs ? tv : NULL, 0);
@@ -1107,19 +1108,18 @@ SYSCALL_DEFINE2(osf_utimes, const char __user *, filename,
 SYSCALL_DEFINE5(osf_select, int, n, fd_set __user *, inp, fd_set __user *, outp,
 		fd_set __user *, exp, struct timeval32 __user *, tvp)
 {
-	struct timespec end_time, *to = NULL;
+	struct timespec64 end_time, *to = NULL;
 	if (tvp) {
-		struct timeval tv;
+		struct timespec64 tv;
 		to = &end_time;
 
 		if (get_tv32(&tv, tvp))
 		    	return -EFAULT;
 
-		if (tv.tv_sec < 0 || tv.tv_usec < 0)
+		if (tv.tv_sec < 0 || tv.tv_nsec < 0)
 			return -EINVAL;
 
-		if (poll_select_set_timeout(to, tv.tv_sec,
-					    tv.tv_usec * NSEC_PER_USEC))
+		if (poll_select_set_timeout(to, tv.tv_sec, tv.tv_nsec))
 			return -EINVAL;		
 
 	}
@@ -1192,9 +1192,9 @@ SYSCALL_DEFINE4(osf_wait4, pid_t, pid, int __user *, ustatus, int, options,
 		return -EFAULT;
 	if (!ur)
 		return err;
-	if (put_tv32(&ur->ru_utime, &r.ru_utime))
+	if (put_tv_to_tv32(&ur->ru_utime, &r.ru_utime))
 		return -EFAULT;
-	if (put_tv32(&ur->ru_stime, &r.ru_stime))
+	if (put_tv_to_tv32(&ur->ru_stime, &r.ru_stime))
 		return -EFAULT;
 	if (copy_to_user(&ur->ru_maxrss, &r.ru_maxrss,
 	      sizeof(struct rusage32) - offsetof(struct rusage32, ru_maxrss)))
@@ -1210,18 +1210,18 @@ SYSCALL_DEFINE4(osf_wait4, pid_t, pid, int __user *, ustatus, int, options,
 SYSCALL_DEFINE2(osf_usleep_thread, struct timeval32 __user *, sleep,
 		struct timeval32 __user *, remain)
 {
-	struct timeval tmp;
+	struct timespec64 tmp;
 	unsigned long ticks;
 
 	if (get_tv32(&tmp, sleep))
 		goto fault;
 
-	ticks = timeval_to_jiffies(&tmp);
+	ticks = timespec64_to_jiffies(&tmp);
 
 	ticks = schedule_timeout_interruptible(ticks);
 
 	if (remain) {
-		jiffies_to_timeval(ticks, &tmp);
+		jiffies_to_timespec64(ticks, &tmp);
 		if (put_tv32(remain, &tmp))
 			goto fault;
 	}
@@ -1280,7 +1280,7 @@ SYSCALL_DEFINE1(old_adjtimex, struct timex32 __user *, txc_p)
 	if (copy_to_user(txc_p, &txc, offsetof(struct timex32, time)) ||
 	    (copy_to_user(&txc_p->tick, &txc.tick, sizeof(struct timex32) - 
 			  offsetof(struct timex32, tick))) ||
-	    (put_tv32(&txc_p->time, &txc.time)))
+	    (put_tv_to_tv32(&txc_p->time, &txc.time)))
 	  return -EFAULT;
 
 	return ret;
-- 
2.9.0

Re: [Y2038] [PATCH 2/2] alpha: osf_sys.c: use timespec64 where appropriate

[Ben Hutchings]

On Tue, 2017-11-07 at 15:09 +0100, Arnd Bergmann wrote:
> Some of the syscall helper functions (do_utimes, poll_select_set_timeout,
> core_sys_select) have changed over the past year or two to use
> 'timespec64' pointers rather than 'timespec'. This was fine on alpha,
> since 64-bit architectures treat the two as the same type.
> 
> However, I'd like to change that behavior and make 'timespec64' a proper
> type of its own even on 64-bit architectures, and that will introduce
> harmless type mismatch warnings here.
> 
> Also, I'm trying to kill off the do_gettimeofday() helper in favor of
> ktime_get() and related interfaces throughout the kernel.
[...]
> @@ -1004,9 +1013,10 @@ SYSCALL_DEFINE2(osf_gettimeofday, struct timeval32 __user *, tv,
>  		struct timezone __user *, tz)
>  {
>  	if (tv) {
> -		struct timeval ktv;
> -		do_gettimeofday(&ktv);
> -		if (put_tv32(tv, &ktv))
> +		struct timespec64 kts;
> +
> +		ktime_get_ts64(&kts);
[...]

But this syscall is supposed to use the realtime clock, no?  It seems
like the correct substitute here is getnstimeofday64() (as the kernel-
doc comment for do_gettimeofday() *almost* says).

Ben.

-- 
Ben Hutchings
Software Developer, Codethink Ltd.

Re: [Y2038] [PATCH 2/2] alpha: osf_sys.c: use timespec64 where appropriate

[Arnd Bergmann]

On Wed, Nov 8, 2017 at 1:22 AM, Ben Hutchings
<ben.hutchings@codethink.co.uk> wrote:
> On Tue, 2017-11-07 at 15:09 +0100, Arnd Bergmann wrote:
>> Some of the syscall helper functions (do_utimes, poll_select_set_timeout,
>> core_sys_select) have changed over the past year or two to use
>> 'timespec64' pointers rather than 'timespec'. This was fine on alpha,
>> since 64-bit architectures treat the two as the same type.
>>
>> However, I'd like to change that behavior and make 'timespec64' a proper
>> type of its own even on 64-bit architectures, and that will introduce
>> harmless type mismatch warnings here.
>>
>> Also, I'm trying to kill off the do_gettimeofday() helper in favor of
>> ktime_get() and related interfaces throughout the kernel.
> [...]
>> @@ -1004,9 +1013,10 @@ SYSCALL_DEFINE2(osf_gettimeofday, struct timeval32 __user *, tv,
>>               struct timezone __user *, tz)
>>  {
>>       if (tv) {
>> -             struct timeval ktv;
>> -             do_gettimeofday(&ktv);
>> -             if (put_tv32(tv, &ktv))
>> +             struct timespec64 kts;
>> +
>> +             ktime_get_ts64(&kts);
> [...]
>
> But this syscall is supposed to use the realtime clock, no?  It seems
> like the correct substitute here is getnstimeofday64() (as the kernel-
> doc comment for do_gettimeofday() *almost* says).

It should be ktime_get_real_ts64(), thanks for noticing my mistake.

ktime_get_real_ts64() is the same as getnstimeofday64(), but I'm
trying to use the ktime_get* naming where possible.

        Arnd

Re: [PATCH 1/2] alpha: osf_sys.c: fix put_tv32 regression

[Al Viro]

On Tue, Nov 07, 2017 at 03:09:24PM +0100, Arnd Bergmann wrote:
> There was a typo in the new version of put_tv32() that caused
> uninitialized stack data to be written back to user space, rather
> than writing the actual timeval for the emulation of
> gettimeofday(), wait4(), usleep_thread() and old_adjtimex().
> 
> This fixes it to write the correct data again.

*blink*

the bug is real, all right, and the fix is correct one, but where
do you get an infoleak?  What it is is a user-triggerable oops -
just pass it an unmapped address.  For anything mapped r/w it's
simply a no-op - userland data is unchanged.

IOW, the fix is correct, but commit message isn't - it's

"user-triggerable oops and in all cases failed to modify userland timeval32"

not

"uninitialized stack data to be written back to user space"

Re: [PATCH 1/2] alpha: osf_sys.c: fix put_tv32 regression

[Arnd Bergmann]

On Tue, Nov 7, 2017 at 4:52 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Tue, Nov 07, 2017 at 03:09:24PM +0100, Arnd Bergmann wrote:
>> There was a typo in the new version of put_tv32() that caused
>> uninitialized stack data to be written back to user space, rather
>> than writing the actual timeval for the emulation of
>> gettimeofday(), wait4(), usleep_thread() and old_adjtimex().
>>
>> This fixes it to write the correct data again.
>
> *blink*
>
> the bug is real, all right, and the fix is correct one, but where
> do you get an infoleak?  What it is is a user-triggerable oops -
> just pass it an unmapped address.  For anything mapped r/w it's
> simply a no-op - userland data is unchanged.
>
> IOW, the fix is correct, but commit message isn't - it's
>
> "user-triggerable oops and in all cases failed to modify userland timeval32"
>
> not
>
> "uninitialized stack data to be written back to user space"

Ah right, sorry about that. I misread the statement as setting
the temporary structure to itself rather than setting it to the contents
of the user structure.

Do you want to update the description as suggested and forward it,
or should I send a fixed version? I'm about to leave the office for
today, so I'd have to do it tomorrow then.

       Arnd