* [PATCH] ntp: verify offset doesn't overflow in ntp_update_offset
@ 2015-12-03 20:46 Sasha Levin
2015-12-04 20:26 ` John Stultz
2015-12-08 0:02 ` John Stultz
0 siblings, 2 replies; 4+ messages in thread
From: Sasha Levin @ 2015-12-03 20:46 UTC (permalink / raw)
To: john.stultz, tglx; +Cc: linux-kernel, Sasha Levin
We need to make sure that the offset is valid before manipulating it,
otherwise it might overflow on the multiplication.
Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
---
kernel/time/ntp.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
index 149cc80..36616c3 100644
--- a/kernel/time/ntp.c
+++ b/kernel/time/ntp.c
@@ -297,6 +297,9 @@ static void ntp_update_offset(long offset)
if (!(time_status & STA_PLL))
return;
+ /* Make sure the multiplication below won't overflow */
+ offset = clamp(offset, -MAXPHASE, MAXPHASE);
+
if (!(time_status & STA_NANO))
offset *= NSEC_PER_USEC;
@@ -304,8 +307,7 @@ static void ntp_update_offset(long offset)
* Scale the phase adjustment and
* clamp to the operating range.
*/
- offset = min(offset, MAXPHASE);
- offset = max(offset, -MAXPHASE);
+ offset = clamp(offset, -MAXPHASE, MAXPHASE);
/*
* Select how the frequency is to be controlled
--
1.7.10.4
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] ntp: verify offset doesn't overflow in ntp_update_offset
2015-12-03 20:46 [PATCH] ntp: verify offset doesn't overflow in ntp_update_offset Sasha Levin
@ 2015-12-04 20:26 ` John Stultz
2015-12-08 0:02 ` John Stultz
1 sibling, 0 replies; 4+ messages in thread
From: John Stultz @ 2015-12-04 20:26 UTC (permalink / raw)
To: Sasha Levin; +Cc: Thomas Gleixner, lkml
On Thu, Dec 3, 2015 at 12:46 PM, Sasha Levin <sasha.levin@oracle.com> wrote:
> We need to make sure that the offset is valid before manipulating it,
> otherwise it might overflow on the multiplication.
>
> Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
Thanks for sending this in. I've queued it for 4.5
thanks
-john
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] ntp: verify offset doesn't overflow in ntp_update_offset
2015-12-03 20:46 [PATCH] ntp: verify offset doesn't overflow in ntp_update_offset Sasha Levin
2015-12-04 20:26 ` John Stultz
@ 2015-12-08 0:02 ` John Stultz
2015-12-08 0:22 ` Sasha Levin
1 sibling, 1 reply; 4+ messages in thread
From: John Stultz @ 2015-12-08 0:02 UTC (permalink / raw)
To: Sasha Levin; +Cc: Thomas Gleixner, lkml
On Thu, Dec 3, 2015 at 12:46 PM, Sasha Levin <sasha.levin@oracle.com> wrote:
> We need to make sure that the offset is valid before manipulating it,
> otherwise it might overflow on the multiplication.
>
> Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
> ---
> kernel/time/ntp.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
> index 149cc80..36616c3 100644
> --- a/kernel/time/ntp.c
> +++ b/kernel/time/ntp.c
> @@ -297,6 +297,9 @@ static void ntp_update_offset(long offset)
> if (!(time_status & STA_PLL))
> return;
>
> + /* Make sure the multiplication below won't overflow */
> + offset = clamp(offset, -MAXPHASE, MAXPHASE);
> +
> if (!(time_status & STA_NANO))
> offset *= NSEC_PER_USEC;
So looking at this a bit closer, this bit looks sort of crazy since we
clam the offset, do the multiply and then do the exact same clamp.
I'd much rather do a more logical clamp(offset, -USEC_PER_SEC,
USEC_PER_SEC), but only in the case where we do the multiply.
Any objection to that?
thanks
-john
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] ntp: verify offset doesn't overflow in ntp_update_offset
2015-12-08 0:02 ` John Stultz
@ 2015-12-08 0:22 ` Sasha Levin
0 siblings, 0 replies; 4+ messages in thread
From: Sasha Levin @ 2015-12-08 0:22 UTC (permalink / raw)
To: John Stultz; +Cc: Thomas Gleixner, lkml
On 12/07/2015 07:02 PM, John Stultz wrote:
> On Thu, Dec 3, 2015 at 12:46 PM, Sasha Levin <sasha.levin@oracle.com> wrote:
>> > We need to make sure that the offset is valid before manipulating it,
>> > otherwise it might overflow on the multiplication.
>> >
>> > Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
>> > ---
>> > kernel/time/ntp.c | 6 ++++--
>> > 1 file changed, 4 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
>> > index 149cc80..36616c3 100644
>> > --- a/kernel/time/ntp.c
>> > +++ b/kernel/time/ntp.c
>> > @@ -297,6 +297,9 @@ static void ntp_update_offset(long offset)
>> > if (!(time_status & STA_PLL))
>> > return;
>> >
>> > + /* Make sure the multiplication below won't overflow */
>> > + offset = clamp(offset, -MAXPHASE, MAXPHASE);
>> > +
>> > if (!(time_status & STA_NANO))
>> > offset *= NSEC_PER_USEC;
> So looking at this a bit closer, this bit looks sort of crazy since we
> clam the offset, do the multiply and then do the exact same clamp.
>
> I'd much rather do a more logical clamp(offset, -USEC_PER_SEC,
> USEC_PER_SEC), but only in the case where we do the multiply.
>
> Any objection to that?
Nope. Sounds right.
Thanks,
Sasha
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2015-12-08 0:22 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-12-03 20:46 [PATCH] ntp: verify offset doesn't overflow in ntp_update_offset Sasha Levin
2015-12-04 20:26 ` John Stultz
2015-12-08 0:02 ` John Stultz
2015-12-08 0:22 ` Sasha Levin
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.