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