[PATCH] time: verify time values in adjtimex ADJ_SETOFFSET to avoid overflow

[PATCH] time: verify time values in adjtimex ADJ_SETOFFSET to avoid overflow

[Sasha Levin]

Make sure the tv_usec makes sense. We might multiply them later which can
cause an overflow and undefined behavior.

Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
---
 kernel/time/timekeeping.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index d563c19..aa3c1c2 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -1987,6 +1987,10 @@ int do_adjtimex(struct timex *txc)
 
 	if (txc->modes & ADJ_SETOFFSET) {
 		struct timespec delta;
+
+		if (txc->time.tv_usec >= USEC_PER_SEC || txc->time.tv_usec <= -USEC_PER_SEC)
+			return -EINVAL;
+
 		delta.tv_sec  = txc->time.tv_sec;
 		delta.tv_nsec = txc->time.tv_usec;
 		if (!(txc->modes & ADJ_NANO))
-- 
1.7.10.4

Re: [PATCH] time: verify time values in adjtimex ADJ_SETOFFSET to avoid overflow

[John Stultz]

On Thu, Dec 3, 2015 at 7:09 PM, Sasha Levin <sasha.levin@oracle.com> wrote:
> Make sure the tv_usec makes sense. We might multiply them later which can
> cause an overflow and undefined behavior.
>
> Signed-off-by: Sasha Levin <sasha.levin@oracle.com>

Thanks for sending this in. I've queued it for 4.5

thanks
-john

Re: [PATCH] time: verify time values in adjtimex ADJ_SETOFFSET to avoid overflow

[Thomas Gleixner]

On Thu, 3 Dec 2015, Sasha Levin wrote:

> Make sure the tv_usec makes sense. We might multiply them later which can
> cause an overflow and undefined behavior.
> 
> Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
> ---
>  kernel/time/timekeeping.c |    4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> index d563c19..aa3c1c2 100644
> --- a/kernel/time/timekeeping.c
> +++ b/kernel/time/timekeeping.c
> @@ -1987,6 +1987,10 @@ int do_adjtimex(struct timex *txc)
>  
>  	if (txc->modes & ADJ_SETOFFSET) {
>  		struct timespec delta;
> +
> +		if (txc->time.tv_usec >= USEC_PER_SEC || txc->time.tv_usec <= -USEC_PER_SEC)
> +			return -EINVAL;

That's not a canonical timeval. timeval_valid() is what you want to
check it. Or has adjtimex some magic exception here?

Thanks,

	tglx

Re: [PATCH] time: verify time values in adjtimex ADJ_SETOFFSET to avoid overflow

[Sasha Levin]

On 12/05/2015 12:10 PM, Thomas Gleixner wrote:
> On Thu, 3 Dec 2015, Sasha Levin wrote:
> 
>> Make sure the tv_usec makes sense. We might multiply them later which can
>> cause an overflow and undefined behavior.
>>
>> Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
>> ---
>>  kernel/time/timekeeping.c |    4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
>> index d563c19..aa3c1c2 100644
>> --- a/kernel/time/timekeeping.c
>> +++ b/kernel/time/timekeeping.c
>> @@ -1987,6 +1987,10 @@ int do_adjtimex(struct timex *txc)
>>  
>>  	if (txc->modes & ADJ_SETOFFSET) {
>>  		struct timespec delta;
>> +
>> +		if (txc->time.tv_usec >= USEC_PER_SEC || txc->time.tv_usec <= -USEC_PER_SEC)
>> +			return -EINVAL;
> 
> That's not a canonical timeval. timeval_valid() is what you want to
> check it. Or has adjtimex some magic exception here?

Nope, it looks like timeval_valid() is indeed what I've needed to use.

Is there a reason ntp_validate_timex() doesn't do timeval_valid() too
for at least the ADJ_SETOFFSET case? If not, I'll add it in.


Thanks,
Sasha

Re: [PATCH] time: verify time values in adjtimex ADJ_SETOFFSET to avoid overflow

[Thomas Gleixner]

On Sat, 5 Dec 2015, Sasha Levin wrote:
> On 12/05/2015 12:10 PM, Thomas Gleixner wrote:
> > On Thu, 3 Dec 2015, Sasha Levin wrote:
> > 
> >> Make sure the tv_usec makes sense. We might multiply them later which can
> >> cause an overflow and undefined behavior.
> >>
> >> Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
> >> ---
> >>  kernel/time/timekeeping.c |    4 ++++
> >>  1 file changed, 4 insertions(+)
> >>
> >> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> >> index d563c19..aa3c1c2 100644
> >> --- a/kernel/time/timekeeping.c
> >> +++ b/kernel/time/timekeeping.c
> >> @@ -1987,6 +1987,10 @@ int do_adjtimex(struct timex *txc)
> >>  
> >>  	if (txc->modes & ADJ_SETOFFSET) {
> >>  		struct timespec delta;
> >> +
> >> +		if (txc->time.tv_usec >= USEC_PER_SEC || txc->time.tv_usec <= -USEC_PER_SEC)
> >> +			return -EINVAL;
> > 
> > That's not a canonical timeval. timeval_valid() is what you want to
> > check it. Or has adjtimex some magic exception here?
> 
> Nope, it looks like timeval_valid() is indeed what I've needed to use.
> 
> Is there a reason ntp_validate_timex() doesn't do timeval_valid() too
> for at least the ADJ_SETOFFSET case? If not, I'll add it in.

Not that I know, but John might have some opinion on that.

Thanks,

	tglx

Re: [PATCH] time: verify time values in adjtimex ADJ_SETOFFSET to avoid overflow

[Richard Cochran]

On Sun, Dec 06, 2015 at 10:07:01AM +0100, Thomas Gleixner wrote:
> On Sat, 5 Dec 2015, Sasha Levin wrote:
> > On 12/05/2015 12:10 PM, Thomas Gleixner wrote:
> > > That's not a canonical timeval. timeval_valid() is what you want to
> > > check it. Or has adjtimex some magic exception here?

You can't use timeval_valid for ADJ_SETOFFSET, because the seconds
field can also be negative (when setting the time back).

> > Nope, it looks like timeval_valid() is indeed what I've needed to use.
> > 
> > Is there a reason ntp_validate_timex() doesn't do timeval_valid() too
> > for at least the ADJ_SETOFFSET case? If not, I'll add it in.

So you should not use timeval_valid, and the original patch is also
not right.  The rule is:

	The value of a timeval is the sum of its fields, but the
	field tv_usec must always be non-negative.

We had a discussion about this a year or two ago.  Maybe I can find it
again.

The overflow is a latent problem, and the patch should:

1. return error in case (txc->time.tv_usec >= USEC_PER_SEC)
2. remove the redundant test in timekeeping_inject_offset.

Thanks,
Richard

Re: [PATCH] time: verify time values in adjtimex ADJ_SETOFFSET to avoid overflow

[John Stultz]

On Sun, Dec 6, 2015 at 2:11 PM, Richard Cochran
<richardcochran@gmail.com> wrote:
> The overflow is a latent problem, and the patch should:
>
> 1. return error in case (txc->time.tv_usec >= USEC_PER_SEC)
> 2. remove the redundant test in timekeeping_inject_offset.

So we probably want to keep the check in timekeeping_inject_offset()
since there can be other users as well of that function.

But its probably cleanest to add a check in ntp_validate_timex()
instead of where this patch does it.

thanks
-john

(And thanks Thomas for taking a second look here)

[RFC][PATCH -reworked] time: Verify time values in adjtimex ADJ_SETOFFSET to avoid overflow

[John Stultz]

From: Sasha Levin <sasha.levin@oracle.com>

Make sure the tv_usec makes sense. We might multiply them later which can
cause an overflow and undefined behavior.

Cc: Sasha Levin <sasha.levin@oracle.com>
Cc: Richard Cochran <richardcochran@gmail.com>
Cc: Thomas Gleixner <tglx@linutronix.de>,
Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
[jstultz: Moved corrected check to ntp_validate_timex]
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
Here's my attempt at reworking the patch.
Let me know if you have any thoughts or objections.
thanks
-john


 kernel/time/ntp.c         | 14 ++++++++++++--
 kernel/time/timekeeping.c |  1 +
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
index 36616c3..e9a1874 100644
--- a/kernel/time/ntp.c
+++ b/kernel/time/ntp.c
@@ -676,8 +676,18 @@ int ntp_validate_timex(struct timex *txc)
 			return -EINVAL;
 	}
 
-	if ((txc->modes & ADJ_SETOFFSET) && (!capable(CAP_SYS_TIME)))
-		return -EPERM;
+	if (txc->modes & ADJ_SETOFFSET) {
+		/* In order to inject time, you gotta be super-user! */
+		if (!capable(CAP_SYS_TIME))
+			return -EPERM;
+
+		/*
+		 * tv_sec can be positive or negative, but usec
+		 * must be positive and from 0->USEC_PER_SEC
+		 */
+		if (txc->time.tv_usec >= USEC_PER_SEC)
+			return -EINVAL;
+	}
 
 	/*
 	 * Check for potential multiplication overflows that can
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 99188ee..a37222b 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -1986,6 +1986,7 @@ int do_adjtimex(struct timex *txc)
 
 	if (txc->modes & ADJ_SETOFFSET) {
 		struct timespec delta;
+
 		delta.tv_sec  = txc->time.tv_sec;
 		delta.tv_nsec = txc->time.tv_usec;
 		if (!(txc->modes & ADJ_NANO))
-- 
1.9.1

Re: [RFC][PATCH -reworked] time: Verify time values in adjtimex ADJ_SETOFFSET to avoid overflow

[John Stultz]

On Mon, Dec 7, 2015 at 12:11 PM, John Stultz <john.stultz@linaro.org> wrote:
> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> index 99188ee..a37222b 100644
> --- a/kernel/time/timekeeping.c
> +++ b/kernel/time/timekeeping.c
> @@ -1986,6 +1986,7 @@ int do_adjtimex(struct timex *txc)
>
>         if (txc->modes & ADJ_SETOFFSET) {
>                 struct timespec delta;
> +
>                 delta.tv_sec  = txc->time.tv_sec;
>                 delta.tv_nsec = txc->time.tv_usec;
>                 if (!(txc->modes & ADJ_NANO))

Gah. Except for this needless whitespace. Ignore that please.

thanks
-john

Re: [PATCH] time: verify time values in adjtimex ADJ_SETOFFSET to avoid overflow

[Thomas Gleixner]

On Mon, 7 Dec 2015, John Stultz wrote:

> On Sun, Dec 6, 2015 at 2:11 PM, Richard Cochran
> <richardcochran@gmail.com> wrote:
> > The overflow is a latent problem, and the patch should:
> >
> > 1. return error in case (txc->time.tv_usec >= USEC_PER_SEC)
> > 2. remove the redundant test in timekeeping_inject_offset.
> 
> So we probably want to keep the check in timekeeping_inject_offset()
> since there can be other users as well of that function.
> 
> But its probably cleanest to add a check in ntp_validate_timex()
> instead of where this patch does it.

So instead of open coding the checks on both sites, can we please have
an inline function with proper comments why time.tv_sec can be
negative, something like adjtimex_timeval_is_valid() or such.

Thanks,

	tglx

Re: [RFC][PATCH -reworked] time: Verify time values in adjtimex ADJ_SETOFFSET to avoid overflow

[Thomas Gleixner]

On Mon, 7 Dec 2015, John Stultz wrote:
> On Mon, Dec 7, 2015 at 12:11 PM, John Stultz <john.stultz@linaro.org> wrote:
> > diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> > index 99188ee..a37222b 100644
> > --- a/kernel/time/timekeeping.c
> > +++ b/kernel/time/timekeeping.c
> > @@ -1986,6 +1986,7 @@ int do_adjtimex(struct timex *txc)
> >
> >         if (txc->modes & ADJ_SETOFFSET) {
> >                 struct timespec delta;
> > +
> >                 delta.tv_sec  = txc->time.tv_sec;
> >                 delta.tv_nsec = txc->time.tv_usec;
> >                 if (!(txc->modes & ADJ_NANO))
> 
> Gah. Except for this needless whitespace. Ignore that please.

It's needless for this patch, but correct by itself :)

Re: [PATCH] time: verify time values in adjtimex ADJ_SETOFFSET to avoid overflow

[John Stultz]

On Mon, Dec 7, 2015 at 12:15 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Mon, 7 Dec 2015, John Stultz wrote:
>
>> On Sun, Dec 6, 2015 at 2:11 PM, Richard Cochran
>> <richardcochran@gmail.com> wrote:
>> > The overflow is a latent problem, and the patch should:
>> >
>> > 1. return error in case (txc->time.tv_usec >= USEC_PER_SEC)
>> > 2. remove the redundant test in timekeeping_inject_offset.
>>
>> So we probably want to keep the check in timekeeping_inject_offset()
>> since there can be other users as well of that function.
>>
>> But its probably cleanest to add a check in ntp_validate_timex()
>> instead of where this patch does it.
>
> So instead of open coding the checks on both sites, can we please have
> an inline function with proper comments why time.tv_sec can be
> negative, something like adjtimex_timeval_is_valid() or such.

Sure. I'll respin with that.

thanks
-john

Re: [PATCH] time: verify time values in adjtimex ADJ_SETOFFSET to avoid overflow

[John Stultz]

On Mon, Dec 7, 2015 at 12:15 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Mon, 7 Dec 2015, John Stultz wrote:
>
>> On Sun, Dec 6, 2015 at 2:11 PM, Richard Cochran
>> <richardcochran@gmail.com> wrote:
>> > The overflow is a latent problem, and the patch should:
>> >
>> > 1. return error in case (txc->time.tv_usec >= USEC_PER_SEC)
>> > 2. remove the redundant test in timekeeping_inject_offset.
>>
>> So we probably want to keep the check in timekeeping_inject_offset()
>> since there can be other users as well of that function.
>>
>> But its probably cleanest to add a check in ntp_validate_timex()
>> instead of where this patch does it.
>
> So instead of open coding the checks on both sites, can we please have
> an inline function with proper comments why time.tv_sec can be
> negative, something like adjtimex_timeval_is_valid() or such.

Right. So the only gotcha with this is that adjtimex wants to check
that the timeval is valid (before we convert it to a timespec), but
timekeeping_inject_offset wants to make sure the timespec is valid. So
one nice inline function won't cut it.

But I can add a timespec_inject_offset_valid() and
timeval_inject_offset_valid() which will do the same basic check for
each type.

thanks
-john