Re: [PATCH] time: fix a assignment error in ntp module
Miroslav, On Mon, 17 Jun 2019, Miroslav Lichvar wrote: > On Mon, Jun 17, 2019 at 02:14:57PM +0200, Thomas Gleixner wrote: > > On Mon, 17 Jun 2019, 维康石 wrote: > > > Yes,the >UINT_MAX value can be passed by > > > syscall adjtimex->do_adjtimex->__do_adjtimex->process_adjtimex_modes by > > > the > > > proper arugments. > > > > So there is clearly some sanity check missing, but surely not that > > type cast. > > As the offset is saved in an int (and returned via adjtimex() in the > tai field), should be the maximum INT_MAX? Right. > We probably also want to avoid overflow in the offset on a leap second > and the CLOCK_TAI clock itself, so maybe it would make sense to > specify a much smaller maximum like 100? > > Even 1000 should be good enough for near future. Negative values are > not allowed anyway. If the Earth's rotation changed significantly > (e.g. hitting a very large asteroid), there probably wouldn't be > anyone left to care about TAI. Hehehe. I leave it to you to find a sane limit taking all the possible events into account :) Thanks, tglx
Re: [PATCH] time: fix a assignment error in ntp module
On Mon, Jun 17, 2019 at 02:14:57PM +0200, Thomas Gleixner wrote: > On Mon, 17 Jun 2019, 维康石 wrote: > > Yes,the >UINT_MAX value can be passed by > > syscall adjtimex->do_adjtimex->__do_adjtimex->process_adjtimex_modes by the > > proper arugments. > > So there is clearly some sanity check missing, but surely not that > type cast. As the offset is saved in an int (and returned via adjtimex() in the tai field), should be the maximum INT_MAX? We probably also want to avoid overflow in the offset on a leap second and the CLOCK_TAI clock itself, so maybe it would make sense to specify a much smaller maximum like 100? Even 1000 should be good enough for near future. Negative values are not allowed anyway. If the Earth's rotation changed significantly (e.g. hitting a very large asteroid), there probably wouldn't be anyone left to care about TAI. -- Miroslav Lichvar
Re: [PATCH] time: fix a assignment error in ntp module
On Mon, 17 Jun 2019, 维康石 wrote: A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing? A: Top-posting. Q: What is the most annoying thing in e-mail? A: No. Q: Should I include quotations after my reply? http://daringfireball.net/2007/07/on_top > Yes,the >UINT_MAX value can be passed by > syscall adjtimex->do_adjtimex->__do_adjtimex->process_adjtimex_modes by the > proper arugments. So there is clearly some sanity check missing, but surely not that type cast. > > > - if (txc->modes & ADJ_TAI && txc->constant > 0) > > > + if (txc->modes & ADJ_TAI && (int)txc->constant > 0) > > > *time_tai = txc->constant; > > > > The way more interesting question is whether txc->constant can be > > > UINT_MAX. In that case the txc->constant would be truncated. > > > > Miroslav? Thanks, tglx
Re: [PATCH] time: fix a assignment error in ntp module
On Mon, 22 Apr 2019, Weikang shi wrote: > From: swkhack > > It is meanless to check a 64bit(txc->constant) value is postive > when the value has to be assigned to a 32 bit variable(*time_tai). > So I make a temp type conversion before the compare. What? Casting it to int makes it more negative, right? That's just wrong: long long x = 0x; int y = (int) x; x is obviously negative, but y not. C type casting 101. > Signed-off-by: swkhack > --- > kernel/time/ntp.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c > index 92a90014a..6b454eafc 100644 > --- a/kernel/time/ntp.c > +++ b/kernel/time/ntp.c > @@ -690,7 +690,7 @@ static inline void process_adjtimex_modes(const struct > __kernel_timex *txc, > time_constant = max(time_constant, 0l); > } > > - if (txc->modes & ADJ_TAI && txc->constant > 0) > + if (txc->modes & ADJ_TAI && (int)txc->constant > 0) > *time_tai = txc->constant; The way more interesting question is whether txc->constant can be > UINT_MAX. In that case the txc->constant would be truncated. Miroslav? Thanks, tglx
Re: [PATCH] time: fix a assignment error in ntp module
On Mon, 22 Apr 2019, Weikang shi wrote: > From: swkhack > > It is meanless to check a 64bit(txc->constant) value is postive > when the value has to be assigned to a 32 bit variable(*time_tai). > So I make a temp type conversion before the compare. Errm no. This is missing a proper range check in the first place. > Signed-off-by: swkhack > --- > kernel/time/ntp.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c > index 92a90014a..6b454eafc 100644 > --- a/kernel/time/ntp.c > +++ b/kernel/time/ntp.c > @@ -690,7 +690,7 @@ static inline void process_adjtimex_modes(const struct > __kernel_timex *txc, > time_constant = max(time_constant, 0l); > } > > - if (txc->modes & ADJ_TAI && txc->constant > 0) > + if (txc->modes & ADJ_TAI && (int)txc->constant > 0) > *time_tai = txc->constant; > > if (txc->modes & ADJ_OFFSET) > -- > 2.17.1 > >
[PATCH] time: fix a assignment error in ntp module
From: swkhack It is meanless to check a 64bit(txc->constant) value is postive when the value has to be assigned to a 32 bit variable(*time_tai). So I make a temp type conversion before the compare. Signed-off-by: swkhack --- kernel/time/ntp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c index 92a90014a..6b454eafc 100644 --- a/kernel/time/ntp.c +++ b/kernel/time/ntp.c @@ -690,7 +690,7 @@ static inline void process_adjtimex_modes(const struct __kernel_timex *txc, time_constant = max(time_constant, 0l); } - if (txc->modes & ADJ_TAI && txc->constant > 0) + if (txc->modes & ADJ_TAI && (int)txc->constant > 0) *time_tai = txc->constant; if (txc->modes & ADJ_OFFSET) -- 2.17.1
