[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CALAqxLXR9kZwjuGW91=ocE-6wfwrfC4rimfsMxUbFjNcYQq13w@mail.gmail.com>
Date: Wed, 3 Dec 2014 20:12:24 -0800
From: John Stultz <john.stultz@...aro.org>
To: Sasha Levin <sasha.levin@...cle.com>
Cc: lkml <linux-kernel@...r.kernel.org>,
Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH] time: adjtimex: validate the ADJ_FREQUENCY case
On Wed, Dec 3, 2014 at 6:40 PM, Sasha Levin <sasha.levin@...cle.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@...cle.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@...cle.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists