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
| ||
|
Date: Wed, 21 Sep 2016 12:52:11 +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. PING -----Original Message----- From: Liav Rehana Sent: Sunday, September 11, 2016 9:32 AM 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.
Powered by blists - more mailing lists