Received-SPF: pass (google.com: domain of linux-kernel+bounces-178562-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
From: Thomas Gleixner <tglx@linutronix.de>
To: Justin Stitt <justinstitt@google.com>, John Stultz <jstultz@google.com>,
 Stephen Boyd <sboyd@kernel.org>, Nathan Chancellor <nathan@kernel.org>,
 Bill Wendling <morbo@google.com>
Cc: linux-kernel@vger.kernel.org, llvm@lists.linux.dev,
 linux-hardening@vger.kernel.org, Justin Stitt <justinstitt@google.com>
Subject: Re: [PATCH] ntp: remove accidental integer wrap-around
In-Reply-To: <20240507-b4-sio-ntp-usec-v1-1-15003fc9c2b4@google.com>
References: <20240507-b4-sio-ntp-usec-v1-1-15003fc9c2b4@google.com>
Date: Tue, 14 May 2024 12:38:03 +0200
Message-ID: <87v83gllv8.ffs@tglx>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain

On Tue, May 07 2024 at 04:34, Justin Stitt wrote:
> Using syzkaller alongside the newly reintroduced signed integer overflow
> sanitizer spits out this report:
>
> [  138.454979] ------------[ cut here ]------------
> [  138.458089] UBSAN: signed-integer-overflow in ../kernel/time/ntp.c:461:16
> [  138.462134] 9223372036854775807 + 500 cannot be represented in type 'long'
> [  138.466234] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 6.8.0-rc2-00038-gc0a509640e93-dirty #10
> [  138.471498] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.3-debian-1.16.3-2 04/01/2014
> [  138.477110] Call Trace:
> [  138.478657]  <IRQ>
> [  138.479964]  dump_stack_lvl+0x93/0xd0
> [  138.482276]  handle_overflow+0x171/0x1b0
> [  138.484699]  second_overflow+0x2d6/0x500
> [  138.487133]  accumulate_nsecs_to_secs+0x60/0x160
> [  138.489931]  timekeeping_advance+0x1fe/0x890
> [  138.492535]  update_wall_time+0x10/0x30

Same comment vs. trimming.

> 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").

Again. Irrelevant to the problem.

> Let's introduce a new macro and use that against NTP_PHASE_LIMIT to
> properly limit the max size of time_maxerror without overflowing during
> the check itself.

This fails to tell what is causing the issue and just talks about what
the patch is doing. The latter can be seen from the patch itself, no?

Something like this:

   On second overflow time_maxerror is unconditionally incremented and
   the result is checked against NTP_PHASE_LIMIT, but the increment can
   overflow into negative space.

   Prevent this by checking the overflow condition before incrementing.

Hmm?

But that obviously begs the question why this can happen at all.

#define MAXPHASE 500000000L       
#define NTP_PHASE_LIMIT ((MAXPHASE / NSEC_PER_USEC) << 5)

==> NTP_PHASE_LIMIT = 1.6e+07 = 0xf42400

#define MAXFREQ 500000

So how can 0xf42400 + 500000/1000 overflow in the first place?

It can't unless time_maxerror is somehow initialized to a bogus
value and indeed it is:

process_adjtimex_modes()
        ....
	if (txc->modes & ADJ_MAXERROR)
		time_maxerror = txc->maxerror;

So that wants to be fixed and not the symptom.

Thanks,

        tglx