[PATCH v2] ntp: safeguard against time_constant overflow case

[PATCH v2] ntp: safeguard against time_constant overflow case

[Justin Stitt]

Using syzkaller with the recently reintroduced signed integer overflow
sanitizer produces this UBSAN report:

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

Rework the logic surrounding time_constant and how it is incremented so
that it doesn't wrap-around.

Link: https://github.com/llvm/llvm-project/pull/82432 [1]
Closes: https://github.com/KSPP/linux/issues/352
Cc: linux-hardening@vger.kernel.org
Signed-off-by: Justin Stitt <justinstitt@google.com>
---
Changes in v2:
- Adjust commit log (thanks Thomas)
- massively simplify bounds checking for time_constant
- Link to v1: https://lore.kernel.org/r/20240506-b4-sio-ntp-c-v1-1-a01281aa01ba@google.com
---
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 was re-enabled in the
kernel with Commit 557f8c582a9ba8ab ("ubsan: Reintroduce signed overflow
sanitizer").

Here's the syzkaller reproducer:
| # {Threaded:false Repeat:false RepeatTimes:0 Procs:1 Slowdown:1 Sandbox:
| # SandboxArg:0 Leak:false NetInjection:false NetDevices:false
| # NetReset:false Cgroups:false BinfmtMisc:false CloseFDs:false KCSAN:false
| # DevlinkPCI:false NicVF:false USB:false VhciInjection:false Wifi:false
| # IEEE802154:false Sysctl:false Swap:false UseTmpDir:false
| # HandleSegv:false Repro:false Trace:false LegacyOptions:{Collide:false
| # Fault:false FaultCall:0 FaultNth:0}}
| clock_adjtime(0x0, &(0x7f0000000280)={0xff, 0x0, 0x0, 0x0, 0x0, 0x0, 0x7ffffffffffffffe})

... which was used against Kees' tree here (v6.8rc2):
https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/log/?h=wip/v6.9-rc2/unsigned-overflow-sanitizer

... with this config:
https://gist.github.com/JustinStitt/824976568b0f228ccbcbe49f3dee9bf4
---
 kernel/time/ntp.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
index 406dccb79c2b..d64f69e14938 100644
--- a/kernel/time/ntp.c
+++ b/kernel/time/ntp.c
@@ -733,8 +733,7 @@ static inline void process_adjtimex_modes(const struct __kernel_timex *txc,
 		time_esterror = txc->esterror;
 
 	if (txc->modes & ADJ_TIMECONST) {
-		time_constant = txc->constant;
-		if (!(time_status & STA_NANO))
+		if (!(time_status & STA_NANO) && time_constant < MAXTC)
 			time_constant += 4;
 		time_constant = min(time_constant, (long)MAXTC);
 		time_constant = max(time_constant, 0l);

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

Best regards,
--
Justin Stitt <justinstitt@google.com>

Re: [PATCH v2] ntp: safeguard against time_constant overflow case

[Thomas Gleixner]

Justin!

On Fri, May 17 2024 at 00:47, Justin Stitt wrote:
>  	if (txc->modes & ADJ_TIMECONST) {
> -		time_constant = txc->constant;
> -		if (!(time_status & STA_NANO))
> +		if (!(time_status & STA_NANO) && time_constant < MAXTC)
>  			time_constant += 4;
>  		time_constant = min(time_constant, (long)MAXTC);
>  		time_constant = max(time_constant, 0l);

Let me digest this.

The original code does:

	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);

Your change results in:

	if (!(time_status & STA_NANO) && time_constant < MAXTC)
		time_constant += 4;
	time_constant = min(time_constant, (long)MAXTC);
	time_constant = max(time_constant, 0l);

IOW, you lost the intent of the code to assign the user space supplied
value of txc->constant.

Aside of that you clearly failed to map the deep analysis I provided to
you vs. the time_maxerror issue to this one:

# git grep 'time_constant.*=' kernel/time/
ntp.c:66:static long                    time_constant = 2;

  That's the static initializer

kernel/time/ntp.c:736:              time_constant = txc->constant;
kernel/time/ntp.c:738:                      time_constant += 4;
kernel/time/ntp.c:739:              time_constant = min(time_constant, (long)MAXTC);
kernel/time/ntp.c:740:              time_constant = max(time_constant, 0l);

  That's the part of process_adjtimex_modes() you are trying to
  "fix". So it's exactly the same problem as with time_maxerror, no?

And therefore you provide a "safeguard" against overflow for the price of
making the syscall disfunctional. Seriously?

Did you even try to run something else than the bad case reproducer
against your fix?

No. You did not. Any of the related real use case tests would have
failed.

I told you yesterday:

   Tools are good to pin-point symptoms, but they are by definition
   patently bad in root cause analysis. Otherwise we could just let the
   tool write the "fix".

Such a tool would have at least produced a correct "fix" to cure the
symptom.

Thanks,

        tglx

Re: [PATCH v2] ntp: safeguard against time_constant overflow case

[Thomas Gleixner]

On Fri, May 17 2024 at 22:18, Justin Stitt wrote:
> On Fri, May 17, 2024, 19:33 Thomas Gleixner <tglx@linutronix.de> wrote:
> I accidentally sent a Frankstein-esque creation of two patches I was
> working on. Not my brightest moment. It got past my testing because (as you
> pointed out) I only ran the reproducer against my _fix_...

Shit happens.

> Let me really parse everything you've said and v3 will surely knock your
> socks off. You'll have to wait till Monday though :)

Take your time. There is no rush.

Thanks,

        tglx

[tip: timers/urgent] ntp: Safeguard against time_constant overflow

[tip-bot2 for Justin Stitt]

The following commit has been merged into the timers/urgent branch of tip:

Commit-ID:     06c03c8edce333b9ad9c6b207d93d3a5ae7c10c0
Gitweb:        https://git.kernel.org/tip/06c03c8edce333b9ad9c6b207d93d3a5ae7c10c0
Author:        Justin Stitt <justinstitt@google.com>
AuthorDate:    Fri, 17 May 2024 00:47:10 
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Mon, 05 Aug 2024 16:14:14 +02:00

ntp: Safeguard against time_constant overflow

Using syzkaller with the recently reintroduced signed integer overflow
sanitizer produces this UBSAN report:

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

The user supplied time_constant value is incremented by four and then
clamped to the operating range.

Before commit eea83d896e31 ("ntp: NTP4 user space bits update") the user
supplied value was sanity checked to be in the operating range. That change
removed the sanity check and relied on clamping after incrementing which
does not work correctly when the user supplied value is in the overflow
zone of the '+ 4' operation.

The operation requires CAP_SYS_TIME and the side effect of the overflow is
NTP getting out of sync.

Similar to the fixups for time_maxerror and time_esterror, clamp the user
space supplied value to the operating range.

[ tglx: Switch to clamping ]

Fixes: eea83d896e31 ("ntp: NTP4 user space bits update")
Signed-off-by: Justin Stitt <justinstitt@google.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Miroslav Lichvar <mlichvar@redhat.com>
Cc: stable@vger.kernel.org
Link: https://lore.kernel.org/all/20240517-b4-sio-ntp-c-v2-1-f3a80096f36f@google.com
Closes: https://github.com/KSPP/linux/issues/352
---
 kernel/time/ntp.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
index 502e1e5..8d2dd21 100644
--- a/kernel/time/ntp.c
+++ b/kernel/time/ntp.c
@@ -733,11 +733,10 @@ static inline void process_adjtimex_modes(const struct __kernel_timex *txc,
 		time_esterror = clamp(txc->esterror, 0, NTP_PHASE_LIMIT);
 
 	if (txc->modes & ADJ_TIMECONST) {
-		time_constant = txc->constant;
+		time_constant = clamp(txc->constant, 0, MAXTC);
 		if (!(time_status & STA_NANO))
 			time_constant += 4;
-		time_constant = min(time_constant, (long)MAXTC);
-		time_constant = max(time_constant, 0l);
+		time_constant = clamp(time_constant, 0, MAXTC);
 	}
 
 	if (txc->modes & ADJ_TAI &&