1. Patchew
  2. linux
  3. View series

[PATCH] ntp: safeguard against time_constant overflow case

Justin Stitt posted 1 patch 1 year, 7 months ago
There is a newer version of this series
kernel/time/ntp.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
[PATCH] ntp: safeguard against time_constant overflow case
Posted by Justin Stitt 1 year, 7 months ago 
Using syzkaller with the recently reintroduced signed integer overflow
sanitizer produces this UBSAN report:

[   46.809326] ------------[ cut here ]------------
[   46.812882] UBSAN: signed-integer-overflow in ../kernel/time/ntp.c:738:18
[   46.817676] 9223372036854775806 + 4 cannot be represented in type 'long'
[   46.822346] CPU: 1 PID: 685 Comm: syz-executor.0 Not tainted 6.8.0-rc2-00036-g679ee73ec453 #2
[   46.828270] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.3-debian-1.16.3-2 04/01/2014
[   46.834836] Call Trace:
[   46.836625]  <TASK>
[   46.838147]  dump_stack_lvl+0x93/0xd0
[   46.840771]  handle_overflow+0x171/0x1b0
[   46.843516]  __do_adjtimex+0x1236/0x1440
[   46.846275]  do_adjtimex+0x2be/0x740
[   46.848864]  __x64_sys_clock_adjtime+0x154/0x1d0
[   46.852164]  do_syscall_64+0xd7/0x1b0
[   46.854783]  ? arch_exit_to_user_mode_prepare+0x11/0x60
[   46.858426]  entry_SYSCALL_64_after_hwframe+0x6f/0x77
[   46.861914] RIP: 0033:0x7fde90aaf539
[   46.864500] Code: 28 00 00 00 75 05 48 83 c4 28 c3 e8 f1 14 00 00 90 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 8
[   46.877151] RSP: 002b:00007ffebfe63358 EFLAGS: 00000246 ORIG_RAX: 0000000000000131
[   46.882279] RAX: ffffffffffffffda RBX: 00007fde90be3f80 RCX: 00007fde90aaf539
[   46.887270] RDX: 0000000000000000 RSI: 0000000020000280 RDI: 0000000000000000
[   46.892174] RBP: 00007fde90b0e496 R08: 0000000000000000 R09: 0000000000000000
[   46.897061] R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
[   46.902020] R13: 0000000000000095 R14: 00007fde90be3f80 R15: 00007fde90be3f80
[   46.906946]  </TASK>
[   46.908537] ---[ end trace ]---

Historically, the signed integer overflow sanitizer did not work in the
kernel due to its interaction with `-fwrapv` but this has since been
changed [1] in the newest version of Clang; It being re-enabled in the
kernel with Commit 557f8c582a9ba8ab ("ubsan: Reintroduce signed overflow
sanitizer").

Nonetheless, let's slightly rework the logic surrounding time_constant
and how it is incremented such that we avoid unintentional wrap-around
(even though it is extremely unlikely to be hit in non-fuzzing scenarios).

[1]: https://github.com/llvm/llvm-project/pull/82432

Signed-off-by: Justin Stitt <justinstitt@google.com>
---
 kernel/time/ntp.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
index 406dccb79c2b..a9f039601968 100644
--- a/kernel/time/ntp.c
+++ b/kernel/time/ntp.c
@@ -65,6 +65,9 @@ static s64			time_offset;
 /* pll time constant:							*/
 static long			time_constant = 2;
 
+/* pll time constant increment:						*/
+static long			time_constant_inc = 4;
+
 /* maximum error (usecs):						*/
 static long			time_maxerror = NTP_PHASE_LIMIT;
 
@@ -734,10 +737,10 @@ static inline void process_adjtimex_modes(const struct __kernel_timex *txc,
 
 	if (txc->modes & ADJ_TIMECONST) {
 		time_constant = txc->constant;
-		if (!(time_status & STA_NANO))
-			time_constant += 4;
-		time_constant = min(time_constant, (long)MAXTC);
-		time_constant = max(time_constant, 0l);
+		if (!(time_status & STA_NANO) &&
+		    unlikely(LONG_MAX - time_constant_inc >= time_constant))
+			time_constant += time_constant_inc;
+		time_constant = clamp_t(long, time_constant, 0, MAXTC);
 	}
 
 	if (txc->modes & ADJ_TAI &&

---
base-commit: 0106679839f7c69632b3b9833c3268c316c0a9fc
change-id: 20240506-b4-sio-ntp-c-c227b02c65a3

Best regards,
--
Justin Stitt <justinstitt@google.com>
Re: [PATCH] ntp: safeguard against time_constant overflow case
Posted by Thomas Gleixner 1 year, 7 months ago 
On Mon, May 06 2024 at 22:01, Justin Stitt wrote:

> Using syzkaller with the recently reintroduced signed integer overflow
> sanitizer produces this UBSAN report:
>
> [   46.809326] ------------[ cut here ]------------
> [   46.812882] UBSAN: signed-integer-overflow in ../kernel/time/ntp.c:738:18
> [   46.817676] 9223372036854775806 + 4 cannot be represented in type 'long'
> [   46.822346] CPU: 1 PID: 685 Comm: syz-executor.0 Not tainted 6.8.0-rc2-00036-g679ee73ec453 #2
> [   46.828270] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.3-debian-1.16.3-2 04/01/2014
> [   46.834836] Call Trace:
> [   46.836625]  <TASK>
> [   46.838147]  dump_stack_lvl+0x93/0xd0
> [   46.840771]  handle_overflow+0x171/0x1b0
> [   46.843516]  __do_adjtimex+0x1236/0x1440
> [   46.846275]  do_adjtimex+0x2be/0x740
> [   46.848864]  __x64_sys_clock_adjtime+0x154/0x1d0
> [   46.852164]  do_syscall_64+0xd7/0x1b0
> [   46.854783]  ? arch_exit_to_user_mode_prepare+0x11/0x60
> [   46.858426]  entry_SYSCALL_64_after_hwframe+0x6f/0x77
> [   46.861914] RIP: 0033:0x7fde90aaf539
> [   46.864500] Code: 28 00 00 00 75 05 48 83 c4 28 c3 e8 f1 14 00 00 90 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 8
> [   46.877151] RSP: 002b:00007ffebfe63358 EFLAGS: 00000246 ORIG_RAX: 0000000000000131
> [   46.882279] RAX: ffffffffffffffda RBX: 00007fde90be3f80 RCX: 00007fde90aaf539
> [   46.887270] RDX: 0000000000000000 RSI: 0000000020000280 RDI: 0000000000000000
> [   46.892174] RBP: 00007fde90b0e496 R08: 0000000000000000 R09: 0000000000000000
> [   46.897061] R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
> [   46.902020] R13: 0000000000000095 R14: 00007fde90be3f80 R15: 00007fde90be3f80
> [   46.906946]  </TASK>
> [   46.908537] ---[ end trace ]---

Please trim stack traces so they contain only useful information.

 UBSAN: signed-integer-overflow in ../kernel/time/ntp.c:738:18
 9223372036854775806 + 4 cannot be represented in type 'long'
 Call Trace:
  <TASK>
  handle_overflow+0x171/0x1b0
  __do_adjtimex+0x1236/0x1440
  do_adjtimex+0x2be/0x740
  __x64_sys_clock_adjtime+0x154/0x1d0
  do_syscall_64+0xd7/0x1b0

Is completely sufficient, no?

> Historically, the signed integer overflow sanitizer did not work in the
> kernel due to its interaction with `-fwrapv` but this has since been
> changed [1] in the newest version of Clang; It being re-enabled in the
> kernel with Commit 557f8c582a9ba8ab ("ubsan: Reintroduce signed overflow
> sanitizer").

How is that relevant to the problem?

> Nonetheless, let's slightly rework the logic surrounding time_constant

s/Nonetheless, let's slightly /Rework/

> and how it is incremented such that we avoid unintentional wrap-around
> (even though it is extremely unlikely to be hit in non-fuzzing
> scenarios).

We don't avoid anything. Please write change logs in imperative mood.

>  	if (txc->modes & ADJ_TIMECONST) {
>  		time_constant = txc->constant;
> -		if (!(time_status & STA_NANO))
> -			time_constant += 4;
> -		time_constant = min(time_constant, (long)MAXTC);
> -		time_constant = max(time_constant, 0l);
> +		if (!(time_status & STA_NANO) &&
> +		    unlikely(LONG_MAX - time_constant_inc >= time_constant))

What's unlikely about this? Correct operation of adjtimex() will
increment, no?

As this obviously will be clamped to MAXTC anyway, you can spare that whole
LONG_MAX - time_constant_inc dance and simply do:

		if (!(time_status & STA_NANO) && time_constant < MAXTC)
                	time_constant += 4;

No?

> +			time_constant += time_constant_inc;
> +		time_constant = clamp_t(long, time_constant, 0, MAXTC);

Thanks,

        tglx
Re: [PATCH] ntp: safeguard against time_constant overflow case
Posted by John Stultz 1 year, 7 months ago 
On Mon, May 6, 2024 at 3:01 PM Justin Stitt <justinstitt@google.com> wrote:
>
> Nonetheless, let's slightly rework the logic surrounding time_constant
> and how it is incremented such that we avoid unintentional wrap-around
> (even though it is extremely unlikely to be hit in non-fuzzing scenarios).
>
> [1]: https://github.com/llvm/llvm-project/pull/82432
>
> Signed-off-by: Justin Stitt <justinstitt@google.com>
> ---
>  kernel/time/ntp.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
> index 406dccb79c2b..a9f039601968 100644
> --- a/kernel/time/ntp.c
> +++ b/kernel/time/ntp.c
> @@ -65,6 +65,9 @@ static s64                    time_offset;
>  /* pll time constant:                                                  */
>  static long                    time_constant = 2;
>
> +/* pll time constant increment:                                                */
> +static long                    time_constant_inc = 4;
> +

I'd probably use a `#define TIME_CONSTANT_INC 4` for this.

>  /* maximum error (usecs):                                              */
>  static long                    time_maxerror = NTP_PHASE_LIMIT;
>
> @@ -734,10 +737,10 @@ static inline void process_adjtimex_modes(const struct __kernel_timex *txc,
>
>         if (txc->modes & ADJ_TIMECONST) {
>                 time_constant = txc->constant;
> -               if (!(time_status & STA_NANO))
> -                       time_constant += 4;
> -               time_constant = min(time_constant, (long)MAXTC);
> -               time_constant = max(time_constant, 0l);
> +               if (!(time_status & STA_NANO) &&
> +                   unlikely(LONG_MAX - time_constant_inc >= time_constant))
> +                       time_constant += time_constant_inc;
> +               time_constant = clamp_t(long, time_constant, 0, MAXTC);
>         }

Overall, this looks fine. Though the time_status conditional is now a
little unwieldy.

I wonder if some sort of a helper like:
      time_constant = safe_add(time_constant, TIME_CONSTANT_INC, LONG_MAX);

Might make this a little easier to read?

thanks
-john
Re: [PATCH] ntp: safeguard against time_constant overflow case
Posted by Justin Stitt 1 year, 7 months ago 
Hi,

On Mon, May 06, 2024 at 11:02:17PM -0700, John Stultz wrote:
> On Mon, May 6, 2024 at 3:01 PM Justin Stitt <justinstitt@google.com> wrote:
> >
> > Nonetheless, let's slightly rework the logic surrounding time_constant
> > and how it is incremented such that we avoid unintentional wrap-around
> > (even though it is extremely unlikely to be hit in non-fuzzing scenarios).
> >
> > [1]: https://github.com/llvm/llvm-project/pull/82432
> >
> > Signed-off-by: Justin Stitt <justinstitt@google.com>
> > ---
> >  kernel/time/ntp.c | 11 +++++++----
> >  1 file changed, 7 insertions(+), 4 deletions(-)
> >
> > diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
> > index 406dccb79c2b..a9f039601968 100644
> > --- a/kernel/time/ntp.c
> > +++ b/kernel/time/ntp.c
> > @@ -65,6 +65,9 @@ static s64                    time_offset;
> >  /* pll time constant:                                                  */
> >  static long                    time_constant = 2;
> >
> > +/* pll time constant increment:                                                */
> > +static long                    time_constant_inc = 4;
> > +
> 
> I'd probably use a `#define TIME_CONSTANT_INC 4` for this.
> 
> >  /* maximum error (usecs):                                              */
> >  static long                    time_maxerror = NTP_PHASE_LIMIT;
> >
> > @@ -734,10 +737,10 @@ static inline void process_adjtimex_modes(const struct __kernel_timex *txc,
> >
> >         if (txc->modes & ADJ_TIMECONST) {
> >                 time_constant = txc->constant;
> > -               if (!(time_status & STA_NANO))
> > -                       time_constant += 4;
> > -               time_constant = min(time_constant, (long)MAXTC);
> > -               time_constant = max(time_constant, 0l);
> > +               if (!(time_status & STA_NANO) &&
> > +                   unlikely(LONG_MAX - time_constant_inc >= time_constant))
> > +                       time_constant += time_constant_inc;
> > +               time_constant = clamp_t(long, time_constant, 0, MAXTC);
> >         }
> 
> Overall, this looks fine. Though the time_status conditional is now a
> little unwieldy.
> 
> I wonder if some sort of a helper like:
>       time_constant = safe_add(time_constant, TIME_CONSTANT_INC, LONG_MAX);
> 
> Might make this a little easier to read?

How about something like this:

	if (txc->modes & ADJ_TIMECONST) {
		if (!(time_status & STA_NANO))
			time_constant = clamp_t(long, txc->constant,
						-TIME_CONSTANT_INC,
						MAXTC - TIME_CONSTANT_INC) +
						TIME_CONSTANT_INC;
		else
			time_constant = clamp_t(long, txc->constant, 0, MAXTC);
	}

We can remove the initial assignment and use some fancy clamps.

> 
> thanks
> -john

Thanks
Justin
Re: [PATCH] ntp: safeguard against time_constant overflow case
Posted by Thomas Gleixner 1 year, 7 months ago 
On Tue, May 07 2024 at 22:03, Justin Stitt wrote:
> On Mon, May 06, 2024 at 11:02:17PM -0700, John Stultz wrote:
>> > @@ -734,10 +737,10 @@ static inline void process_adjtimex_modes(const struct __kernel_timex *txc,
>> >
>> >         if (txc->modes & ADJ_TIMECONST) {
>> >                 time_constant = txc->constant;
>> > -               if (!(time_status & STA_NANO))
>> > -                       time_constant += 4;
>> > -               time_constant = min(time_constant, (long)MAXTC);
>> > -               time_constant = max(time_constant, 0l);
>> > +               if (!(time_status & STA_NANO) &&
>> > +                   unlikely(LONG_MAX - time_constant_inc >= time_constant))
>> > +                       time_constant += time_constant_inc;
>> > +               time_constant = clamp_t(long, time_constant, 0, MAXTC);
>> >         }
>> 
>> Overall, this looks fine. Though the time_status conditional is now a
>> little unwieldy.
>> 
>> I wonder if some sort of a helper like:
>>       time_constant = safe_add(time_constant, TIME_CONSTANT_INC, LONG_MAX);
>> 
>> Might make this a little easier to read?
>
> How about something like this:
>
> 	if (txc->modes & ADJ_TIMECONST) {
> 		if (!(time_status & STA_NANO))
> 			time_constant = clamp_t(long, txc->constant,
> 						-TIME_CONSTANT_INC,
> 						MAXTC - TIME_CONSTANT_INC) +
> 						TIME_CONSTANT_INC;
> 		else
> 			time_constant = clamp_t(long, txc->constant, 0, MAXTC);
> 	}
>
> We can remove the initial assignment and use some fancy clamps.

That's unreadable TBH.

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.