ntp: fix bug in adjtimex reading time offset

[PATCH] ntp: fix bug in adjtimex reading time offset

Posted by Dalmas, Marcelo (GE Vernova) 1 year, 2 months ago

Due to an unsigned cast, adjtimex returns wrong offest when using ADJ_MICRO and the offset is negative.
In this case a small negative offset return approximately 4.29 seconds (~ 2^32/1000 milliseconds).

Signed-off-by: Marcelo Dalmas <marcelo.dalmas@ge.com>
---
 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 b550ebe0f03b..02e7fe6a0afa 100644
--- a/kernel/time/ntp.c
+++ b/kernel/time/ntp.c
@@ -798,7 +798,7 @@ int __do_adjtimex(struct __kernel_timex *txc, const struct timespec64 *ts,

                txc->offset = shift_right(ntpdata->time_offset * NTP_INTERVAL_FREQ, NTP_SCALE_SHIFT);
                if (!(ntpdata->time_status & STA_NANO))
-                       txc->offset = (u32)txc->offset / NSEC_PER_USEC;
+                       txc->offset /= NSEC_PER_USEC;
        }

        result = ntpdata->time_state;
--
2.39.5

Re: [PATCH] ntp: fix bug in adjtimex reading time offset

Posted by Thomas Gleixner 1 year, 2 months ago

On Mon, Nov 25 2024 at 12:16, Marcelo Dalmas wrote:
> Due to an unsigned cast, adjtimex returns wrong offest when using ADJ_MICRO and the offset is negative.
> In this case a small negative offset return approximately 4.29 seconds (~ 2^32/1000 milliseconds).

Nice find. Please format the changelog to be within ~72 characters per
line next time.

> Signed-off-by: Marcelo Dalmas <marcelo.dalmas@ge.com>
> ---
>  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 b550ebe0f03b..02e7fe6a0afa 100644
> --- a/kernel/time/ntp.c
> +++ b/kernel/time/ntp.c
> @@ -798,7 +798,7 @@ int __do_adjtimex(struct __kernel_timex *txc, const struct timespec64 *ts,
>
>                 txc->offset = shift_right(ntpdata->time_offset * NTP_INTERVAL_FREQ, NTP_SCALE_SHIFT);
>                 if (!(ntpdata->time_status & STA_NANO))
> -                       txc->offset = (u32)txc->offset / NSEC_PER_USEC;
> +                       txc->offset /= NSEC_PER_USEC;

This patch is white space damaged. (tabs are replaced with spaces).

I fixed it up to you this time. Please send your next patch to yourself
first and verify that it applies cleanly.

Thanks,

        tglx

Re: [PATCH] ntp: fix bug in adjtimex reading time offset

Posted by Thomas Gleixner 1 year, 2 months ago

On Wed, Nov 27 2024 at 15:10, Thomas Gleixner wrote:

> On Mon, Nov 25 2024 at 12:16, Marcelo Dalmas wrote:
>> Due to an unsigned cast, adjtimex returns wrong offest when using ADJ_MICRO and the offset is negative.
>> In this case a small negative offset return approximately 4.29 seconds (~ 2^32/1000 milliseconds).
>
> Nice find. Please format the changelog to be within ~72 characters per
> line next time.
>
>> Signed-off-by: Marcelo Dalmas <marcelo.dalmas@ge.com>
>> ---
>>  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 b550ebe0f03b..02e7fe6a0afa 100644
>> --- a/kernel/time/ntp.c
>> +++ b/kernel/time/ntp.c
>> @@ -798,7 +798,7 @@ int __do_adjtimex(struct __kernel_timex *txc, const struct timespec64 *ts,
>>
>>                 txc->offset = shift_right(ntpdata->time_offset * NTP_INTERVAL_FREQ, NTP_SCALE_SHIFT);
>>                 if (!(ntpdata->time_status & STA_NANO))
>> -                       txc->offset = (u32)txc->offset / NSEC_PER_USEC;
>> +                       txc->offset /= NSEC_PER_USEC;
>
> This patch is white space damaged. (tabs are replaced with spaces).
>
> I fixed it up to you this time. Please send your next patch to yourself
> first and verify that it applies cleanly.

And it triggers a compile fail on 32bit as txc::offset is of type 'long
long'.

Re: [PATCH] ntp: fix bug in adjtimex reading time offset

Posted by Richard Cochran 1 year, 2 months ago

On Wed, Nov 27, 2024 at 03:10:30PM +0100, Thomas Gleixner wrote:
> On Mon, Nov 25 2024 at 12:16, Marcelo Dalmas wrote:
> > Due to an unsigned cast, adjtimex returns wrong offest when using ADJ_MICRO and the offset is negative.
> > In this case a small negative offset return approximately 4.29 seconds (~ 2^32/1000 milliseconds).
> 
> Nice find.

How did this slip in?  Git blame tells:

    commit ead25417f82ed7f8a21da4dcefc768169f7da884
    Author: Deepa Dinamani <deepa.kernel@gmail.com>
    Date:   Mon Jul 2 22:44:21 2018 -0700

    timex: use __kernel_timex internally
    
    struct timex is not y2038 safe.
    Replace all uses of timex with y2038 safe __kernel_timex.
    
    ...
    
    The patch was generated by the following coccinelle script:

    ...

So I guess combining random other manual fixes into a patch that
claims to be generated is a bad idea?

Thanks,
Richard

Re: [PATCH] ntp: fix bug in adjtimex reading time offset

Posted by Thomas Gleixner 1 year, 2 months ago

On Wed, Nov 27 2024 at 08:05, Richard Cochran wrote:
> On Wed, Nov 27, 2024 at 03:10:30PM +0100, Thomas Gleixner wrote:
>     The patch was generated by the following coccinelle script:
>
>     ...
>
> So I guess combining random other manual fixes into a patch that
> claims to be generated is a bad idea?

Indeed. I just figured out why the cast is there. txc::time_offset is of
type 'long long', so the division triggers a build fail on 32-bit.

It want's to be:

   txc->time_offset = div_s64(txc->time_offset, NSEC_PER_USEC);

....

Re: [PATCH] ntp: fix bug in adjtimex reading time offset

Posted by Thomas Gleixner 1 year, 2 months ago

On Wed, Nov 27 2024 at 18:35, Thomas Gleixner wrote:
> On Wed, Nov 27 2024 at 08:05, Richard Cochran wrote:
>> On Wed, Nov 27, 2024 at 03:10:30PM +0100, Thomas Gleixner wrote:
>>     The patch was generated by the following coccinelle script:
>>
>>     ...
>>
>> So I guess combining random other manual fixes into a patch that
>> claims to be generated is a bad idea?
>
> Indeed. I just figured out why the cast is there. txc::time_offset is of
> type 'long long', so the division triggers a build fail on 32-bit.
>
> It want's to be:
>
>    txc->time_offset = div_s64(txc->time_offset, NSEC_PER_USEC);

Or simpler by using a (s32) cast instead, which is sufficient as
time_offset must be in the range of [INT_MIN ... INT_MAX] because
NTP_SCALE_SHIFT is 32.

Re: [PATCH] ntp: fix bug in adjtimex reading time offset

Posted by Thomas Gleixner 1 year, 2 months ago

On Wed, Nov 27 2024 at 18:39, Thomas Gleixner wrote:
> On Wed, Nov 27 2024 at 18:35, Thomas Gleixner wrote:
>> On Wed, Nov 27 2024 at 08:05, Richard Cochran wrote:
>>> On Wed, Nov 27, 2024 at 03:10:30PM +0100, Thomas Gleixner wrote:
>>>     The patch was generated by the following coccinelle script:
>>>
>>>     ...
>>>
>>> So I guess combining random other manual fixes into a patch that
>>> claims to be generated is a bad idea?
>>
>> Indeed. I just figured out why the cast is there. txc::time_offset is of
>> type 'long long', so the division triggers a build fail on 32-bit.
>>
>> It want's to be:
>>
>>    txc->time_offset = div_s64(txc->time_offset, NSEC_PER_USEC);
>
> Or simpler by using a (s32) cast instead, which is sufficient as
> time_offset must be in the range of [INT_MIN ... INT_MAX] because
> NTP_SCALE_SHIFT is 32.

But that requires a comment while div_s64() is self explaining and it
does not matter performance wise as the 32bit implementations reduce it
to a 32bit/32bit division.

Thanks,

        tglx