NetBSD Problem Report #58636

From www@netbsd.org  Sat Aug 24 20:59:28 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 619EF1A923F
	for <gnats-bugs@gnats.NetBSD.org>; Sat, 24 Aug 2024 20:59:28 +0000 (UTC)
Message-Id: <20240824205926.C8F021A9241@mollari.NetBSD.org>
Date: Sat, 24 Aug 2024 20:59:26 +0000 (UTC)
From: campbell+netbsd@mumble.net
Reply-To: campbell+netbsd@mumble.net
To: gnats-bugs@NetBSD.org
Subject: npf(7): unnecessary time precision
X-Send-Pr-Version: www-1.0

>Number:         58636
>Category:       kern
>Synopsis:       npf(7): unnecessary time precision
>Confidential:   no
>Severity:       non-critical
>Priority:       low
>Responsible:    kern-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Sat Aug 24 21:00:00 +0000 2024
>Originator:     Taylor R Campbell
>Release:        current, 10, 9, ...
>Organization:
The NpfBSD32 Foundation
>Environment:
>Description:
npf uses getnanouptime/.tv_sec to record a 32-bit timestamp:

    274 static inline void
    275 conn_update_atime(npf_conn_t *con)
    276 {
    277 	struct timespec tsnow;
    278 
    279 	getnanouptime(&tsnow);
    280 	atomic_store_relaxed(&con->c_atime, tsnow.tv_sec);
    281 }

https://nxr.netbsd.org/xref/src/sys/net/npf/npf_conn.c?r=1.35#274

It uses this to determine when connection state has expired:

    709 /*
    710  * npf_conn_expired: criterion to check if connection is expired.
    711  */
    712 bool
    713 npf_conn_expired(npf_t *npf, const npf_conn_t *con, uint64_t tsnow)
    714 {
    715 	const unsigned flags = atomic_load_relaxed(&con->c_flags);
    716 	const int etime = npf_state_etime(npf, &con->c_state, con->c_proto);
    717 	int elapsed;
    718 
    719 	if (__predict_false(flags & CONN_EXPIRE)) {
    720 		/* Explicitly marked to be expired. */
    721 		return true;
    722 	}
    723 
    724 	/*
    725 	 * Note: another thread may update 'atime' and it might
    726 	 * become greater than 'now'.
    727 	 */
    728 	elapsed = (int64_t)tsnow - atomic_load_relaxed(&con->c_atime);
    729 	return elapsed > etime;
    730 }

https://nxr.netbsd.org/xref/src/sys/net/npf/npf_conn.c?r=1.35#709

But the timeouts are all at most 31-bit, and these are all relative times.  So there's no need to do any 64-bit arithmetic anywhere.

This confuses coverity into thinking there is a y2k38 safety bug:

*** CID 1597779:  High impact quality  (Y2K38_SAFETY)
/sys/net/npf/npf_conn.c: 280 in conn_update_atime()
274     static inline void
275     conn_update_atime(npf_conn_t *con)
276     {
277     	struct timespec tsnow;
278     
279     	getnanouptime(&tsnow);
>>>     CID 1597779:  High impact quality  (Y2K38_SAFETY)
>>>     A "time_t" value is stored in an integer with too few bits to accommodate it.  The expression "tsnow.tv_sec" is cast to "uint32_t".
280     	atomic_store_relaxed(&con->c_atime, tsnow.tv_sec);
281     }
282     
283     /*
284      * npf_conn_check: check that:
285      *
>How-To-Repeat:
code inspection
>Fix:
1. Use time_uptime32 instead of getnanouptime/.tv_sec.
2. Use uint32_t for everything here (and maybe assert the timeout is below INT32_MAX so wraparound is definitely not an issue).
3. Maybe file a PR at https://github.com/rmind/npf too.

Home	PR Database Search