NAK: [SRU][Trusty][PATCH v2 1/1] posix-timers: Sanitize overrun handling
Kleber Souza
kleber.souza at canonical.com
Wed Nov 28 16:38:17 UTC 2018
On 11/28/18 6:19 AM, Khalid Elmously wrote:
> From: Thomas Gleixner <tglx at linutronix.de>
>
> CVE-2018-12896
>
> The posix timer overrun handling is broken because the forwarding functions
> can return a huge number of overruns which does not fit in an int. As a
> consequence timer_getoverrun(2) and siginfo::si_overrun can turn into
> random number generators.
>
> The k_clock::timer_forward() callbacks return a 64 bit value now. Make
> k_itimer::ti_overrun[_last] 64bit as well, so the kernel internal
> accounting is correct. 3Remove the temporary (int) casts.
>
> Add a helper function which clamps the overrun value returned to user space
> via timer_getoverrun(2) or siginfo::si_overrun limited to a positive value
> between 0 and INT_MAX. INT_MAX is an indicator for user space that the
> overrun value has been clamped.
>
> Reported-by: Team OWL337 <icytxw at gmail.com>
> Signed-off-by: Thomas Gleixner <tglx at linutronix.de>
> Acked-by: John Stultz <john.stultz at linaro.org>
> Cc: Peter Zijlstra <peterz at infradead.org>
> Cc: Michael Kerrisk <mtk.manpages at gmail.com>
> Link: https://lkml.kernel.org/r/20180626132705.018623573@linutronix.de
> (backported from commit 78c9c4dfbf8c04883941445a195276bb4bb92c76)
When some changes more than simple context adjustments are needed,
please add a few words explaining what was needed for the backport. In
this case it would be relevant to mention that the path of some files
were different (e.g. kernel/time/posix-cpu-timers.c ->
kernel/posix-cpu-timers.c), etc. That helps the people reviewing and
looking at the code later to easily spot the difference from the
original patch.
> Signed-off-by: Khalid Elmously <khalid.elmously at canonical.com>
> ---
> include/linux/posix-timers.h | 4 ++--
> kernel/posix-cpu-timers.c | 8 ++++----
> kernel/posix-timers.c | 31 ++++++++++++++++++++-----------
> 3 files changed, 26 insertions(+), 17 deletions(-)
>
> diff --git a/include/linux/posix-timers.h b/include/linux/posix-timers.h
> index 907f3fd191ac..3e28a1a8d823 100644
> --- a/include/linux/posix-timers.h
> +++ b/include/linux/posix-timers.h
> @@ -65,8 +65,8 @@ struct k_itimer {
> spinlock_t it_lock;
> clockid_t it_clock; /* which timer type */
> timer_t it_id; /* timer id */
> - int it_overrun; /* overrun on pending signal */
> - int it_overrun_last; /* overrun on last delivered signal */
> + s64 it_overrun; /* overrun on pending signal */
> + s64 it_overrun_last; /* overrun on last delivered signal */
> int it_requeue_pending; /* waiting to requeue this timer */
> #define REQUEUE_PENDING 1
> int it_sigev_notify; /* notify word of sigevent struct */
> diff --git a/kernel/posix-cpu-timers.c b/kernel/posix-cpu-timers.c
> index c7f31aa272f7..0f62265a707c 100644
> --- a/kernel/posix-cpu-timers.c
> +++ b/kernel/posix-cpu-timers.c
> @@ -103,7 +103,7 @@ static void bump_cpu_timer(struct k_itimer *timer,
> continue;
>
> timer->it.cpu.expires += incr;
> - timer->it_overrun += 1 << i;
> + timer->it_overrun += 1LL << i;
> delta -= incr;
> }
> }
> @@ -761,7 +761,7 @@ static int posix_cpu_timer_set(struct k_itimer *timer, int flags,
> timer->it_requeue_pending = (timer->it_requeue_pending + 2) &
> ~REQUEUE_PENDING;
> timer->it_overrun_last = 0;
> - timer->it_overrun = -1;
> + timer->it_overrun = -1LL;
>
> if (new_expires != 0 && !(val < new_expires)) {
> /*
> @@ -1119,7 +1119,7 @@ out_unlock:
>
> out:
> timer->it_overrun_last = timer->it_overrun;
> - timer->it_overrun = -1;
> + timer->it_overrun = -1LL;
> ++timer->it_requeue_pending;
> }
>
> @@ -1327,7 +1327,7 @@ static int do_cpu_nanosleep(const clockid_t which_clock, int flags,
> memset(&timer, 0, sizeof timer);
> spin_lock_init(&timer.it_lock);
> timer.it_clock = which_clock;
> - timer.it_overrun = -1;
> + timer.it_overrun = -1LL;
I know the above changes and some below from -1 to -1LL on the
assignments makes sense to have a consistency with the changes made on
the other files, but unless there's a very strong reason to divert from
mainline we tend to not make changes on the code that wasn't on the
original patch and would make the code different from mainline. The
reason for this is that cherry-picking or backporting patches in the
future can demand some extra work because of the diversion. If you
really think this should be changed you can send a patch upstream
suggesting it :-).
So that's why I'm NAK'ing it and will do for the Xenial one as well.
Could you please resend with changes to make it as close as possible to
the original patch?
Thanks,
Kleber
> error = posix_cpu_timer_create(&timer);
> timer.it_process = current;
> if (!error) {
> diff --git a/kernel/posix-timers.c b/kernel/posix-timers.c
> index 26b910462a0c..8ef1ab65fe84 100644
> --- a/kernel/posix-timers.c
> +++ b/kernel/posix-timers.c
> @@ -346,6 +346,17 @@ static __init int init_posix_timers(void)
>
> __initcall(init_posix_timers);
>
> +/*
> + * The siginfo si_overrun field and the return value of timer_getoverrun(2)
> + * are of type int. Clamp the overrun value to INT_MAX
> + */
> +static inline int timer_overrun_to_int(struct k_itimer *timr, int baseval)
> +{
> + s64 sum = timr->it_overrun_last + (s64)baseval;
> +
> + return sum > (s64)INT_MAX ? INT_MAX : (int)sum;
> +}
> +
> static void schedule_next_timer(struct k_itimer *timr)
> {
> struct hrtimer *timer = &timr->it.real.timer;
> @@ -353,12 +364,11 @@ static void schedule_next_timer(struct k_itimer *timr)
> if (timr->it.real.interval.tv64 == 0)
> return;
>
> - timr->it_overrun += (unsigned int) hrtimer_forward(timer,
> - timer->base->get_time(),
> - timr->it.real.interval);
> + timr->it_overrun += hrtimer_forward(timer, timer->base->get_time(),
> + timr->it.real.interval);
>
> timr->it_overrun_last = timr->it_overrun;
> - timr->it_overrun = -1;
> + timr->it_overrun = -1LL;
> ++timr->it_requeue_pending;
> hrtimer_restart(timer);
> }
> @@ -387,7 +397,7 @@ void do_schedule_next_timer(struct siginfo *info)
> else
> schedule_next_timer(timr);
>
> - info->si_overrun += timr->it_overrun_last;
> + info->si_overrun = timer_overrun_to_int(timr, info->si_overrun);
> }
>
> if (timr)
> @@ -482,9 +492,8 @@ static enum hrtimer_restart posix_timer_fn(struct hrtimer *timer)
> now = ktime_add(now, kj);
> }
> #endif
> - timr->it_overrun += (unsigned int)
> - hrtimer_forward(timer, now,
> - timr->it.real.interval);
> + timr->it_overrun += hrtimer_forward(timer, now,
> + timr->it.real.interval);
> ret = HRTIMER_RESTART;
> ++timr->it_requeue_pending;
> }
> @@ -624,7 +633,7 @@ SYSCALL_DEFINE3(timer_create, const clockid_t, which_clock,
> it_id_set = IT_ID_SET;
> new_timer->it_id = (timer_t) new_timer_id;
> new_timer->it_clock = which_clock;
> - new_timer->it_overrun = -1;
> + new_timer->it_overrun = -1LL;
>
> if (timer_event_spec) {
> if (copy_from_user(&event, timer_event_spec, sizeof (event))) {
> @@ -754,7 +763,7 @@ common_timer_get(struct k_itimer *timr, struct itimerspec *cur_setting)
> */
> if (iv.tv64 && (timr->it_requeue_pending & REQUEUE_PENDING ||
> (timr->it_sigev_notify & ~SIGEV_THREAD_ID) == SIGEV_NONE))
> - timr->it_overrun += (unsigned int) hrtimer_forward(timer, now, iv);
> + timr->it_overrun += hrtimer_forward(timer, now, iv);
>
> remaining = ktime_sub(hrtimer_get_expires(timer), now);
> /* Return 0 only, when the timer is expired and not pending */
> @@ -816,7 +825,7 @@ SYSCALL_DEFINE1(timer_getoverrun, timer_t, timer_id)
> if (!timr)
> return -EINVAL;
>
> - overrun = timr->it_overrun_last;
> + overrun = timer_overrun_to_int(timr, 0);
> unlock_timer(timr, flags);
>
> return overrun;
More information about the kernel-team
mailing list