* [PATCH] time: adjtimex: validate the ADJ_FREQUENCY case @ 2014-12-04 0:25 Sasha Levin 2014-12-04 1:09 ` John Stultz 0 siblings, 1 reply; 5+ messages in thread From: Sasha Levin @ 2014-12-04 0:25 UTC (permalink / raw) To: linux-kernel; +Cc: Sasha Levin, John Stultz, Thomas Gleixner Verify that the frequency value from userspace is valid and makes sense. Unverified values can cause overflows later on. Signed-off-by: Sasha Levin <sasha.levin@oracle.com> --- kernel/time/ntp.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c index 87a346f..54828cf 100644 --- a/kernel/time/ntp.c +++ b/kernel/time/ntp.c @@ -633,6 +633,15 @@ int ntp_validate_timex(struct timex *txc) if ((txc->modes & ADJ_SETOFFSET) && (!capable(CAP_SYS_TIME))) return -EPERM; + if (txc->modes & ADJ_FREQUENCY) { + if (!capable(CAP_SYS_TIME)) + return -EPERM; + if (txc->freq < 0) + return -EINVAL; + if (LONG_MAX / PPM_SCALE < txc->freq) + return -EINVAL; + } + return 0; } -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] time: adjtimex: validate the ADJ_FREQUENCY case 2014-12-04 0:25 [PATCH] time: adjtimex: validate the ADJ_FREQUENCY case Sasha Levin @ 2014-12-04 1:09 ` John Stultz 2014-12-04 2:40 ` Sasha Levin 0 siblings, 1 reply; 5+ messages in thread From: John Stultz @ 2014-12-04 1:09 UTC (permalink / raw) To: Sasha Levin; +Cc: lkml, Thomas Gleixner On Wed, Dec 3, 2014 at 4:25 PM, Sasha Levin <sasha.levin@oracle.com> wrote: > Verify that the frequency value from userspace is valid and makes sense. > > Unverified values can cause overflows later on. > > Signed-off-by: Sasha Levin <sasha.levin@oracle.com> > --- > kernel/time/ntp.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c > index 87a346f..54828cf 100644 > --- a/kernel/time/ntp.c > +++ b/kernel/time/ntp.c > @@ -633,6 +633,15 @@ int ntp_validate_timex(struct timex *txc) > if ((txc->modes & ADJ_SETOFFSET) && (!capable(CAP_SYS_TIME))) > return -EPERM; > > + if (txc->modes & ADJ_FREQUENCY) { > + if (!capable(CAP_SYS_TIME)) > + return -EPERM; So does this actually change behavior? We check CAP_SYS_TIME if modes is set to anything a few lines above (with the exception of ADJ_ADJTIME which only allows for ADJ_OFFSET_SINGLESHOT or ADJ_OFFSET_READONLY). Granted, that logic isn't intuitive to read (and probably needs a cleanup) but seems ok. > + if (txc->freq < 0) > + return -EINVAL; ? Freq adjustments can be negative.... Am I just missing something here? > + if (LONG_MAX / PPM_SCALE < txc->freq) > + return -EINVAL; > + } This part seems reasonable though. We bound the output, but overflows could result in negative result when it was specified positive. I'm curious: I know many of your patches come from trinity issues, but this one isn't super clear in the commit message how it was found. Did an actually issue crop up here, or was this just something you came up with while looking at the 3.18rc hang problem? thanks -john ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] time: adjtimex: validate the ADJ_FREQUENCY case 2014-12-04 1:09 ` John Stultz @ 2014-12-04 2:40 ` Sasha Levin 2014-12-04 4:12 ` John Stultz 0 siblings, 1 reply; 5+ messages in thread From: Sasha Levin @ 2014-12-04 2:40 UTC (permalink / raw) To: John Stultz; +Cc: lkml, Thomas Gleixner On 12/03/2014 08:09 PM, John Stultz wrote: > On Wed, Dec 3, 2014 at 4:25 PM, Sasha Levin <sasha.levin@oracle.com> wrote: >> Verify that the frequency value from userspace is valid and makes sense. >> >> Unverified values can cause overflows later on. >> >> Signed-off-by: Sasha Levin <sasha.levin@oracle.com> >> --- >> kernel/time/ntp.c | 9 +++++++++ >> 1 file changed, 9 insertions(+) >> >> diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c >> index 87a346f..54828cf 100644 >> --- a/kernel/time/ntp.c >> +++ b/kernel/time/ntp.c >> @@ -633,6 +633,15 @@ int ntp_validate_timex(struct timex *txc) >> if ((txc->modes & ADJ_SETOFFSET) && (!capable(CAP_SYS_TIME))) >> return -EPERM; >> >> + if (txc->modes & ADJ_FREQUENCY) { >> + if (!capable(CAP_SYS_TIME)) >> + return -EPERM; > > So does this actually change behavior? We check CAP_SYS_TIME if modes > is set to anything a few lines above (with the exception of > ADJ_ADJTIME which only allows for ADJ_OFFSET_SINGLESHOT or > ADJ_OFFSET_READONLY). > > Granted, that logic isn't intuitive to read (and probably needs a > cleanup) but seems ok. No, it doesn't change behaviour. The logic, as you said, is a mess - so I tried to keep this change (I actually have a few more which look very similar) as readable and safe as possible >> + if (txc->freq < 0) >> + return -EINVAL; > > ? Freq adjustments can be negative.... Am I just missing something here? No, My bad, this should actually be: if (LONG_MIN / PPM_SCALE > txc->freq) return -EINVAL; >> + if (LONG_MAX / PPM_SCALE < txc->freq) >> + return -EINVAL; >> + } > > This part seems reasonable though. We bound the output, but overflows > could result in negative result when it was specified positive. The overflows could actually result in being anything, as this is considered undefined behaviour. > I'm curious: I know many of your patches come from trinity issues, but > this one isn't super clear in the commit message how it was found. Did > an actually issue crop up here, or was this just something you came up > with while looking at the 3.18rc hang problem? This is just me playing with the undefined behaviour/gcc5 patch and trinity, it doesn't have anything to do with the hang problem. Thanks, Sasha ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] time: adjtimex: validate the ADJ_FREQUENCY case 2014-12-04 2:40 ` Sasha Levin @ 2014-12-04 4:12 ` John Stultz 2014-12-18 21:23 ` John Stultz 0 siblings, 1 reply; 5+ messages in thread From: John Stultz @ 2014-12-04 4:12 UTC (permalink / raw) To: Sasha Levin; +Cc: lkml, Thomas Gleixner On Wed, Dec 3, 2014 at 6:40 PM, Sasha Levin <sasha.levin@oracle.com> wrote: > On 12/03/2014 08:09 PM, John Stultz wrote: >> On Wed, Dec 3, 2014 at 4:25 PM, Sasha Levin <sasha.levin@oracle.com> wrote: >>> Verify that the frequency value from userspace is valid and makes sense. >>> >>> Unverified values can cause overflows later on. >>> >>> Signed-off-by: Sasha Levin <sasha.levin@oracle.com> >>> --- >>> kernel/time/ntp.c | 9 +++++++++ >>> 1 file changed, 9 insertions(+) >>> >>> diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c >>> index 87a346f..54828cf 100644 >>> --- a/kernel/time/ntp.c >>> +++ b/kernel/time/ntp.c >>> @@ -633,6 +633,15 @@ int ntp_validate_timex(struct timex *txc) >>> if ((txc->modes & ADJ_SETOFFSET) && (!capable(CAP_SYS_TIME))) >>> return -EPERM; >>> >>> + if (txc->modes & ADJ_FREQUENCY) { >>> + if (!capable(CAP_SYS_TIME)) >>> + return -EPERM; >> >> So does this actually change behavior? We check CAP_SYS_TIME if modes >> is set to anything a few lines above (with the exception of >> ADJ_ADJTIME which only allows for ADJ_OFFSET_SINGLESHOT or >> ADJ_OFFSET_READONLY). >> >> Granted, that logic isn't intuitive to read (and probably needs a >> cleanup) but seems ok. > > No, it doesn't change behaviour. The logic, as you said, is a mess - so > I tried to keep this change (I actually have a few more which look very > similar) as readable and safe as possible Ok, could you maybe just add the (fixed) overflow check in one patch (which we'll need to backport to -stable) and we'll try to do a cleanup of the logic in a separate patch? >>> + if (txc->freq < 0) >>> + return -EINVAL; >> >> ? Freq adjustments can be negative.... Am I just missing something here? > > No, My bad, this should actually be: > > if (LONG_MIN / PPM_SCALE > txc->freq) > return -EINVAL; > >>> + if (LONG_MAX / PPM_SCALE < txc->freq) >>> + return -EINVAL; >>> + } >> >> This part seems reasonable though. We bound the output, but overflows >> could result in negative result when it was specified positive. > > The overflows could actually result in being anything, as this is considered > undefined behaviour. > >> I'm curious: I know many of your patches come from trinity issues, but >> this one isn't super clear in the commit message how it was found. Did >> an actually issue crop up here, or was this just something you came up >> with while looking at the 3.18rc hang problem? > > This is just me playing with the undefined behaviour/gcc5 patch and trinity, > it doesn't have anything to do with the hang problem. Ok, just curious. Thanks! -john ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] time: adjtimex: validate the ADJ_FREQUENCY case 2014-12-04 4:12 ` John Stultz @ 2014-12-18 21:23 ` John Stultz 0 siblings, 0 replies; 5+ messages in thread From: John Stultz @ 2014-12-18 21:23 UTC (permalink / raw) To: Sasha Levin; +Cc: lkml, Thomas Gleixner On Wed, Dec 3, 2014 at 8:12 PM, John Stultz <john.stultz@linaro.org> wrote: > On Wed, Dec 3, 2014 at 6:40 PM, Sasha Levin <sasha.levin@oracle.com> wrote: >> On 12/03/2014 08:09 PM, John Stultz wrote: >>> On Wed, Dec 3, 2014 at 4:25 PM, Sasha Levin <sasha.levin@oracle.com> wrote: >>>> Verify that the frequency value from userspace is valid and makes sense. >>>> >>>> Unverified values can cause overflows later on. >>>> >>>> Signed-off-by: Sasha Levin <sasha.levin@oracle.com> >>>> --- >>>> kernel/time/ntp.c | 9 +++++++++ >>>> 1 file changed, 9 insertions(+) >>>> >>>> diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c >>>> index 87a346f..54828cf 100644 >>>> --- a/kernel/time/ntp.c >>>> +++ b/kernel/time/ntp.c >>>> @@ -633,6 +633,15 @@ int ntp_validate_timex(struct timex *txc) >>>> if ((txc->modes & ADJ_SETOFFSET) && (!capable(CAP_SYS_TIME))) >>>> return -EPERM; >>>> >>>> + if (txc->modes & ADJ_FREQUENCY) { >>>> + if (!capable(CAP_SYS_TIME)) >>>> + return -EPERM; >>> >>> So does this actually change behavior? We check CAP_SYS_TIME if modes >>> is set to anything a few lines above (with the exception of >>> ADJ_ADJTIME which only allows for ADJ_OFFSET_SINGLESHOT or >>> ADJ_OFFSET_READONLY). >>> >>> Granted, that logic isn't intuitive to read (and probably needs a >>> cleanup) but seems ok. >> >> No, it doesn't change behaviour. The logic, as you said, is a mess - so >> I tried to keep this change (I actually have a few more which look very >> similar) as readable and safe as possible > > Ok, could you maybe just add the (fixed) overflow check in one patch > (which we'll need to backport to -stable) and we'll try to do a > cleanup of the logic in a separate patch? > > >>>> + if (txc->freq < 0) >>>> + return -EINVAL; >>> >>> ? Freq adjustments can be negative.... Am I just missing something here? >> >> No, My bad, this should actually be: >> >> if (LONG_MIN / PPM_SCALE > txc->freq) >> return -EINVAL; >> >>>> + if (LONG_MAX / PPM_SCALE < txc->freq) >>>> + return -EINVAL; >>>> + } >>> >>> This part seems reasonable though. We bound the output, but overflows >>> could result in negative result when it was specified positive. >> >> The overflows could actually result in being anything, as this is considered >> undefined behaviour. >> Just FYI, I've queued a fixed up version of your patch for testing. Will submit it w/ stable cc'ed once that's done. thanks -john ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-12-18 21:23 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-12-04 0:25 [PATCH] time: adjtimex: validate the ADJ_FREQUENCY case Sasha Levin 2014-12-04 1:09 ` John Stultz 2014-12-04 2:40 ` Sasha Levin 2014-12-04 4:12 ` John Stultz 2014-12-18 21:23 ` John Stultz
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.