NAK: [SRU][Trusty][PATCH v2 1/1] posix-timers: Sanitize overrun handling
Khalid Elmously
khalid.elmously at canonical.com
Thu Nov 29 05:42:50 UTC 2018
On 2018-11-28 17:38:17 , Kleber Souza wrote:
> 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.
Ack
> > 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 for the explanation and for the review.
I will send an update
Khalid
>
> 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