Thomas,
I appreciate you reviewing my patches.
On Fri, May 24, 2024 at 5:09 AM Thomas Gleixner <t...@linutronix.de> wrote:
>
> On Fri, May 17 2024 at 20:22, Justin Stitt wrote:
> > time_maxerror is unconditionally incremented and the result is checked
> > against NTP_PHASE_LIMIT, but the increment itself can overflow,
> > resulting in wrap-around to negative space.
> >
> > The user can supply some crazy values which is causing the overflow. Add
> > an extra validation step checking that maxerror is reasonable.
>
> The user can supply any value which can cause an overflow as the input
> is unchecked. Add ...
>
> Hmm?
>
> > diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> > index b58dffc58a8f..321f251c02aa 100644
> > --- a/kernel/time/timekeeping.c
> > +++ b/kernel/time/timekeeping.c
> > @@ -2388,6 +2388,11 @@ static int timekeeping_validate_timex(const struct
> > __kernel_timex *txc)
> > }
> > }
> >
> > + if (txc->modes & ADJ_MAXERROR) {
> > + if (txc->maxerror < 0 || txc->maxerror > NTP_PHASE_LIMIT)
> > + return -EINVAL;
> > + }
>
> I dug into history to find a Fixes tag. That unearthed something
> interesting. Exactly this check used to be there until commit
> eea83d896e31 ("ntp: NTP4 user space bits update") which landed in
> 2.6.30. The change log says:
Thanks for doing the archaeology.
>
> "If some values for adjtimex() are outside the acceptable range, they
> are now simply normalized instead of letting the syscall fail."
>
> The problem with that commit is that it did not do any normalization at
> all and just relied on the actual time_maxerror handling in
> second_overflow(), which is both insufficient and also prone to that
> overflow issue.
>
> So instead of turning the clock back, we might be better off to actually
> put the normalization in place at the assignment:
>
> time_maxerror = min(max(0, txc->maxerror), NTP_PHASE_LIMIT);
A saturating resolution strategy is one that I've taken with some of
my other overflow patches.
... but how about: clamp(txc->maxerror, 0, NTP_PHASE_LIMIT)
>
> or something like that.
>
> Miroslav: Any opinion on that?
>
> Thanks,
>
> tglx
Anyways, I'm waiting to see how the whole overflow/wraparound
discussion in general evolves and, of course, how the local discussion
about this patch shapes up.
Thanks
Justin
