Messages in this thread Patch in this message | | | Date | Wed, 14 Mar 2012 22:16:32 +0100 (CET) | From | Thomas Gleixner <> | Subject | Re: [PATCH] ntp: Fix integer overflow when setting time |
| |
On Wed, 14 Mar 2012, Sasha Levin wrote:
> On 64bit machines, the difference between 'time_reftime' and current time > is 8 bits, this variable is used as the divisor in div_s64() to calculate > the frequency, but div_s64() assumes the divisor is 32bits.
Why is that delta 8 bits????
> This means that we are able to trigger a division by zero if the difference > has 0's in it's lower bytes. This way it would skip the sanity checks before > the div_64s() but when it gets to that div it would get truncated and > become 0.
This does not make sense at all.
So what I assume you are talking about is, that the divisor (i.e. the delta) is greater than (1 << 32) - 1 and all 32 lower bits are 0, right ?
> diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c > index 17fb1b9..efe894a 100644 > --- a/kernel/time/ntp.c > +++ b/kernel/time/ntp.c > @@ -289,7 +289,7 @@ static inline s64 ntp_update_offset_fll(s64 offset64, long secs) > > time_status |= STA_MODE; > > - return div_s64(offset64 << (NTP_SCALE_SHIFT - SHIFT_FLL), secs); > + return div64_s64(offset64 << (NTP_SCALE_SHIFT - SHIFT_FLL), secs);
This needs a comment and it would be nice to avoid the 64 division on 32bit machines. Something like:
#if BITS_PER_LONG == 64 # define div64_long(x,y) div64_s64(x,y) #else # define div64_long(x,y) div_s64(x,y) #endif
Nice catch, though it took me a while to grok the changelog, as the 8 bit case is catched by the MINSEC check. Please be more careful about that.
Thanks,
tglx
| |