linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* x86_64 system lockup from userspace using setitimer()
@ 2007-03-13 18:55 Johannes Bauer
  2007-03-13 19:19 ` Andreas Schwab
  2007-03-13 20:02 ` Chuck Ebbert
  0 siblings, 2 replies; 16+ messages in thread
From: Johannes Bauer @ 2007-03-13 18:55 UTC (permalink / raw)
  To: linux-kernel

Dear Community,

I think I've encountered a bug with the Linux kernel which results in a 
complete system lockup and which can be started without root priviliges. 
It's reproducible with 2.6.20.1 and 2.6.20.2 and only x64_64 seems affected.

Here's the code which triggers the bug (originally found by me using an 
only partly initialized "struct itimerval" structure - hence the strange 
values in it_interval):

-----8<-----8<-----8<-----8<-----8<-----8<-----8<-----8<-----8<-----
#include <stdio.h>
#include <sys/time.h>
#include <unistd.h>

int main(int argc, char **argv) {
     struct itimerval tim = {
         .it_interval = {
             .tv_sec = 140735669863712,
             .tv_usec = 4199521
         },
         .it_value = {
             .tv_sec = 0,
             .tv_usec =  100000
         }
     };
     setitimer(ITIMER_REAL, &tim, NULL);
     while (1) sleep(1);
     return 0;
}
-----8<-----8<-----8<-----8<-----8<-----8<-----8<-----8<-----8<-----

Compiled with gcc 4.1.1 with "gcc -O2 -Wall -o crash crash.c".

The sourcecode can be found at 
http://www.johannes-bauer.com/crash/crash.c and my kernel configuration 
is at http://www.johannes-bauer.com/crash/config

Any further questions: feel free to ask. Please CC me for any posts in 
this thread.

Greetings,
Johannes

-- 
"A PC without Windows is like a chocolate cake without mustard."

Johannes Bauer
91054 Erlangen

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

* Re: x86_64 system lockup from userspace using setitimer()
  2007-03-13 18:55 x86_64 system lockup from userspace using setitimer() Johannes Bauer
@ 2007-03-13 19:19 ` Andreas Schwab
  2007-03-13 20:02 ` Chuck Ebbert
  1 sibling, 0 replies; 16+ messages in thread
From: Andreas Schwab @ 2007-03-13 19:19 UTC (permalink / raw)
  To: Johannes Bauer; +Cc: linux-kernel

Johannes Bauer <JohannesBauer@gmx.de> writes:

> Dear Community,
>
> I think I've encountered a bug with the Linux kernel which results in a
> complete system lockup and which can be started without root
> priviliges. It's reproducible with 2.6.20.1 and 2.6.20.2 and only x64_64
> seems affected.

I can also reproduce it on ia64 with 2.6.20.  2.6.16.42 is ok.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

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

* Re: x86_64 system lockup from userspace using setitimer()
  2007-03-13 18:55 x86_64 system lockup from userspace using setitimer() Johannes Bauer
  2007-03-13 19:19 ` Andreas Schwab
@ 2007-03-13 20:02 ` Chuck Ebbert
  2007-03-13 20:33   ` Thomas Gleixner
  1 sibling, 1 reply; 16+ messages in thread
From: Chuck Ebbert @ 2007-03-13 20:02 UTC (permalink / raw)
  To: Johannes Bauer; +Cc: linux-kernel, Thomas Gleixner, schwab

Johannes Bauer wrote:
> Dear Community,
> 
> I think I've encountered a bug with the Linux kernel which results in a
> complete system lockup and which can be started without root priviliges.
> It's reproducible with 2.6.20.1 and 2.6.20.2 and only x64_64 seems
> affected.
> 
> Here's the code which triggers the bug (originally found by me using an
> only partly initialized "struct itimerval" structure - hence the strange
> values in it_interval):
> 
> -----8<-----8<-----8<-----8<-----8<-----8<-----8<-----8<-----8<-----
> #include <stdio.h>
> #include <sys/time.h>
> #include <unistd.h>
> 
> int main(int argc, char **argv) {
>     struct itimerval tim = {
>         .it_interval = {
>             .tv_sec = 140735669863712,
>             .tv_usec = 4199521
>         },
>         .it_value = {
>             .tv_sec = 0,
>             .tv_usec =  100000
>         }
>     };
>     setitimer(ITIMER_REAL, &tim, NULL);
>     while (1) sleep(1);
>     return 0;
> }
> -----8<-----8<-----8<-----8<-----8<-----8<-----8<-----8<-----8<-----
> 
> Compiled with gcc 4.1.1 with "gcc -O2 -Wall -o crash crash.c".
> 
> The sourcecode can be found at
> http://www.johannes-bauer.com/crash/crash.c and my kernel configuration
> is at http://www.johannes-bauer.com/crash/config
> 
> Any further questions: feel free to ask. Please CC me for any posts in
> this thread.

Could this be fixed by:

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=8bfd9a7a229b5f3d3eda5d7d45c2eebec5b4ba16

[PATCH] hrtimers: prevent possible itimer DoS

?


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

* Re: x86_64 system lockup from userspace using setitimer()
  2007-03-13 20:02 ` Chuck Ebbert
@ 2007-03-13 20:33   ` Thomas Gleixner
  2007-03-14 10:00     ` [PATCH] hrtimer: prevent overrun DoS in hrtimer_forward() Thomas Gleixner
  0 siblings, 1 reply; 16+ messages in thread
From: Thomas Gleixner @ 2007-03-13 20:33 UTC (permalink / raw)
  To: Chuck Ebbert; +Cc: Johannes Bauer, linux-kernel, schwab

On Tue, 2007-03-13 at 16:02 -0400, Chuck Ebbert wrote:
> >     struct itimerval tim = {
> >         .it_interval = {
> >             .tv_sec = 140735669863712,
> >             .tv_usec = 4199521
> >         },
> Could this be fixed by:
> 
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=8bfd9a7a229b5f3d3eda5d7d45c2eebec5b4ba16
> 
> [PATCH] hrtimers: prevent possible itimer DoS

No. The possible DoS is only when high res timers are enabled, which is
not the case in 2.6.20.

Looking at the values 

140735669863712 = 0x7FFF 939C 0520

We convert second to nanoseconds:

140735669863712 * 1e9 =  0x1DCD 4BC3 6B82 914B 4000

The seconds value is limited to LONG_MAX, but on a 64 bit machine, the
140735669863712 is inside LONG_MAX and we have an multiplication
overflow.

I'm not sure, how this results in a DoS, but I will look into this
tomorrow morning, when I'm more awake.

	tglx



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

* [PATCH] hrtimer: prevent overrun DoS in hrtimer_forward()
  2007-03-13 20:33   ` Thomas Gleixner
@ 2007-03-14 10:00     ` Thomas Gleixner
  2007-03-14 10:08       ` Ingo Molnar
                         ` (2 more replies)
  0 siblings, 3 replies; 16+ messages in thread
From: Thomas Gleixner @ 2007-03-14 10:00 UTC (permalink / raw)
  To: Chuck Ebbert
  Cc: Johannes Bauer, linux-kernel, schwab, Stable Kernel Team, Greg KH,
	Adrian Bunk, Andrew Morton, Ingo Molnar

hrtimer_forward() does not check for the possible overflow of
timer->expires. This can happen on 64 bit machines with large interval
values and results currently in an endless loop in the softirq because
the expiry value becomes negative and therefor the timer is expired all
the time.

Check for this condition and set the expiry value to the max. expiry
time in the future.

The fix should be applied to stable kernel series as well.

Signed-off-by: Thomas Gleixner <tglx@linutronix,de>

diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c
index ec4cb9f..5e7122d 100644
--- a/kernel/hrtimer.c
+++ b/kernel/hrtimer.c
@@ -644,6 +644,12 @@ hrtimer_forward(struct hrtimer *timer, k
 		orun++;
 	}
 	timer->expires = ktime_add(timer->expires, interval);
+	/*
+	 * Make sure, that the result did not wrap with a very large
+	 * interval.
+	 */
+	if (timer->expires.tv64 < 0)
+		timer->expires = ktime_set(KTIME_SEC_MAX, 0);
 
 	return orun;
 }



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

* Re: [PATCH] hrtimer: prevent overrun DoS in hrtimer_forward()
  2007-03-14 10:00     ` [PATCH] hrtimer: prevent overrun DoS in hrtimer_forward() Thomas Gleixner
@ 2007-03-14 10:08       ` Ingo Molnar
  2007-03-16 20:43       ` Andrew Morton
  2007-04-04 21:11       ` Adrian Bunk
  2 siblings, 0 replies; 16+ messages in thread
From: Ingo Molnar @ 2007-03-14 10:08 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Chuck Ebbert, Johannes Bauer, linux-kernel, schwab,
	Stable Kernel Team, Greg KH, Adrian Bunk, Andrew Morton


* Thomas Gleixner <tglx@linutronix.de> wrote:

> hrtimer_forward() does not check for the possible overflow of 
> timer->expires. This can happen on 64 bit machines with large interval 
> values and results currently in an endless loop in the softirq because 
> the expiry value becomes negative and therefor the timer is expired 
> all the time.
> 
> Check for this condition and set the expiry value to the max. expiry 
> time in the future.
> 
> The fix should be applied to stable kernel series as well.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix,de>

ouch ... nice one.

Acked-by: Ingo Molnar <mingo@elte.hu>

	Ingo

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

* Re: [PATCH] hrtimer: prevent overrun DoS in hrtimer_forward()
  2007-03-14 10:00     ` [PATCH] hrtimer: prevent overrun DoS in hrtimer_forward() Thomas Gleixner
  2007-03-14 10:08       ` Ingo Molnar
@ 2007-03-16 20:43       ` Andrew Morton
  2007-03-16 21:05         ` Thomas Gleixner
  2007-04-04 21:11       ` Adrian Bunk
  2 siblings, 1 reply; 16+ messages in thread
From: Andrew Morton @ 2007-03-16 20:43 UTC (permalink / raw)
  To: tglx
  Cc: Chuck Ebbert, Johannes Bauer, linux-kernel, schwab,
	Stable Kernel Team, Greg KH, Adrian Bunk, Ingo Molnar

On Wed, 14 Mar 2007 11:00:12 +0100 Thomas Gleixner <tglx@linutronix.de> wrote:

> rtimer_forward() does not check for the possible overflow of
> timer->expires. This can happen on 64 bit machines with large interval
> values and results currently in an endless loop in the softirq because
> the expiry value becomes negative and therefor the timer is expired all
> the time.
> 
> Check for this condition and set the expiry value to the max. expiry
> time in the future.
> 
> The fix should be applied to stable kernel series as well.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix,de>
> 
> diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c
> index ec4cb9f..5e7122d 100644
> --- a/kernel/hrtimer.c
> +++ b/kernel/hrtimer.c
> @@ -644,6 +644,12 @@ hrtimer_forward(struct hrtimer *timer, k
>  		orun++;
>  	}
>  	timer->expires = ktime_add(timer->expires, interval);
> +	/*
> +	 * Make sure, that the result did not wrap with a very large
> +	 * interval.
> +	 */
> +	if (timer->expires.tv64 < 0)
> +		timer->expires = ktime_set(KTIME_SEC_MAX, 0);
>  
>  	return orun;
>  }

kernel/hrtimer.c: In function 'hrtimer_forward':
kernel/hrtimer.c:652: warning: overflow in implicit constant conversion

problem is, KTIME_SEC_MAX is 9,223,372,036 and ktime_set() takes a `long'.


This?

--- a/include/linux/ktime.h~ktime_set-fix-arg-type
+++ a/include/linux/ktime.h
@@ -72,13 +72,13 @@ typedef union {
  *
  * Return the ktime_t representation of the value
  */
-static inline ktime_t ktime_set(const long secs, const unsigned long nsecs)
+static inline ktime_t ktime_set(const s64 secs, const unsigned long nsecs)
 {
 #if (BITS_PER_LONG == 64)
 	if (unlikely(secs >= KTIME_SEC_MAX))
 		return (ktime_t){ .tv64 = KTIME_MAX };
 #endif
-	return (ktime_t) { .tv64 = (s64)secs * NSEC_PER_SEC + (s64)nsecs };
+	return (ktime_t) { .tv64 = secs * NSEC_PER_SEC + (s64)nsecs };
 }
 
 /* Subtract two ktime_t variables. rem = lhs -rhs: */
_

I worry about that `secs >= KTIME_SEC_MAX' comparison in there, too.  Both
operands are signed.



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

* Re: [PATCH] hrtimer: prevent overrun DoS in hrtimer_forward()
  2007-03-16 20:43       ` Andrew Morton
@ 2007-03-16 21:05         ` Thomas Gleixner
  2007-03-18 21:16           ` Chuck Ebbert
  0 siblings, 1 reply; 16+ messages in thread
From: Thomas Gleixner @ 2007-03-16 21:05 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Chuck Ebbert, Johannes Bauer, linux-kernel, schwab,
	Stable Kernel Team, Greg KH, Adrian Bunk, Ingo Molnar

On Fri, 2007-03-16 at 12:43 -0800, Andrew Morton wrote:
> On Wed, 14 Mar 2007 11:00:12 +0100 Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> > rtimer_forward() does not check for the possible overflow of
> > timer->expires. This can happen on 64 bit machines with large interval
> > values and results currently in an endless loop in the softirq because
> > the expiry value becomes negative and therefor the timer is expired all
> > the time.
> > 
> > Check for this condition and set the expiry value to the max. expiry
> > time in the future.
> > 
> > The fix should be applied to stable kernel series as well.
> > 
> > Signed-off-by: Thomas Gleixner <tglx@linutronix,de>
> > 
> > diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c
> > index ec4cb9f..5e7122d 100644
> > --- a/kernel/hrtimer.c
> > +++ b/kernel/hrtimer.c
> > @@ -644,6 +644,12 @@ hrtimer_forward(struct hrtimer *timer, k
> >  		orun++;
> >  	}
> >  	timer->expires = ktime_add(timer->expires, interval);
> > +	/*
> > +	 * Make sure, that the result did not wrap with a very large
> > +	 * interval.
> > +	 */
> > +	if (timer->expires.tv64 < 0)
> > +		timer->expires = ktime_set(KTIME_SEC_MAX, 0);
> >  
> >  	return orun;
> >  }
> 
> kernel/hrtimer.c: In function 'hrtimer_forward':
> kernel/hrtimer.c:652: warning: overflow in implicit constant conversion
> 
> problem is, KTIME_SEC_MAX is 9,223,372,036 and ktime_set() takes a `long'.

Stupid me :(

> This?
> 
> --- a/include/linux/ktime.h~ktime_set-fix-arg-type
> +++ a/include/linux/ktime.h
> @@ -72,13 +72,13 @@ typedef union {
>   *
>   * Return the ktime_t representation of the value
>   */
> -static inline ktime_t ktime_set(const long secs, const unsigned long nsecs)
> +static inline ktime_t ktime_set(const s64 secs, const unsigned long nsecs)
>  {
>  #if (BITS_PER_LONG == 64)
>  	if (unlikely(secs >= KTIME_SEC_MAX))
>  		return (ktime_t){ .tv64 = KTIME_MAX };
>  #endif
> -	return (ktime_t) { .tv64 = (s64)secs * NSEC_PER_SEC + (s64)nsecs };
> +	return (ktime_t) { .tv64 = secs * NSEC_PER_SEC + (s64)nsecs };
>  }
>  
>  /* Subtract two ktime_t variables. rem = lhs -rhs: */
> _
> 
> I worry about that `secs >= KTIME_SEC_MAX' comparison in there, too.  Both
> operands are signed.

I'd prefer this one: The maximum seconds value we can handle on 32bit is
LONG_MAX.

diff --git a/include/linux/ktime.h b/include/linux/ktime.h
index c68c7ac..248305b 100644
--- a/include/linux/ktime.h
+++ b/include/linux/ktime.h
@@ -57,7 +57,11 @@ typedef union {
 } ktime_t;
 
 #define KTIME_MAX			((s64)~((u64)1 << 63))
-#define KTIME_SEC_MAX			(KTIME_MAX / NSEC_PER_SEC)
+#if (BITS_PER_LONG == 64)
+# define KTIME_SEC_MAX			(KTIME_MAX / NSEC_PER_SEC)
+#else
+# define KTIME_SEC_MAX			LONG_MAX
+#endif
 
 /*
  * ktime_t definitions when using the 64-bit scalar representation:



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

* Re: [PATCH] hrtimer: prevent overrun DoS in hrtimer_forward()
  2007-03-16 21:05         ` Thomas Gleixner
@ 2007-03-18 21:16           ` Chuck Ebbert
  2007-03-18 21:32             ` Thomas Gleixner
  0 siblings, 1 reply; 16+ messages in thread
From: Chuck Ebbert @ 2007-03-18 21:16 UTC (permalink / raw)
  To: tglx
  Cc: Andrew Morton, Johannes Bauer, linux-kernel, schwab,
	Stable Kernel Team, Greg KH, Adrian Bunk, Ingo Molnar

Thomas Gleixner wrote:
> 
> I'd prefer this one: The maximum seconds value we can handle on 32bit is
> LONG_MAX.
> 
> diff --git a/include/linux/ktime.h b/include/linux/ktime.h
> index c68c7ac..248305b 100644
> --- a/include/linux/ktime.h
> +++ b/include/linux/ktime.h
> @@ -57,7 +57,11 @@ typedef union {
>  } ktime_t;
>  
>  #define KTIME_MAX			((s64)~((u64)1 << 63))
> -#define KTIME_SEC_MAX			(KTIME_MAX / NSEC_PER_SEC)
> +#if (BITS_PER_LONG == 64)
> +# define KTIME_SEC_MAX			(KTIME_MAX / NSEC_PER_SEC)
> +#else
> +# define KTIME_SEC_MAX			LONG_MAX
> +#endif
>  
>  /*
>   * ktime_t definitions when using the 64-bit scalar representation:
> 

Just to be clear: this replaces the earlier patch, right?





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

* Re: [PATCH] hrtimer: prevent overrun DoS in hrtimer_forward()
  2007-03-18 21:16           ` Chuck Ebbert
@ 2007-03-18 21:32             ` Thomas Gleixner
  2007-03-18 21:53               ` Chuck Ebbert
  0 siblings, 1 reply; 16+ messages in thread
From: Thomas Gleixner @ 2007-03-18 21:32 UTC (permalink / raw)
  To: Chuck Ebbert
  Cc: Andrew Morton, Johannes Bauer, linux-kernel, schwab,
	Stable Kernel Team, Greg KH, Adrian Bunk, Ingo Molnar

On Sun, 2007-03-18 at 17:16 -0400, Chuck Ebbert wrote:
> Thomas Gleixner wrote:
> > 
> > I'd prefer this one: The maximum seconds value we can handle on 32bit is
> > LONG_MAX.
> > 
> > diff --git a/include/linux/ktime.h b/include/linux/ktime.h
> > index c68c7ac..248305b 100644
> > --- a/include/linux/ktime.h
> > +++ b/include/linux/ktime.h
> > @@ -57,7 +57,11 @@ typedef union {
> >  } ktime_t;
> >  
> >  #define KTIME_MAX			((s64)~((u64)1 << 63))
> > -#define KTIME_SEC_MAX			(KTIME_MAX / NSEC_PER_SEC)
> > +#if (BITS_PER_LONG == 64)
> > +# define KTIME_SEC_MAX			(KTIME_MAX / NSEC_PER_SEC)
> > +#else
> > +# define KTIME_SEC_MAX			LONG_MAX
> > +#endif
> >  
> >  /*
> >   * ktime_t definitions when using the 64-bit scalar representation:
> > 
> 
> Just to be clear: this replaces the earlier patch, right?

This replaces the fix Andrew did.

http://marc.info/?l=linux-kernel&m=117407812411997&w=2

	tglx



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

* Re: [PATCH] hrtimer: prevent overrun DoS in hrtimer_forward()
  2007-03-18 21:32             ` Thomas Gleixner
@ 2007-03-18 21:53               ` Chuck Ebbert
  2007-03-18 22:04                 ` Thomas Gleixner
  0 siblings, 1 reply; 16+ messages in thread
From: Chuck Ebbert @ 2007-03-18 21:53 UTC (permalink / raw)
  To: tglx
  Cc: Andrew Morton, Johannes Bauer, linux-kernel, schwab,
	Stable Kernel Team, Greg KH, Adrian Bunk, Ingo Molnar

Thomas Gleixner wrote:
> On Sun, 2007-03-18 at 17:16 -0400, Chuck Ebbert wrote:
>> Thomas Gleixner wrote:
>>> I'd prefer this one: The maximum seconds value we can handle on 32bit is
>>> LONG_MAX.
>>>
>>> diff --git a/include/linux/ktime.h b/include/linux/ktime.h
>>> index c68c7ac..248305b 100644
>>> --- a/include/linux/ktime.h
>>> +++ b/include/linux/ktime.h
>>> @@ -57,7 +57,11 @@ typedef union {
>>>  } ktime_t;
>>>  
>>>  #define KTIME_MAX			((s64)~((u64)1 << 63))
>>> -#define KTIME_SEC_MAX			(KTIME_MAX / NSEC_PER_SEC)
>>> +#if (BITS_PER_LONG == 64)
>>> +# define KTIME_SEC_MAX			(KTIME_MAX / NSEC_PER_SEC)
>>> +#else
>>> +# define KTIME_SEC_MAX			LONG_MAX
>>> +#endif
>>>  
>>>  /*
>>>   * ktime_t definitions when using the 64-bit scalar representation:
>>>
>> Just to be clear: this replaces the earlier patch, right?
> 
> This replaces the fix Andrew did.
> 
> http://marc.info/?l=linux-kernel&m=117407812411997&w=2
> 

Right, but is the original "Prevent DOS" patch from you still needed?
Or did Andrew's patch replace that one, and now this replaces his?


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

* Re: [PATCH] hrtimer: prevent overrun DoS in hrtimer_forward()
  2007-03-18 22:04                 ` Thomas Gleixner
@ 2007-03-18 22:02                   ` Chuck Ebbert
  0 siblings, 0 replies; 16+ messages in thread
From: Chuck Ebbert @ 2007-03-18 22:02 UTC (permalink / raw)
  To: tglx
  Cc: Andrew Morton, Johannes Bauer, linux-kernel, schwab,
	Stable Kernel Team, Greg KH, Adrian Bunk, Ingo Molnar

Thomas Gleixner wrote:
> On Sun, 2007-03-18 at 17:53 -0400, Chuck Ebbert wrote:
>>>> Just to be clear: this replaces the earlier patch, right?
>>> This replaces the fix Andrew did.
>>>
>>> http://marc.info/?l=linux-kernel&m=117407812411997&w=2
>>>
>> Right, but is the original "Prevent DOS" patch from you still needed?
>> Or did Andrew's patch replace that one, and now this replaces his?
> 
> The original patch is still needed - it handles the problem in the first
> place.
> 
> I missed to compile it for 32bit and Andrew did a fix, which I replaced.

Ah, OK, and both of those are now in the queue for 2.6.20-stable.



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

* Re: [PATCH] hrtimer: prevent overrun DoS in hrtimer_forward()
  2007-03-18 21:53               ` Chuck Ebbert
@ 2007-03-18 22:04                 ` Thomas Gleixner
  2007-03-18 22:02                   ` Chuck Ebbert
  0 siblings, 1 reply; 16+ messages in thread
From: Thomas Gleixner @ 2007-03-18 22:04 UTC (permalink / raw)
  To: Chuck Ebbert
  Cc: Andrew Morton, Johannes Bauer, linux-kernel, schwab,
	Stable Kernel Team, Greg KH, Adrian Bunk, Ingo Molnar

On Sun, 2007-03-18 at 17:53 -0400, Chuck Ebbert wrote:
> >> Just to be clear: this replaces the earlier patch, right?
> > 
> > This replaces the fix Andrew did.
> > 
> > http://marc.info/?l=linux-kernel&m=117407812411997&w=2
> > 
> 
> Right, but is the original "Prevent DOS" patch from you still needed?
> Or did Andrew's patch replace that one, and now this replaces his?

The original patch is still needed - it handles the problem in the first
place.

I missed to compile it for 32bit and Andrew did a fix, which I replaced.

	tglx



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

* Re: [PATCH] hrtimer: prevent overrun DoS in hrtimer_forward()
  2007-03-14 10:00     ` [PATCH] hrtimer: prevent overrun DoS in hrtimer_forward() Thomas Gleixner
  2007-03-14 10:08       ` Ingo Molnar
  2007-03-16 20:43       ` Andrew Morton
@ 2007-04-04 21:11       ` Adrian Bunk
  2007-04-04 21:30         ` Thomas Gleixner
  2 siblings, 1 reply; 16+ messages in thread
From: Adrian Bunk @ 2007-04-04 21:11 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Chuck Ebbert, Johannes Bauer, linux-kernel, schwab,
	Stable Kernel Team, Greg KH, Andrew Morton, Ingo Molnar

On Wed, Mar 14, 2007 at 11:00:12AM +0100, Thomas Gleixner wrote:
> hrtimer_forward() does not check for the possible overflow of
> timer->expires. This can happen on 64 bit machines with large interval
> values and results currently in an endless loop in the softirq because
> the expiry value becomes negative and therefor the timer is expired all
> the time.
> 
> Check for this condition and set the expiry value to the max. expiry
> time in the future.
> 
> The fix should be applied to stable kernel series as well.


Is this relevant for 2.6.16?

I'm asking since KTIME_SEC_MAX is not used in 2.6.16, and therefore the 
check in ktime_set() is also missing.


> Signed-off-by: Thomas Gleixner <tglx@linutronix,de>
> 
> diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c
> index ec4cb9f..5e7122d 100644
> --- a/kernel/hrtimer.c
> +++ b/kernel/hrtimer.c
> @@ -644,6 +644,12 @@ hrtimer_forward(struct hrtimer *timer, k
>  		orun++;
>  	}
>  	timer->expires = ktime_add(timer->expires, interval);
> +	/*
> +	 * Make sure, that the result did not wrap with a very large
> +	 * interval.
> +	 */
> +	if (timer->expires.tv64 < 0)
> +		timer->expires = ktime_set(KTIME_SEC_MAX, 0);
>  
>  	return orun;
>  }
> 

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed


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

* Re: [PATCH] hrtimer: prevent overrun DoS in hrtimer_forward()
  2007-04-04 21:11       ` Adrian Bunk
@ 2007-04-04 21:30         ` Thomas Gleixner
  2007-04-09 13:01           ` Adrian Bunk
  0 siblings, 1 reply; 16+ messages in thread
From: Thomas Gleixner @ 2007-04-04 21:30 UTC (permalink / raw)
  To: Adrian Bunk
  Cc: Chuck Ebbert, Johannes Bauer, linux-kernel, schwab,
	Stable Kernel Team, Greg KH, Andrew Morton, Ingo Molnar

On Wed, 2007-04-04 at 23:11 +0200, Adrian Bunk wrote:
> On Wed, Mar 14, 2007 at 11:00:12AM +0100, Thomas Gleixner wrote:
> > hrtimer_forward() does not check for the possible overflow of
> > timer->expires. This can happen on 64 bit machines with large interval
> > values and results currently in an endless loop in the softirq because
> > the expiry value becomes negative and therefor the timer is expired all
> > the time.
> > 
> > Check for this condition and set the expiry value to the max. expiry
> > time in the future.
> > 
> > The fix should be applied to stable kernel series as well.
> 
> 
> Is this relevant for 2.6.16?
> 
> I'm asking since KTIME_SEC_MAX is not used in 2.6.16, and therefore the 
> check in ktime_set() is also missing.

KTIME_SEC_MAX was introduced with commit
96dd7421a06a5bc6eb731323b95efcb2fd864854

to fix a conversion problem on 64 bit machines, which is also present in
2.6.16 AFAICT.

The patch just makes use of this constant. So you need to pull it as
well.

	tglx



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

* Re: [PATCH] hrtimer: prevent overrun DoS in hrtimer_forward()
  2007-04-04 21:30         ` Thomas Gleixner
@ 2007-04-09 13:01           ` Adrian Bunk
  0 siblings, 0 replies; 16+ messages in thread
From: Adrian Bunk @ 2007-04-09 13:01 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Chuck Ebbert, Johannes Bauer, linux-kernel, schwab,
	Stable Kernel Team, Greg KH, Andrew Morton, Ingo Molnar

On Wed, Apr 04, 2007 at 11:30:48PM +0200, Thomas Gleixner wrote:
> On Wed, 2007-04-04 at 23:11 +0200, Adrian Bunk wrote:
> > On Wed, Mar 14, 2007 at 11:00:12AM +0100, Thomas Gleixner wrote:
> > > hrtimer_forward() does not check for the possible overflow of
> > > timer->expires. This can happen on 64 bit machines with large interval
> > > values and results currently in an endless loop in the softirq because
> > > the expiry value becomes negative and therefor the timer is expired all
> > > the time.
> > > 
> > > Check for this condition and set the expiry value to the max. expiry
> > > time in the future.
> > > 
> > > The fix should be applied to stable kernel series as well.
> > 
> > 
> > Is this relevant for 2.6.16?
> > 
> > I'm asking since KTIME_SEC_MAX is not used in 2.6.16, and therefore the 
> > check in ktime_set() is also missing.
> 
> KTIME_SEC_MAX was introduced with commit
> 96dd7421a06a5bc6eb731323b95efcb2fd864854
> 
> to fix a conversion problem on 64 bit machines, which is also present in
> 2.6.16 AFAICT.
> 
> The patch just makes use of this constant. So you need to pull it as
> well.

Thanks, below is what I applied.

> 	tglx

cu
Adrian


Thomas Gleixner (3):
      prevent timespec/timeval to ktime_t overflow
      fix MTIME_SEC_MAX on 32-bit
      hrtimer: prevent overrun DoS in hrtimer_forward()


diff --git a/include/linux/ktime.h b/include/linux/ktime.h
index f3dec45..4548ddb 100644
--- a/include/linux/ktime.h
+++ b/include/linux/ktime.h
@@ -56,7 +56,12 @@ typedef union {
 #endif
 } ktime_t;
 
-#define KTIME_MAX			(~((u64)1 << 63))
+#define KTIME_MAX			((s64)~((u64)1 << 63))
+#if (BITS_PER_LONG == 64)
+# define KTIME_SEC_MAX			(KTIME_MAX / NSEC_PER_SEC)
+#else
+# define KTIME_SEC_MAX			LONG_MAX
+#endif
 
 /*
  * ktime_t definitions when using the 64-bit scalar representation:
@@ -77,6 +82,10 @@ typedef union {
  */
 static inline ktime_t ktime_set(const long secs, const unsigned long nsecs)
 {
+#if (BITS_PER_LONG == 64)
+	if (unlikely(secs >= KTIME_SEC_MAX))
+		return (ktime_t){ .tv64 = KTIME_MAX };
+#endif
 	return (ktime_t) { .tv64 = (s64)secs * NSEC_PER_SEC + (s64)nsecs };
 }
 
diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c
index 14bc9cf..a29ceb0 100644
--- a/kernel/hrtimer.c
+++ b/kernel/hrtimer.c
@@ -316,6 +316,12 @@ hrtimer_forward(struct hrtimer *timer, ktime_t interval)
 		orun++;
 	}
 	timer->expires = ktime_add(timer->expires, interval);
+	/*
+	 * Make sure, that the result did not wrap with a very large
+	 * interval.
+	 */
+	if (timer->expires.tv64 < 0)
+		timer->expires = ktime_set(KTIME_SEC_MAX, 0);
 
 	return orun;
 }


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

end of thread, other threads:[~2007-04-09 13:01 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2007-03-13 18:55 x86_64 system lockup from userspace using setitimer() Johannes Bauer
2007-03-13 19:19 ` Andreas Schwab
2007-03-13 20:02 ` Chuck Ebbert
2007-03-13 20:33   ` Thomas Gleixner
2007-03-14 10:00     ` [PATCH] hrtimer: prevent overrun DoS in hrtimer_forward() Thomas Gleixner
2007-03-14 10:08       ` Ingo Molnar
2007-03-16 20:43       ` Andrew Morton
2007-03-16 21:05         ` Thomas Gleixner
2007-03-18 21:16           ` Chuck Ebbert
2007-03-18 21:32             ` Thomas Gleixner
2007-03-18 21:53               ` Chuck Ebbert
2007-03-18 22:04                 ` Thomas Gleixner
2007-03-18 22:02                   ` Chuck Ebbert
2007-04-04 21:11       ` Adrian Bunk
2007-04-04 21:30         ` Thomas Gleixner
2007-04-09 13:01           ` Adrian Bunk

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