[v2] ntp: safeguard against time_constant overflow case

Commit Message

Justin Stitt May 17, 2024, 12:47 a.m. UTC

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: [e-mail]
Signed-off-by: Justin Stitt <[e-mail]>
---
Changes in v2:
- Adjust commit log (thanks Thomas)
- massively simplify bounds checking for time_constant
- Link to v1: https://lore.kernel.org/r/[e-mail]
---
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(-)


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

Best regards,
--
Justin Stitt <[e-mail]>

Comments

Thomas Gleixner May 18, 2024, 2:33 a.m. UTC | #1

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

Thomas Gleixner May 19, 2024, 11:31 a.m. UTC | #2

On Fri, May 17 2024 at 22:18, Justin Stitt wrote:
> On Fri, May 17, 2024, 19:33 Thomas Gleixner <[e-mail]> 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

Patch

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