NetBSD Problem Report #57279

From tsutsui@ceres.dti.ne.jp  Tue Mar 21 14:15:16 2023
Return-Path: <tsutsui@ceres.dti.ne.jp>
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))
	(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id 7DF711A923C
	for <gnats-bugs@gnats.NetBSD.org>; Tue, 21 Mar 2023 14:15:16 +0000 (UTC)
Message-Id: <202303211415.32LEF6YF018530@ceres.dti.ne.jp>
Date: Tue, 21 Mar 2023 23:15:06 +0900 (JST)
From: Izumi Tsutsui <tsutsui@ceres.dti.ne.jp>
Reply-To: tsutsui@ceres.dti.ne.jp
To: gnats-bugs@NetBSD.org
Cc: tsutsui@ceres.dti.ne.jp
Subject: 32 bit time_t leftover in NFS code
X-Send-Pr-Version: 3.95

>Number:         57279
>Category:       kern
>Synopsis:       32 bit time_t leftover in NFS code
>Confidential:   no
>Severity:       non-critical
>Priority:       medium
>Responsible:    kern-bug-people
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Tue Mar 21 14:20:00 +0000 2023
>Closed-Date:    Sat Apr 01 20:03:16 +0000 2023
>Last-Modified:  Sat Apr 01 20:03:16 +0000 2023
>Originator:     Izumi Tsutsui
>Release:        NetBSD 9.3 and -current
>Organization:
>Environment:
System: NetBSD mirage 9.3 NetBSD 9.3 (GENERIC) #0: Thu Aug 4 15:30:37 UTC 2022 mkrepro@mkrepro.NetBSD.org:/usr/src/sys/arch/i386/compile/GENERIC i386
Architecture: all
Machine: all
>Description:
There are calculations that use int or long for time_t time_second
or tv_sec in src/sys/nfs.

>How-To-Repeat:
Code inspection.
(maybe we need some lint tools for time_t calculations?)

>Fix:
Index: sys/nfs/nfs_clntsubs.c
===================================================================
RCS file: /cvsroot/src/sys/nfs/nfs_clntsubs.c,v
retrieving revision 1.6
diff -u -p -d -r1.6 nfs_clntsubs.c
--- sys/nfs/nfs_clntsubs.c	28 Feb 2022 08:45:36 -0000	1.6
+++ sys/nfs/nfs_clntsubs.c	21 Mar 2023 14:09:19 -0000
@@ -390,7 +390,7 @@ nfs_check_wccdata(struct nfsnode *np, co
 	if (docheck) {
 		struct vnode *vp = NFSTOV(np);
 		struct nfsmount *nmp;
-		long now = time_second;
+		time_t now = time_second;
 		const struct timespec *omtime = &np->n_vattr->va_mtime;
 		const struct timespec *octime = &np->n_vattr->va_ctime;
 		const char *reason = NULL; /* XXX: gcc */
Index: sys/nfs/nfs_iod.c
===================================================================
RCS file: /cvsroot/src/sys/nfs/nfs_iod.c,v
retrieving revision 1.8
diff -u -p -d -r1.8 nfs_iod.c
--- sys/nfs/nfs_iod.c	3 Sep 2018 16:29:36 -0000	1.8
+++ sys/nfs/nfs_iod.c	21 Mar 2023 14:09:19 -0000
@@ -409,7 +409,8 @@ nfs_savenickauth(struct nfsmount *nmp, k
 	struct timeval ktvin, ktvout;
 	u_int32_t nick;
 	char *dpos = *dposp, *cp2;
-	int deltasec, error = 0;
+	time_t deltasec;
+	int error = 0;

 	memset(&ktvout, 0, sizeof ktvout);	 /* XXX gcc */

Index: sys/nfs/nfs_vfsops.c
===================================================================
RCS file: /cvsroot/src/sys/nfs/nfs_vfsops.c,v
retrieving revision 1.244
diff -u -p -d -r1.244 nfs_vfsops.c
--- sys/nfs/nfs_vfsops.c	17 Mar 2023 00:46:35 -0000	1.244
+++ sys/nfs/nfs_vfsops.c	21 Mar 2023 14:09:19 -0000
@@ -323,7 +323,7 @@ nfs_mountroot(void)
 	struct mount *mp;
 	struct vnode *vp;
 	struct lwp *l;
-	long n;
+	time_t n;
 	int error;

 	l = curlwp; /* XXX */
@@ -378,7 +378,7 @@ nfs_mountroot(void)
 		panic("nfs_mountroot: getattr for root");
 	n = attr.va_atime.tv_sec;
 #ifdef	DEBUG
-	printf("root time: 0x%lx\n", n);
+	printf("root time: 0x%" PRIx64 "\n", n);
 #endif
 	setrootfstime(n);


>Release-Note:

>Audit-Trail:
From: mlelstv@serpens.de (Michael van Elst)
To: gnats-bugs@netbsd.org
Cc: 
Subject: Re: kern/57279: 32 bit time_t leftover in NFS code
Date: Tue, 21 Mar 2023 15:02:45 -0000 (UTC)

 tsutsui@ceres.dti.ne.jp (Izumi Tsutsui) writes:

 >There are calculations that use int or long for time_t time_second
 >or tv_sec in src/sys/nfs.

 >===================================================================
 >RCS file: /cvsroot/src/sys/nfs/nfs_iod.c,v
 >retrieving revision 1.8
 >diff -u -p -d -r1.8 nfs_iod.c
 >--- sys/nfs/nfs_iod.c	3 Sep 2018 16:29:36 -0000	1.8
 >+++ sys/nfs/nfs_iod.c	21 Mar 2023 14:09:19 -0000
 >@@ -409,7 +409,8 @@ nfs_savenickauth(struct nfsmount *nmp, k
 > 	struct timeval ktvin, ktvout;
 > 	u_int32_t nick;
 > 	char *dpos = *dposp, *cp2;
 >-	int deltasec, error = 0;
 >+	time_t deltasec;
 >+	int error = 0;
 > 
 > 	memset(&ktvout, 0, sizeof ktvout);	 /* XXX gcc */


 That part doesn't look sufficient. The code is working with 32bit
 timestamps and probably needs some rewrite to operate correctly
 with 64bit timestamps.

 On the other hand, the kerberos code looks broken and was written
 for KERB4. It needs to be removed or replaced with RPCSEC_GSS code.

From: Izumi Tsutsui <tsutsui@ceres.dti.ne.jp>
To: gnats-bugs@netbsd.org
Cc: tsutsui@ceres.dti.ne.jp
Subject: Re: kern/57279: 32 bit time_t leftover in NFS code
Date: Wed, 22 Mar 2023 00:37:02 +0900

 mlelstv@ wrote:

 >  >--- sys/nfs/nfs_iod.c	3 Sep 2018 16:29:36 -0000	1.8
 >  >+++ sys/nfs/nfs_iod.c	21 Mar 2023 14:09:19 -0000
 >  >@@ -409,7 +409,8 @@ nfs_savenickauth(struct nfsmount *nmp, k
 >  > 	struct timeval ktvin, ktvout;
 >  > 	u_int32_t nick;
 >  > 	char *dpos = *dposp, *cp2;
 >  >-	int deltasec, error = 0;
 >  >+	time_t deltasec;
 >  >+	int error = 0;
 >  > 
 >  > 	memset(&ktvout, 0, sizeof ktvout);	 /* XXX gcc */
 >  
 >  
 >  That part doesn't look sufficient. The code is working with 32bit
 >  timestamps and probably needs some rewrite to operate correctly
 >  with 64bit timestamps.

 Yes, it looks more code that uses long for tv_sec in XDR RPC:

 https://github.com/NetBSD/src/blob/netbsd-9/sys/nfs/nfs_iod.c#L430-L431
 > 		ktvout.tv_sec = fxdr_unsigned(long, ktvout.tv_sec);

 https://github.com/NetBSD/src/blob/netbsd-9/sys/nfs/nfs_serv.c#L1006
 > 		*tl++ = txdr_unsigned(boottime.tv_sec);

 but at least "deltasec" should be time_t anyway.

 ---
 Izumi Tsutsui

From: "Christos Zoulas" <christos@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/57279 CVS commit: src/sys/nfs
Date: Tue, 21 Mar 2023 11:47:47 -0400

 Module Name:	src
 Committed By:	christos
 Date:		Tue Mar 21 15:47:46 UTC 2023

 Modified Files:
 	src/sys/nfs: nfs_clntsubs.c nfs_iod.c nfs_vfsops.c

 Log Message:
 PR/57279: Izumi Tsutsui: Fix some {int,long} -> time_t. Still things will
 break eventually because parts of the nfs protocol assume time_t will fit
 in 32 bits.


 To generate a diff of this commit:
 cvs rdiff -u -r1.6 -r1.7 src/sys/nfs/nfs_clntsubs.c
 cvs rdiff -u -r1.8 -r1.9 src/sys/nfs/nfs_iod.c
 cvs rdiff -u -r1.244 -r1.245 src/sys/nfs/nfs_vfsops.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: open->closed
State-Changed-By: dholland@NetBSD.org
State-Changed-When: Sat, 01 Apr 2023 20:03:16 +0000
State-Changed-Why:
Christos committed it.

I think we don't need to fix -9 since it's reasonable to assume -9
will be EOL in 2038 :-)   ... however, if there's some reason to
pull up the change anyway don't let me stop you.

thanks for finding these; 64-bit truncations are really hard to
track down and stamp out.


>Unformatted:

Home	PR Database Search