[SRU][Trusty][PATCH 1/1] posix-timers: Sanitize overrun handling
Khalid Elmously
khalid.elmously at canonical.com
Wed Nov 28 04:19:07 UTC 2018
On 2018-11-27 12:10:51 , Tyler Hicks wrote:
> On 2018-11-27 00:23:17, 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)
> > 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 | 29 +++++++++++++++++++----------
> > 3 files changed, 25 insertions(+), 16 deletions(-)
>
> This seems like some potentially hair backports to get right. I'm still
> going through the Xenial and Trusty patches but I noticed that you
> dropped the hunk that removes the (unsigned int) cast in
> common_timer_get() for the Trusty backport. I don't think that was
> intentional but let me know if it was.
Yes, that was a mistake on my part. Thank you for catching it! (I don't think the cast is harmful anyway, but better to be consistent)
Will send a v2
>
> Tyler
>
> >
> > 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;
> > 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..2577e33bc964 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))) {
> > @@ -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;
> > --
> > 2.17.1
> >
> >
> > --
> > kernel-team mailing list
> > kernel-team at lists.ubuntu.com
> > https://lists.ubuntu.com/mailman/listinfo/kernel-team
More information about the kernel-team
mailing list