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 <[email protected]>
---
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
On Thu, Dec 3, 2015 at 7:09 PM, Sasha Levin <[email protected]> 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 <[email protected]>
Thanks for sending this in. I've queued it for 4.5
thanks
-john
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 <[email protected]>
> ---
> 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
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 <[email protected]>
>> ---
>> 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
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 <[email protected]>
> >> ---
> >> 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
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
On Sun, Dec 6, 2015 at 2:11 PM, Richard Cochran
<[email protected]> 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)
From: Sasha Levin <[email protected]>
Make sure the tv_usec makes sense. We might multiply them later which can
cause an overflow and undefined behavior.
Cc: Sasha Levin <[email protected]>
Cc: Richard Cochran <[email protected]>
Cc: Thomas Gleixner <[email protected]>,
Signed-off-by: Sasha Levin <[email protected]>
[jstultz: Moved corrected check to ntp_validate_timex]
Signed-off-by: John Stultz <[email protected]>
---
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
On Mon, Dec 7, 2015 at 12:11 PM, John Stultz <[email protected]> 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
On Mon, 7 Dec 2015, John Stultz wrote:
> On Sun, Dec 6, 2015 at 2:11 PM, Richard Cochran
> <[email protected]> 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
On Mon, 7 Dec 2015, John Stultz wrote:
> On Mon, Dec 7, 2015 at 12:11 PM, John Stultz <[email protected]> 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 :)
On Mon, Dec 7, 2015 at 12:15 PM, Thomas Gleixner <[email protected]> wrote:
> On Mon, 7 Dec 2015, John Stultz wrote:
>
>> On Sun, Dec 6, 2015 at 2:11 PM, Richard Cochran
>> <[email protected]> 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
On Mon, Dec 7, 2015 at 12:15 PM, Thomas Gleixner <[email protected]> wrote:
> On Mon, 7 Dec 2015, John Stultz wrote:
>
>> On Sun, Dec 6, 2015 at 2:11 PM, Richard Cochran
>> <[email protected]> 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