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

[Sasha Levin <sasha.levin@oracle.com>]

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