linux-kernel - Re: [PATCH] time: adjtimex: validate the ADJ

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 3 Dec 2014 17:09:09 -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 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.

> +               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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/