2015-12-03 20:47:12

by Sasha Levin

Subject: [PATCH] ntp: verify offset doesn't overflow in ntp_update_offset

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 <[email protected]>
---
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

2015-12-04 20:26:32

by John Stultz

Subject: Re: [PATCH] ntp: verify offset doesn't overflow in ntp_update_offset

On Thu, Dec 3, 2015 at 12:46 PM, Sasha Levin <[email protected]> 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 <[email protected]>

Thanks for sending this in. I've queued it for 4.5

thanks
-john

2015-12-08 00:02:46

by John Stultz

Subject: Re: [PATCH] ntp: verify offset doesn't overflow in ntp_update_offset

On Thu, Dec 3, 2015 at 12:46 PM, Sasha Levin <[email protected]> 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 <[email protected]>
> ---
> 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

2015-12-08 00:22:32

by Sasha Levin

Subject: Re: [PATCH] ntp: verify offset doesn't overflow in ntp_update_offset

On 12/07/2015 07:02 PM, John Stultz wrote:
> On Thu, Dec 3, 2015 at 12:46 PM, Sasha Levin <[email protected]> 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 <[email protected]>
>> > ---
>> > 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