Re: [PATCH v2 1/2] nfsd: reject out-of-range nseconds in setattr atime/mtime

On Sun, Jun 14, 2026, at 9:14 PM, robbieko wrote:
> From: Robbie Ko <robbieko-UelDjCVBxVpWk0Htik3J/[email protected]>
>
> A client can send a SETATTR (and, for NFSv3, a file creation) carrying
> an atime or mtime whose nseconds field is out of range. The value is
> well-formed on the wire and decodes cleanly into a valid uint32, but it
> is not a valid timespec64: tv_nsec must be less than NSEC_PER_SEC.
>
> Nothing in the path clamps it. notify_change() runs the time through
> timestamp_truncate(), which does not reduce tv_nsec below NSEC_PER_SEC
> when the filesystem supports nanosecond granularity (s_time_gran == 1),
> and the inode atime/mtime setters store it verbatim (only ctime is
> normalized, via inode_set_ctime_to_ts()).
>
> The un-normalized value then corrupts on-disk metadata. ext4's
> ext4_encode_extra_time() shifts tv_nsec left by EXT4_EPOCH_BITS; an
> out-of-range value overflows the 32-bit extra field and clobbers the
> seconds-epoch bits, so the stored seconds (and thus the year) are wrong
> on read-back. XFS with bigtime mis-stores the timestamp for the same
> reason. There is no WARN_ON anywhere in the path to catch it.
>
> Validate the client-supplied atime/mtime in nfsd_setattr(), which is
> the common choke point for SETATTR and for the create paths (via
> nfsd_create_setattr()), and covers both NFSv2 and NFSv3. The check is
> done up front, before any resources are acquired, so no cleanup path is
> involved; RFC 1813 Section 2.6 leaves error precedence to the
> implementation. Return NFS3ERR_INVAL, which RFC 1813 lists for SETATTR
> and describes as the error for a value the server 'can not store ... in
> its own representation'. The client maps this to EINVAL.
>
> tv_nsec is a long, so the comparison casts it to unsigned long (the same
> width) rather than to u32, matching timespec64_valid(). A u32 cast would
> be wrong on both ends: on 32-bit it cannot widen, and on 64-bit it would
> truncate the NFSv2 value (svcxdr_decode_sattr() computes tv_nsec as
> tmp2 * NSEC_PER_USEC, which can exceed the u32 range). The unsigned long
> cast also rejects a value that became negative when an out-of-range u32
> wire nseconds was assigned to a 32-bit long.
>
> RFC 1094 does not define NFSERR_INVAL for NFSv2; its stat enum has no
> value 22. Map nfserr_inval to nfserr_io in nfsd_map_status() so the
> NFSv2 reply stays within the RFC 1094 status set, the same way that
> function already folds other internal statuses with no NFSv2 equivalent.
> NFSv3 (and NFSv4) leave the INVAL status as is, since it is valid there.
>
> Only client-supplied times are checked: SET_TO_SERVER_TIME requests
> carry no client value and are filled in by the server. The NFSv2 Sun
> 'set both to now' convention clears ATTR_[AM]TIME_SET in the SETATTR
> proc before nfsd_setattr() runs, so it is unaffected. The sattrguard3
> ctime is deliberately left alone: an out-of-range guard simply never
> matches the object's ctime and yields NFS3ERR_NOT_SYNC via the existing
> guardtime comparison, which is the protocol-correct outcome rather than
> rejecting the request.
>
> NFSv4 already rejects such values in nfsd4_decode_nfstime4(), so they do
> not reach this check on that path.
>
> The lack of validation is long-standing and predates the git history of
> this code, so no Fixes: tag is provided. This is a data-integrity fix
> and is a candidate for LTS backport.
>
> Signed-off-by: Robbie Ko <robbieko-UelDjCVBxVpWk0Htik3J/[email protected]>
> ---
>  fs/nfsd/nfsproc.c |  1 +
>  fs/nfsd/vfs.c     | 14 ++++++++++++++
>  2 files changed, 15 insertions(+)
>
> diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c
> index 8873033d1e82..3c8da3f1af6c 100644
> --- a/fs/nfsd/nfsproc.c
> +++ b/fs/nfsd/nfsproc.c
> @@ -33,6 +33,7 @@ static __be32 nfsd_map_status(__be32 status)
>  		break;
>  	case nfserr_symlink:
>  	case nfserr_wrong_type:
> +	case nfserr_inval:
>  		status = nfserr_io;
>  		break;
>  	}
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index eafdf7b7890f..dd0bbf7aad1b 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -515,6 +515,20 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
> 
>  	trace_nfsd_vfs_setattr(rqstp, fhp, iap, guardtime);
> 
> +	/*
> +	 * Reject a client-supplied atime or mtime whose tv_nsec is out of
> +	 * range. Such a value is well-formed on the wire but is not a valid
> +	 * timespec64; storing it verbatim can corrupt on-disk timestamps
> +	 * (for example, ext4 packs tv_nsec << 2 alongside epoch bits).
> +	 * Reject it before acquiring any resources. RFC 1813 Section 2.6
> +	 * leaves error precedence to the implementation.
> +	 */
> +	if (((iap->ia_valid & ATTR_ATIME_SET) &&
> +	     (unsigned long)iap->ia_atime.tv_nsec >= NSEC_PER_SEC) ||
> +	    ((iap->ia_valid & ATTR_MTIME_SET) &&
> +	     (unsigned long)iap->ia_mtime.tv_nsec >= NSEC_PER_SEC))
> +		return nfserr_inval;
> +
>  	if (iap->ia_valid & ATTR_SIZE) {
>  		accmode |= NFSD_MAY_WRITE|NFSD_MAY_OWNER_OVERRIDE;
>  		ftype = S_IFREG;

nfsd3_proc_create() and nfsd_create_locked() call
vfs_create()/vfs_mkdir()/vfs_mknod() before nfsd_create_setattr()
reaches this new nfserr_inval return. The server then reports failure
for the invalid timestamp but leaves the new object behind, so
retries can hit EXIST and clients observe a failed non-idempotent
operation that actually modified the namespace.

But this is not strictly new behavior. "post-create setattr fails,
object left behind" is a long-standing property of this path. Any
failure in nfsd_create_setattr() (commit_metadata, fh_update,
uid/gid) already orphans the object. However, this patch adds one
new, client-triggerable way to hit it. So it's a fresh trigger of
an existing wart, not a brand-new class of bug.

The trade is corruption vs. orphan. Pre-patch, that same input
created the object and stored a corrupt timestamp (the bug the
patch targets). Post-patch it creates the object and reports
failure. Neither is good; the orphan is the lesser evil but it
is still avoidable.


Moreover, on 32-bit platforms, svcxdr_decode_sattr() computes
tmp2 * NSEC_PER_USEC before this check, and that multiplication
is 32-bit unsigned long on ILP32. A wire useconds value such as
4294968 wraps to tv_nsec == 704, so this >= NSEC_PER_SEC test
accepts an out-of-range client value and stores a bogus timestamp
instead of rejecting it.


The sashiko.dev review is based on a branch that does not have
Jeff's fix for CB_GETATTR for this exact issue, so it's
effectively a false positive.


In summary: I don't agree with adding the check in nfsd_setattr().
The incoming values need to be checked closer to the protocol-
specific code because each NFS version has to solve a slightly
different problem.

For NFSv2, the incoming value needs a decode-time guard on the
raw microseconds, because its decode does a unit conversion that
silently corrupts an out-of-range value on 32-bit before any proc
function can see it.

For NFSv3, the check should happen in nfsd3_proc_setattr(), and
in nfsd3_proc_create() before creation is attempted.

NFSv4 (SETATTR, CREATE, and CB_GETATTR) all seem to be covered by
the current implementation and don't need the extra check in
nfsd_setattr().


-- 
Chuck Lever