lkml.org 
[lkml]   [2014]   [Dec]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH] time: adjtimex: validate the ADJ_FREQUENCY case
From
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


\
 
 \ /
  Last update: 2014-12-04 05:41    [W:0.051 / U:0.028 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site