diff mbox series

[17/23] y2038: time: avoid timespec usage in settimeofday()

Message ID 20191108211323.1806194-8-arnd@arndb.de
State Accepted
Commit 5e0fb1b57bea8d11fe77da2bc80f4c9a67e28318
Headers show
Series y2038 cleanups | expand

Commit Message

Arnd Bergmann Nov. 8, 2019, 9:12 p.m. UTC 
The compat_get_timeval() and timeval_valid() interfaces
are deprecated and getting removed along with the definition
of struct timeval itself.

Change the two implementations of the settimeofday()
system call to open-code these helpers and completely
avoid references to timeval.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>

---
 include/linux/syscalls.h |  2 +-
 kernel/time/time.c       | 20 +++++++++-----------
 2 files changed, 10 insertions(+), 12 deletions(-)

-- 
2.20.0

Comments

Thomas Gleixner Nov. 13, 2019, 9:53 p.m. UTC | #1 
On Fri, 8 Nov 2019, Arnd Bergmann wrote:
> -SYSCALL_DEFINE2(settimeofday, struct timeval __user *, tv,

> +SYSCALL_DEFINE2(settimeofday, struct __kernel_old_timeval __user *, tv,

>  		struct timezone __user *, tz)

>  {

>  	struct timespec64 new_ts;

> -	struct timeval user_tv;

>  	struct timezone new_tz;

>  

>  	if (tv) {

> -		if (copy_from_user(&user_tv, tv, sizeof(*tv)))

> +		if (get_user(new_ts.tv_sec, &tv->tv_sec) ||

> +		    get_user(new_ts.tv_nsec, &tv->tv_usec))

>  			return -EFAULT;


How is that supposed to be correct on a 32bit kernel?

>  

> -		if (!timeval_valid(&user_tv))

> +		if (tv->tv_usec > USEC_PER_SEC)

>  			return -EINVAL;


That's incomplete:

static inline bool timeval_valid(const struct timeval *tv)
{
        /* Dates before 1970 are bogus */
        if (tv->tv_sec < 0)
                return false;

	/* Can't have more microseconds then a second */
        if (tv->tv_usec < 0 || tv->tv_usec >= USEC_PER_SEC)
                return false;

        return true;
}


>  

> -		new_ts.tv_sec = user_tv.tv_sec;

> -		new_ts.tv_nsec = user_tv.tv_usec * NSEC_PER_USEC;

> +		new_ts.tv_nsec *= NSEC_PER_USEC;

>  	}

>  	if (tz) {

>  		if (copy_from_user(&new_tz, tz, sizeof(*tz)))

> @@ -245,18 +244,17 @@ COMPAT_SYSCALL_DEFINE2(settimeofday, struct old_timeval32 __user *, tv,

>  		       struct timezone __user *, tz)

>  {

>  	struct timespec64 new_ts;

> -	struct timeval user_tv;

>  	struct timezone new_tz;

>  

>  	if (tv) {

> -		if (compat_get_timeval(&user_tv, tv))

> +		if (get_user(new_ts.tv_sec, &tv->tv_sec) ||

> +		    get_user(new_ts.tv_nsec, &tv->tv_usec))

>  			return -EFAULT;

>  

> -		if (!timeval_valid(&user_tv))

> +		if (new_ts.tv_nsec > USEC_PER_SEC)

>  			return -EINVAL;


Ditto.

Thanks,

	tglx
Arnd Bergmann Nov. 14, 2019, 11:06 a.m. UTC | #2 
On Wed, Nov 13, 2019 at 10:53 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>

> On Fri, 8 Nov 2019, Arnd Bergmann wrote:

> > -SYSCALL_DEFINE2(settimeofday, struct timeval __user *, tv,

> > +SYSCALL_DEFINE2(settimeofday, struct __kernel_old_timeval __user *, tv,

> >               struct timezone __user *, tz)

> >  {

> >       struct timespec64 new_ts;

> > -     struct timeval user_tv;

> >       struct timezone new_tz;

> >

> >       if (tv) {

> > -             if (copy_from_user(&user_tv, tv, sizeof(*tv)))

> > +             if (get_user(new_ts.tv_sec, &tv->tv_sec) ||

> > +                 get_user(new_ts.tv_nsec, &tv->tv_usec))

> >                       return -EFAULT;

>

> How is that supposed to be correct on a 32bit kernel?


I don't see the problem you are referring to. This should behave the
same way on a 32-bit kernel and on a 64-bit kernel, sign-extending
the tv_sec field, and copying the user tv_usec field into the
kernel tv_nsec, to be multiplied by 1000 a few lines later.

Am I missing something?

> > -             if (!timeval_valid(&user_tv))

> > +             if (tv->tv_usec > USEC_PER_SEC)

> >                       return -EINVAL;

>

> That's incomplete:

>

> static inline bool timeval_valid(const struct timeval *tv)

> {

>         /* Dates before 1970 are bogus */

>         if (tv->tv_sec < 0)

>                 return false;

>

>         /* Can't have more microseconds then a second */

>         if (tv->tv_usec < 0 || tv->tv_usec >= USEC_PER_SEC)

>                 return false;

>

>         return true;

> }


My idea was to not duplicate the range check that is done
in do_sys_settimeofday64() and again in do_settimeofday64:

        if (!timespec64_valid_settod(ts))
                return -EINVAL;

The only check we should need in addition to this is to ensure
that passing an invalid tv_usec number doesn't become an
unexpectedly valid tv_nsec after the multiplication.

I agree the patch looks like I'm missing a check here, but
the code after the patch appears clear enough to me.

          Arnd
Thomas Gleixner Nov. 14, 2019, 2:04 p.m. UTC | #3 
On Thu, 14 Nov 2019, Arnd Bergmann wrote:
> On Wed, Nov 13, 2019 at 10:53 PM Thomas Gleixner <tglx@linutronix.de> wrote:

> >

> > On Fri, 8 Nov 2019, Arnd Bergmann wrote:

> > > -SYSCALL_DEFINE2(settimeofday, struct timeval __user *, tv,

> > > +SYSCALL_DEFINE2(settimeofday, struct __kernel_old_timeval __user *, tv,

> > >               struct timezone __user *, tz)

> > >  {

> > >       struct timespec64 new_ts;

> > > -     struct timeval user_tv;

> > >       struct timezone new_tz;

> > >

> > >       if (tv) {

> > > -             if (copy_from_user(&user_tv, tv, sizeof(*tv)))

> > > +             if (get_user(new_ts.tv_sec, &tv->tv_sec) ||

> > > +                 get_user(new_ts.tv_nsec, &tv->tv_usec))

> > >                       return -EFAULT;

> >

> > How is that supposed to be correct on a 32bit kernel?

> 

> I don't see the problem you are referring to. This should behave the

> same way on a 32-bit kernel and on a 64-bit kernel, sign-extending

> the tv_sec field, and copying the user tv_usec field into the

> kernel tv_nsec, to be multiplied by 1000 a few lines later.


You're right. Tired brain failed to see the implicit sign extension in
get_user().

> Am I missing something?


No.

> > > -             if (!timeval_valid(&user_tv))

> > > +             if (tv->tv_usec > USEC_PER_SEC)

> > >                       return -EINVAL;

> >

> > That's incomplete:

> >

> > static inline bool timeval_valid(const struct timeval *tv)

> > {

> >         /* Dates before 1970 are bogus */

> >         if (tv->tv_sec < 0)

> >                 return false;

> >

> >         /* Can't have more microseconds then a second */

> >         if (tv->tv_usec < 0 || tv->tv_usec >= USEC_PER_SEC)

> >                 return false;

> >

> >         return true;

> > }

> 

> My idea was to not duplicate the range check that is done

> in do_sys_settimeofday64() and again in do_settimeofday64:

> 

>         if (!timespec64_valid_settod(ts))

>                 return -EINVAL;

> 

> The only check we should need in addition to this is to ensure

> that passing an invalid tv_usec number doesn't become an

> unexpectedly valid tv_nsec after the multiplication.


Right, but please add a proper comment as you/we are going to scratch heads
4 weeks from now when staring at that check and wondering why it is
incomplete.

Thanks,

	tglx
Arnd Bergmann Nov. 14, 2019, 2:35 p.m. UTC | #4 
On Thu, Nov 14, 2019 at 3:04 PM Thomas Gleixner <tglx@linutronix.de> wrote:
> On Thu, 14 Nov 2019, Arnd Bergmann wrote:

> > On Wed, Nov 13, 2019 at 10:53 PM Thomas Gleixner <tglx@linutronix.de> wrote:

> >

> > My idea was to not duplicate the range check that is done

> > in do_sys_settimeofday64() and again in do_settimeofday64:

> >

> >         if (!timespec64_valid_settod(ts))

> >                 return -EINVAL;

> >

> > The only check we should need in addition to this is to ensure

> > that passing an invalid tv_usec number doesn't become an

> > unexpectedly valid tv_nsec after the multiplication.

>

> Right, but please add a proper comment as you/we are going to scratch heads

> 4 weeks from now when staring at that check and wondering why it is

> incomplete.


Ok, done. I had just uploaded the branch with the fixup for
the __user pointer access in the same patch, but that version
had introduced another typo. I hope the version I uploaded
now has all known issues addressed for tomorrow's linux-next.

      Arnd
Abel Vesa Nov. 14, 2019, 11:01 p.m. UTC | #5 
On 19-11-08 22:12:16, Arnd Bergmann wrote:
> The compat_get_timeval() and timeval_valid() interfaces

> are deprecated and getting removed along with the definition

> of struct timeval itself.

> 

> Change the two implementations of the settimeofday()

> system call to open-code these helpers and completely

> avoid references to timeval.

> 


I get the following rcu stalls due to this patch on riscv64 (on qemu):

	[root@riscv ~]# uname -a
	Linux riscv 5.4.0-rc6-00018-gadde74306a4b #112 SMP Fri Nov 15 00:46:20 EET 2019 riscv64 riscv64 riscv64 GNU/Linux
	[root@riscv ~]# [  420.135710] rcu: INFO: rcu_sched self-detected stall
	on CPU
	[  420.136839] rcu:     3-....: (99702 ticks this GP) idle=482/1/0x4000000000000002 softirq=3322/3322 fqs=48784 
	[  420.138917]  (t=99768 jiffies g=4985 q=8343)
	[  420.139772] Task dump for CPU 3:
	[  420.140236] rdate           R  running task        0   254      1 0x00000008
	[  420.142226] Call Trace:
	[  420.142791] [<ffffffe000037954>] walk_stackframe+0x0/0xa6
	[  420.143911] [<ffffffe000037aba>] show_stack+0x2a/0x34
	[  420.145010] [<ffffffe0000569c8>] sched_show_task+0xf0/0x116
	[  420.145996] [<ffffffe00005b502>] dump_cpu_task+0x3e/0x48
	[  420.147073] [<ffffffe000084e5e>] rcu_dump_cpu_stacks+0x7c/0xb4
	[  420.148243] [<ffffffe0000842f6>] rcu_sched_clock_irq+0x3d6/0x582
	[  420.149349] [<ffffffe0000897b4>] update_process_times+0x1e/0x42
	[  420.150306] [<ffffffe000093a34>] tick_sched_handle.isra.0+0x2a/0x3a
	[  420.150997] [<ffffffe000093ce8>] tick_sched_timer+0x4e/0x92
	[  420.151603] [<ffffffe000089eb6>] __hrtimer_run_queues+0xae/0x108
	[  420.152639] [<ffffffe00008a5ac>] hrtimer_interrupt+0xca/0x1d4
	[  420.153629] [<ffffffe0004de564>] riscv_timer_interrupt+0x32/0x3a
	[  420.154629] [<ffffffe000612ad4>] do_IRQ+0xa4/0xb8
	[  420.155294] [<ffffffe000036814>] ret_from_exception+0x0/0xc
	[  420.156073] [<ffffffe000036814>] ret_from_exception+0x0/0xc
	[  451.556189] rcu: INFO: rcu_sched detected expedited stalls on CPUs/tasks: { 3-... } 100725 jiffies s: 53 root: 0x8/.
	[  451.558689] rcu: blocking rcu_node structures:
	[  451.559501] Task dump for CPU 3:
	[  451.560518] rdate           R  running task        0   254      1 0x00000008
	[  451.561396] Call Trace:
	[  451.561675] [<ffffffe00060ed48>] __schedule+0x158/0x36a
	[  483.147733] rcu: INFO: rcu_sched self-detected stall on CPU
	[  483.148723] rcu:     3-....: (115448 ticks this GP) idle=482/1/0x4000000000000002 softirq=3322/3322 fqs=56510 
	[  483.150220]  (t=115521 jiffies g=4985 q=8400)
	[  483.150885] Task dump for CPU 3:
	[  483.151392] rdate           R  running task        0   254      1 0x00000008
	[  483.152321] Call Trace:
	[  483.152755] [<ffffffe000037954>] walk_stackframe+0x0/0xa6
	[  483.153600] [<ffffffe000037aba>] show_stack+0x2a/0x34
	[  483.154428] [<ffffffe0000569c8>] sched_show_task+0xf0/0x116
	[  483.155325] [<ffffffe00005b502>] dump_cpu_task+0x3e/0x48
	[  483.156199] [<ffffffe000084e5e>] rcu_dump_cpu_stacks+0x7c/0xb4
	[  483.157163] [<ffffffe0000842f6>] rcu_sched_clock_irq+0x3d6/0x582
	[  483.158166] [<ffffffe0000897b4>] update_process_times+0x1e/0x42
	[  483.159257] [<ffffffe000093a34>] tick_sched_handle.isra.0+0x2a/0x3a
	[  483.160240] [<ffffffe000093ce8>] tick_sched_timer+0x4e/0x92
	[  483.160992] [<ffffffe000089eb6>] __hrtimer_run_queues+0xae/0x108
	[  483.161881] [<ffffffe00008a5ac>] hrtimer_interrupt+0xca/0x1d4
	[  483.162778] [<ffffffe0004de564>] riscv_timer_interrupt+0x32/0x3a
	[  483.163542] [<ffffffe000612ad4>] do_IRQ+0xa4/0xb8
	[  483.164241] [<ffffffe000036814>] ret_from_exception+0x0/0xc
	[  483.165108] [<ffffffe000036814>] ret_from_exception+0x0/0xc
	[  515.044254] rcu: INFO: rcu_sched detected expedited stalls on CPUs/tasks: { 3-... } 116597 jiffies s: 53 root: 0x8/.
	[  515.046221] rcu: blocking rcu_node structures:
	[  515.046799] Task dump for CPU 3:
	[  515.047180] rdate           R  running task        0   254      1 0x00000008
	[  515.048476] Call Trace:
	[  515.048895] [<ffffffe00060ed48>] __schedule+0x158/0x36a

I will dig up some more into this tomorrow since it's way past by midnight here.

> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

> ---

>  include/linux/syscalls.h |  2 +-

>  kernel/time/time.c       | 20 +++++++++-----------

>  2 files changed, 10 insertions(+), 12 deletions(-)

> 

> diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h

> index e665920fa359..d0391cc2dae9 100644

> --- a/include/linux/syscalls.h

> +++ b/include/linux/syscalls.h

> @@ -734,7 +734,7 @@ asmlinkage long sys_getcpu(unsigned __user *cpu, unsigned __user *node, struct g

>  /* kernel/time.c */

>  asmlinkage long sys_gettimeofday(struct __kernel_old_timeval __user *tv,

>  				struct timezone __user *tz);

> -asmlinkage long sys_settimeofday(struct timeval __user *tv,

> +asmlinkage long sys_settimeofday(struct __kernel_old_timeval __user *tv,

>  				struct timezone __user *tz);

>  asmlinkage long sys_adjtimex(struct __kernel_timex __user *txc_p);

>  asmlinkage long sys_adjtimex_time32(struct old_timex32 __user *txc_p);

> diff --git a/kernel/time/time.c b/kernel/time/time.c

> index bc114f0be8f1..6bfbe640fd3b 100644

> --- a/kernel/time/time.c

> +++ b/kernel/time/time.c

> @@ -196,22 +196,21 @@ int do_sys_settimeofday64(const struct timespec64 *tv, const struct timezone *tz

>  	return 0;

>  }

>  

> -SYSCALL_DEFINE2(settimeofday, struct timeval __user *, tv,

> +SYSCALL_DEFINE2(settimeofday, struct __kernel_old_timeval __user *, tv,

>  		struct timezone __user *, tz)

>  {

>  	struct timespec64 new_ts;

> -	struct timeval user_tv;

>  	struct timezone new_tz;

>  

>  	if (tv) {

> -		if (copy_from_user(&user_tv, tv, sizeof(*tv)))

> +		if (get_user(new_ts.tv_sec, &tv->tv_sec) ||

> +		    get_user(new_ts.tv_nsec, &tv->tv_usec))

>  			return -EFAULT;

>  

> -		if (!timeval_valid(&user_tv))

> +		if (tv->tv_usec > USEC_PER_SEC)

>  			return -EINVAL;

>  

> -		new_ts.tv_sec = user_tv.tv_sec;

> -		new_ts.tv_nsec = user_tv.tv_usec * NSEC_PER_USEC;

> +		new_ts.tv_nsec *= NSEC_PER_USEC;

>  	}

>  	if (tz) {

>  		if (copy_from_user(&new_tz, tz, sizeof(*tz)))

> @@ -245,18 +244,17 @@ COMPAT_SYSCALL_DEFINE2(settimeofday, struct old_timeval32 __user *, tv,

>  		       struct timezone __user *, tz)

>  {

>  	struct timespec64 new_ts;

> -	struct timeval user_tv;

>  	struct timezone new_tz;

>  

>  	if (tv) {

> -		if (compat_get_timeval(&user_tv, tv))

> +		if (get_user(new_ts.tv_sec, &tv->tv_sec) ||

> +		    get_user(new_ts.tv_nsec, &tv->tv_usec))

>  			return -EFAULT;

>  

> -		if (!timeval_valid(&user_tv))

> +		if (new_ts.tv_nsec > USEC_PER_SEC)

>  			return -EINVAL;

>  

> -		new_ts.tv_sec = user_tv.tv_sec;

> -		new_ts.tv_nsec = user_tv.tv_usec * NSEC_PER_USEC;

> +		new_ts.tv_nsec *= NSEC_PER_USEC;

>  	}

>  	if (tz) {

>  		if (copy_from_user(&new_tz, tz, sizeof(*tz)))

> -- 

> 2.20.0

>
Arnd Bergmann Nov. 15, 2019, 7:58 a.m. UTC | #6 
On Fri, Nov 15, 2019 at 12:01 AM Abel Vesa <abelvesa@linux.com> wrote:
>

> On 19-11-08 22:12:16, Arnd Bergmann wrote:

> > The compat_get_timeval() and timeval_valid() interfaces

> > are deprecated and getting removed along with the definition

> > of struct timeval itself.

> >

> > Change the two implementations of the settimeofday()

> > system call to open-code these helpers and completely

> > avoid references to timeval.

> >


I'm not sure how we get to the RCU stall, but this is almost certainly another
symptom of a typo I had introduced in the patch, which others have also
reported. This is the the fix in today's linux-next:

--- a/kernel/time/time.c
+++ b/kernel/time/time.c
@@ -207,7 +207,7 @@ SYSCALL_DEFINE2(settimeofday, struct
__kernel_old_timeval __user *, tv,
                    get_user(new_ts.tv_nsec, &tv->tv_usec))
                        return -EFAULT;

-               if (tv->tv_usec > USEC_PER_SEC)
+               if (new_ts->tv_usec > USEC_PER_SEC)
                        return -EINVAL;

                new_ts.tv_nsec *= NSEC_PER_USEC;


    Arnd

> I get the following rcu stalls due to this patch on riscv64 (on qemu):

>

>         [root@riscv ~]# uname -a

>         Linux riscv 5.4.0-rc6-00018-gadde74306a4b #112 SMP Fri Nov 15 00:46:20 EET 2019 riscv64 riscv64 riscv64 GNU/Linux

>         [root@riscv ~]# [  420.135710] rcu: INFO: rcu_sched self-detected stall

>         on CPU

>         [  420.136839] rcu:     3-....: (99702 ticks this GP) idle=482/1/0x4000000000000002 softirq=3322/3322 fqs=48784

>         [  420.138917]  (t=99768 jiffies g=4985 q=8343)

>         [  420.139772] Task dump for CPU 3:

>         [  420.140236] rdate           R  running task        0   254      1 0x00000008

>         [  420.142226] Call Trace:

>         [  420.142791] [<ffffffe000037954>] walk_stackframe+0x0/0xa6

>         [  420.143911] [<ffffffe000037aba>] show_stack+0x2a/0x34

>         [  420.145010] [<ffffffe0000569c8>] sched_show_task+0xf0/0x116

>         [  420.145996] [<ffffffe00005b502>] dump_cpu_task+0x3e/0x48

>         [  420.147073] [<ffffffe000084e5e>] rcu_dump_cpu_stacks+0x7c/0xb4

>         [  420.148243] [<ffffffe0000842f6>] rcu_sched_clock_irq+0x3d6/0x582

>         [  420.149349] [<ffffffe0000897b4>] update_process_times+0x1e/0x42

>         [  420.150306] [<ffffffe000093a34>] tick_sched_handle.isra.0+0x2a/0x3a

>         [  420.150997] [<ffffffe000093ce8>] tick_sched_timer+0x4e/0x92

>         [  420.151603] [<ffffffe000089eb6>] __hrtimer_run_queues+0xae/0x108

>         [  420.152639] [<ffffffe00008a5ac>] hrtimer_interrupt+0xca/0x1d4

>         [  420.153629] [<ffffffe0004de564>] riscv_timer_interrupt+0x32/0x3a

>         [  420.154629] [<ffffffe000612ad4>] do_IRQ+0xa4/0xb8

>         [  420.155294] [<ffffffe000036814>] ret_from_exception+0x0/0xc

>         [  420.156073] [<ffffffe000036814>] ret_from_exception+0x0/0xc
Rasmus Villemoes Nov. 15, 2019, 10:27 a.m. UTC | #7 
On 15/11/2019 08.58, Arnd Bergmann wrote:
> On Fri, Nov 15, 2019 at 12:01 AM Abel Vesa <abelvesa@linux.com> wrote:

>>

>> On 19-11-08 22:12:16, Arnd Bergmann wrote:

>>> The compat_get_timeval() and timeval_valid() interfaces

>>> are deprecated and getting removed along with the definition

>>> of struct timeval itself.

>>>

>>> Change the two implementations of the settimeofday()

>>> system call to open-code these helpers and completely

>>> avoid references to timeval.

>>>

> 

> I'm not sure how we get to the RCU stall, but this is almost certainly another

> symptom of a typo I had introduced in the patch, which others have also

> reported. This is the the fix in today's linux-next:

> 

> --- a/kernel/time/time.c

> +++ b/kernel/time/time.c

> @@ -207,7 +207,7 @@ SYSCALL_DEFINE2(settimeofday, struct

> __kernel_old_timeval __user *, tv,

>                     get_user(new_ts.tv_nsec, &tv->tv_usec))

>                         return -EFAULT;

> 

> -               if (tv->tv_usec > USEC_PER_SEC)

> +               if (new_ts->tv_usec > USEC_PER_SEC)

>                         return -EINVAL;


Hopefully not :)

>                 new_ts.tv_nsec *= NSEC_PER_USEC;


So the actual patch in next-20191115 does

-               if (copy_from_user(&user_tv, tv, sizeof(*tv)))
+               if (get_user(new_ts.tv_sec, &tv->tv_sec) ||
+                   get_user(new_ts.tv_nsec, &tv->tv_usec))
                        return -EFAULT;

-               if (!timeval_valid(&user_tv))
+               if (new_ts.tv_nsec > USEC_PER_SEC)
                        return -EINVAL;

-               new_ts.tv_sec = user_tv.tv_sec;
-               new_ts.tv_nsec = user_tv.tv_usec * NSEC_PER_USEC;
+               new_ts.tv_nsec *= NSEC_PER_USEC;

But removing the "user value is < 0" check, relying on the timespec
value being rejected later, is wrong: 1000=8*125. Multiplying by 8
always gives a value with the low three bits clear, multiplying by 125
is reversible. So you can take any target value with the three low bits
clear, logic shift right by 3, multiply by 0x1cac083126e978d5 , and flip
the top three bits as you wish to generate 8 pre-images of that target
value. Four of those will be negative. A trivial example is 0x80..000
(aka LONG_MIN) and its cousins 0xa0..000, 0xc0..000, 0xe0..000 which all
become 0 and thus accepted after multiplying by NSEC_PER_USEC. But also
-858989233 (or -3689348814741906097 if long is 64 bit) become 4226200
which isn't even a multiple of 1000 - there's 500M examples to choose
from :)

I'm pretty sure it doesn't generate worse code, gcc is smart enough to
compile "foo > BAR || foo < 0" as if it was written "(unsigned version
of foo)foo > BAR". And while a value of USEC_PER_SEC itself will not
overflow and then be rejeted because the real comparison done later is
">= NSEC_PER_SEC", I think it's clearer to say "foo >= USEC_PER_SEC ||
foo < 0) just so the same pattern is used.

Rasmus
Arnd Bergmann Nov. 15, 2019, 1:50 p.m. UTC | #8 
On Fri, Nov 15, 2019 at 11:27 AM Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
> On 15/11/2019 08.58, Arnd Bergmann wrote:

> > On Fri, Nov 15, 2019 at 12:01 AM Abel Vesa <abelvesa@linux.com> wrote:

> >>

> > --- a/kernel/time/time.c

> > +++ b/kernel/time/time.c

> > @@ -207,7 +207,7 @@ SYSCALL_DEFINE2(settimeofday, struct

> > __kernel_old_timeval __user *, tv,

> >                     get_user(new_ts.tv_nsec, &tv->tv_usec))

> >                         return -EFAULT;

> >

> > -               if (tv->tv_usec > USEC_PER_SEC)

> > +               if (new_ts->tv_usec > USEC_PER_SEC)

> >                         return -EINVAL;

>

> Hopefully not :)


No, I misquoted from a fix that I had temporarily applied, not the
version in linux-next.

>

> >                 new_ts.tv_nsec *= NSEC_PER_USEC;

>

> So the actual patch in next-20191115 does

>

> -               if (copy_from_user(&user_tv, tv, sizeof(*tv)))

> +               if (get_user(new_ts.tv_sec, &tv->tv_sec) ||

> +                   get_user(new_ts.tv_nsec, &tv->tv_usec))

>                         return -EFAULT;

>

> -               if (!timeval_valid(&user_tv))

> +               if (new_ts.tv_nsec > USEC_PER_SEC)

>                         return -EINVAL;

>

> -               new_ts.tv_sec = user_tv.tv_sec;

> -               new_ts.tv_nsec = user_tv.tv_usec * NSEC_PER_USEC;

> +               new_ts.tv_nsec *= NSEC_PER_USEC;

>

> But removing the "user value is < 0" check, relying on the timespec

> value being rejected later, is wrong


You are right of course, so many ways to get this one line wrong...
Pushed more more update to the branch now.

Thanks for the careful review!

        Arnd
diff mbox series

Patch

diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index e665920fa359..d0391cc2dae9 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -734,7 +734,7 @@  asmlinkage long sys_getcpu(unsigned __user *cpu, unsigned __user *node, struct g
 /* kernel/time.c */
 asmlinkage long sys_gettimeofday(struct __kernel_old_timeval __user *tv,
 				struct timezone __user *tz);
-asmlinkage long sys_settimeofday(struct timeval __user *tv,
+asmlinkage long sys_settimeofday(struct __kernel_old_timeval __user *tv,
 				struct timezone __user *tz);
 asmlinkage long sys_adjtimex(struct __kernel_timex __user *txc_p);
 asmlinkage long sys_adjtimex_time32(struct old_timex32 __user *txc_p);
diff --git a/kernel/time/time.c b/kernel/time/time.c
index bc114f0be8f1..6bfbe640fd3b 100644
--- a/kernel/time/time.c
+++ b/kernel/time/time.c
@@ -196,22 +196,21 @@  int do_sys_settimeofday64(const struct timespec64 *tv, const struct timezone *tz
 	return 0;
 }
 
-SYSCALL_DEFINE2(settimeofday, struct timeval __user *, tv,
+SYSCALL_DEFINE2(settimeofday, struct __kernel_old_timeval __user *, tv,
 		struct timezone __user *, tz)
 {
 	struct timespec64 new_ts;
-	struct timeval user_tv;
 	struct timezone new_tz;
 
 	if (tv) {
-		if (copy_from_user(&user_tv, tv, sizeof(*tv)))
+		if (get_user(new_ts.tv_sec, &tv->tv_sec) ||
+		    get_user(new_ts.tv_nsec, &tv->tv_usec))
 			return -EFAULT;
 
-		if (!timeval_valid(&user_tv))
+		if (tv->tv_usec > USEC_PER_SEC)
 			return -EINVAL;
 
-		new_ts.tv_sec = user_tv.tv_sec;
-		new_ts.tv_nsec = user_tv.tv_usec * NSEC_PER_USEC;
+		new_ts.tv_nsec *= NSEC_PER_USEC;
 	}
 	if (tz) {
 		if (copy_from_user(&new_tz, tz, sizeof(*tz)))
@@ -245,18 +244,17 @@  COMPAT_SYSCALL_DEFINE2(settimeofday, struct old_timeval32 __user *, tv,
 		       struct timezone __user *, tz)
 {
 	struct timespec64 new_ts;
-	struct timeval user_tv;
 	struct timezone new_tz;
 
 	if (tv) {
-		if (compat_get_timeval(&user_tv, tv))
+		if (get_user(new_ts.tv_sec, &tv->tv_sec) ||
+		    get_user(new_ts.tv_nsec, &tv->tv_usec))
 			return -EFAULT;
 
-		if (!timeval_valid(&user_tv))
+		if (new_ts.tv_nsec > USEC_PER_SEC)
 			return -EINVAL;
 
-		new_ts.tv_sec = user_tv.tv_sec;
-		new_ts.tv_nsec = user_tv.tv_usec * NSEC_PER_USEC;
+		new_ts.tv_nsec *= NSEC_PER_USEC;
 	}
 	if (tz) {
 		if (copy_from_user(&new_tz, tz, sizeof(*tz)))