linux-kernel - RE: [PATCH] Fix chance of sign extension to nsec after its msb is set during calculation.

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <VI1PR0501MB2461DA6733DC237223DBEFBFC5FC0@VI1PR0501MB2461.eurprd05.prod.outlook.com>
Date:   Sun, 11 Sep 2016 06:31:44 +0000
From:   Liav Rehana <liavr@...lanox.com>
To:     John Stultz <john.stultz@...aro.org>,
        Thomas Gleixner <tglx@...utronix.de>
CC:     lkml <linux-kernel@...r.kernel.org>,
        Noam Camus <noamca@...lanox.com>,
        Elad Kanfi <eladkan@...lanox.com>
Subject: RE: [PATCH] Fix chance of sign extension to nsec after its msb is set
 during calculation.

>> > > During the calculation of the nsec variable, "delta * tkr->mult" 
> >> > may cause overflow to the msb, if the suspended time is too long.
> >> > In that case, we need to guarantee that the variable will not go 
> >> > through a sign extension during its shift, and thus it will result 
> >> > in a much higher value - close to the larget value of 64 bits.
> >> > The following commit fixes this problem, which causes the following bug:
> >> > Trying to connect through ftp to the os after a long enough 
> >> > suspended time will cause the nsec variable to get a much higher 
> >> > value after its shift because of sign extension, and thus the loop 
> >> > that follows some instructions afterwards, implemented in the 
> >> > inline function __iter_div_u64_rem, will take too long.
> >> >
> >> > Signed-off-by: Liav Rehana <liavr@...lanox.com>
> >> > ---
> >> >  kernel/time/timekeeping.c |    2 +-
> >> >  1 files changed, 1 insertions(+), 1 deletions(-)
> >> >
> >> > diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c 
> >> > index 479d25c..ddf56a5 100644
> >> > --- a/kernel/time/timekeeping.c
> >> > +++ b/kernel/time/timekeeping.c
> >> > @@ -305,7 +305,7 @@ static inline s64 timekeeping_delta_to_ns(struct tk_read_base *tkr,
> >> >     s64 nsec;
> >> >
> >> >     nsec = delta * tkr->mult + tkr->xtime_nsec;
> >> > -   nsec >>= tkr->shift;
> >> > +   nsec = ((u64) nsec) >> tkr->shift;
> >>
> >> This typecast is just a baindaid. What happens if you double the suspend time?
> >> The multiplication will simply overflow. So the proper fix is to 
> >> sanity check delta and do multiple conversions if delta is big 
> >> enough. Preferrably this happens somewhere at the call site and not in this hotpath function.
> >
> > As a side note. John, why is that stuff unsigned at all? Shouldn't we 
> > use
> > u64 for all of this?
>
>Errrr... My memory is quite foggy here. I think we just started way back when with s64 to catch cases where there were negative nsec intervals. Looking through the git logs it seems its > been that way since the beginning of the generic timekeeping logic.
>
>For most cases here u64 is fine. There may be a few cases where we do have valid negative nanosecond intervals, but I can't think of any off the top of my head, and those can >probably be special cased.

In light of your comment for that issue, I would like to change the type of the nsec variable to u64, as it will solve the sign extension problem. For that, I intend to change the type
of that variable in the functions that define it, and in the struct that uses it in kernel/time/timekeeping.c.
Do you think there are other references I should change. Or do you think there is a better solution ? Please provide your opinion on this matter.

Thanks,
	Liav.