Re: [PATCH] time: adjtimex: validate the ADJ_FREQUENCY case

[John Stultz]

On Wed, Dec 3, 2014 at 8:12 PM, John Stultz <john.stultz@xxxxxxxxxx> wrote:
> On Wed, Dec 3, 2014 at 6:40 PM, Sasha Levin <sasha.levin@xxxxxxxxxx> wrote:
>> On 12/03/2014 08:09 PM, John Stultz wrote:
>>> On Wed, Dec 3, 2014 at 4:25 PM, Sasha Levin <sasha.levin@xxxxxxxxxx> 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@xxxxxxxxxx>
>>>> ---
>>>>  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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/