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

[John Stultz]

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.

> +               if (txc->freq < 0)
> +                       return -EINVAL;

?  Freq adjustments can be negative....  Am I just missing something here?

> +               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.

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?

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/