NetBSD Problem Report #58927

From www@netbsd.org  Sat Dec 21 17:10:16 2024
Return-Path: <www@netbsd.org>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
	(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
	 key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256
	 client-signature RSA-PSS (2048 bits) client-digest SHA256)
	(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id CA6961A923A
	for <gnats-bugs@gnats.NetBSD.org>; Sat, 21 Dec 2024 17:10:16 +0000 (UTC)
Message-Id: <20241221171015.05E641A923B@mollari.NetBSD.org>
Date: Sat, 21 Dec 2024 17:10:14 +0000 (UTC)
From: campbell+netbsd@mumble.net
Reply-To: campbell+netbsd@mumble.net
To: gnats-bugs@NetBSD.org
Subject: itimer(9): overrun accounting is broken
X-Send-Pr-Version: www-1.0

>Number:         58927
>Category:       kern
>Synopsis:       itimer(9): overrun accounting is broken
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    kern-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Sat Dec 21 17:15:00 +0000 2024
>Originator:     Taylor R Campbell
>Release:        current, 10
>Organization:
The TimeBSD Overrundation
>Environment:
>Description:
If you set a real-time periodic interval timer with timer_create(CLOCK_REALTIME) and timer_settime, and then the real-time clock is wound forward (e.g., via ntp adjustment), timer_getoverrun is supposed to return the number of missed events (up to DELAYTIMER_MAX, which we define to be 32).

But in an attempt to fix a arithmetic overflow, I broke what I thought was an arithmetic overflow check and was actually a clock-wound-forward check in kern_time.c itimer_callout:

Module Name:    src
Committed By:   riastradh
Date:           Mon Jun 27 00:34:24 UTC 2022

Modified Files:
        src/sys/kern: kern_time.c

Log Message:
setitimer(2): Avoid arithmetic overflow in periodic bookkeeping.

Reported-by: syzbot+93cef6090844ec304cde%syzkaller.appspotmail.com@localhost

https://mail-index.netbsd.org/source-changes/2022/06/27/msg139421.html

 	backwards = (timespeccmp(&it->it_time.it_value, &now, >));
-	timespecadd(&it->it_time.it_value, &it->it_time.it_interval, &next);
+
+	/* Nonnegative interval guaranteed by itimerfix.  */
+	KASSERT(it->it_time.it_interval.tv_sec >= 0);
+	KASSERT(it->it_time.it_interval.tv_nsec >= 0);
+
 	/* Handle the easy case of non-overflown timers first. */
-	if (!backwards && timespeccmp(&next, &now, >)) {
+	if (!backwards &&
+	    timespecaddok(&it->it_time.it_value, &it->it_time.it_interval)) {
+		timespecadd(&it->it_time.it_value, &it->it_time.it_interval,
+		    &next);
 		it->it_time.it_value = next;

So none of the logic to count overruns ever triggers any more.
>How-To-Repeat:
1. set a periodic interval timer (setitimer, timer_settime, timerfd_settime)
2. wind the clock forward by a lot (settimeofday, clock_settime, ntp_adjtime, ...)

The following program _would_ demonstrate this, except it sends softclock callouts into a loop or something so it triggers a heartbeat panic for another reason:

#include <err.h>
#include <errno.h>
#include <limits.h>
#include <signal.h>
#include <stdio.h>
#include <time.h>
#include <unistd.h>

static const char *
showtime(struct timespec t)
{
	static char buf[1024];
	struct tm tm;
	size_t n;

	gmtime_r(&t.tv_sec, &tm);
	n = strftime(buf, sizeof(buf), "%Y-%m-%dT%H:%M:%S.", &tm);
	snprintf(buf + n, sizeof(buf) - n, "%09d", t.tv_nsec);

	return buf;
}

int
main(void)
{
	struct timespec mono0, real0, hack, mono1, delta, real1;
	sigset_t sigs, mask;
	siginfo_t siginfo;
	timer_t t;

	/*
	 * Block signals so we can use sigtimedwait(2) to wait for the
	 * first wakeup.
	 */
	if (sigemptyset(&sigs) == -1)
		err(1, "sigemptyset");
	if (sigaddset(&sigs, SIGALRM) == -1)
		err(1, "sigaddset");
	if (sigprocmask(SIG_BLOCK, &sigs, &mask) == -1)
		err(1, "sigprocmask");

	/*
	 * Create a periodic interval timer on the real-time clock
	 * starting at the next tick and repeating every second after
	 * that.
	 */
	const struct itimerspec it = {
		.it_value = {0, 1},
		.it_interval = {1, 0},
	};
	if (timer_create(CLOCK_REALTIME, NULL, &t) == -1)
		err(1, "timer_create");
	if (timer_settime(t, 0, &it, NULL) == -1)
		err(1, "timer_settime");

	/*
	 * Save the monotonic clock so, after we mess with the
	 * real-time clock, we can find how long we spent in this
	 * program to restore the real-time clock.
	 */
	if (clock_gettime(CLOCK_MONOTONIC, &mono0) == -1)
		err(1, "clock_gettime(CLOCK_MONOTONIC)");
	printf("mono0 %llu.%09d\n",
	    (unsigned long long)mono0.tv_sec, (int)mono0.tv_nsec);

	/*
	 * Advance the real-time clock by INT_MAX + 1 seconds.  This
	 * should cause the timer overrun counter to overflow.
	 */
	if (clock_gettime(CLOCK_REALTIME, &real0) == -1)
		err(1, "clock_gettime(CLOCK_REALTIME)");
	printf("real0 %s\n", showtime(real0));
	hack = real0;
	hack.tv_sec += 1000;
	printf("hack: %s\n", showtime(hack));
	if (clock_settime(CLOCK_REALTIME, &hack) == -1)
		err(1, "clock_settime(CLOCK_REALTIME)");

	/*
	 * Wait up to two seconds for the timer to fire after an
	 * interval.  At this point it should have detected some
	 * overruns because we wound the clock forward.
	 *
	 * If anyting goes wrong, try to restore the real-time clock
	 * before reporting sigtimedwait(2) error.
	 */
	if (sigtimedwait(&sigs, &siginfo, &(const struct timespec){2, 0})
	    == -1) {
		int errno_save = errno;
		if (clock_gettime(CLOCK_MONOTONIC, &mono1) == -1)
			err(1, "clock_gettime");
		timespecsub(&mono1, &mono0, &delta);
		timespecadd(&real0, &delta, &real1);
		if (clock_settime(CLOCK_REALTIME, &real1) == -1)
			err(1, "clock_settime(CLOCK_REALTIME)");
		printf("mono1 %llu.%09d\n",
		    (unsigned long long)mono1.tv_sec, (int)mono1.tv_nsec);
		printf("delta %llu.%09d\n",
		    (unsigned long long)delta.tv_sec, (int)delta.tv_nsec);
		printf("real1 %s\n", showtime(real1));
		errno = errno_save;
		err(1, "sigtimedwait");
	}

	/*
	 * Restore the real-time clock by adding the time spent in this
	 * program so far (mono1 - mono0) to the earlier reading of the
	 * real-time clock (real0).
	 */
	if (clock_gettime(CLOCK_MONOTONIC, &mono1) == -1)
		err(1, "clock_gettime");
	timespecsub(&mono1, &mono0, &delta);
	timespecadd(&real0, &delta, &real1);
	if (clock_settime(CLOCK_REALTIME, &real1) == -1)
		err(1, "clock_settime(CLOCK_REALTIME)");
	printf("mono1 %llu.%09d\n",
	    (unsigned long long)mono1.tv_sec, (int)mono1.tv_nsec);
	printf("delta %llu.%09d\n",
	    (unsigned long long)delta.tv_sec, (int)delta.tv_nsec);
	printf("real1 %s\n", showtime(real1));

	/*
	 * Print the overrun count.  This should saturate at INT_MAX,
	 * and should never go negative.
	 */
	printf("overrun %d\n", timer_getoverrun(t));

	fflush(stdout);
	return ferror(stdout);
}

Expected output: `overrun 32' or `overrun 1000' or something

Actual output: `overrun 0'.
>Fix:
Fix the logic in itimer_callout.

But first, need automatic tests for this arithmetic!

Home	PR Database Search