kern/58636: npf(7): unnecessary time precision

[campbell+netbsd%mumble.net@localhost]

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