Re: [PATCH v3 2/3] nfsd: reject out-of-range nseconds in NFSv3 SETATTR and create ops

On Tue, 2026-06-16 at 13:39 +0800, robbieko wrote:
> From: Robbie Ko <robbieko-UelDjCVBxVpWk0Htik3J/[email protected]>
> 
> A client can send an NFSv3 SETATTR, CREATE, MKDIR, SYMLINK or MKNOD
> 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 setattr 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, which
> 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.
> 
> Validate the client-supplied atime/mtime in the proc handlers and return
> NFS3ERR_INVAL before anything is changed. RFC 1813 lists NFS3ERR_INVAL
> for SETATTR and describes it as the error for a value the server 'can
> not store ... in its own representation'; the client maps it to EINVAL.
> 
> Checking in the proc handlers, rather than in nfsd_setattr(), keeps the
> rejection in front of object creation. The create operations create the
> object before nfsd_create_setattr() runs, so a late failure would leave
> the new object behind and turn a non-idempotent request into a namespace
> change that reports failure. The check is therefore done up front, for
> the create operations before the object is created.
> 
> 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
> truncate on 64-bit; 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.
> 
> Only client-supplied times are checked: SET_TO_SERVER_TIME requests
> carry no client value. 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.
> 
> Signed-off-by: Robbie Ko <robbieko-UelDjCVBxVpWk0Htik3J/[email protected]>
> ---
>  fs/nfsd/nfs3proc.c | 40 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
> 
> diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c
> index 42adc5461db0..32d6b51dbe53 100644
> --- a/fs/nfsd/nfs3proc.c
> +++ b/fs/nfsd/nfs3proc.c
> @@ -29,6 +29,25 @@ static int	nfs3_ftypes[] = {
>  	S_IFIFO,		/* NF3FIFO */
>  };
>  
> +/*
> + * Reject a client-supplied atime or mtime whose nanoseconds field is out
> + * of range. Such a value is well-formed on the wire but is not a valid
> + * timespec64, and storing it verbatim can corrupt on-disk timestamps.
> + * tv_nsec is a long, so it is cast to unsigned long (the same width) to
> + * catch both an over-large value and one that became negative when an
> + * out-of-range u32 wire nseconds was assigned to a 32-bit long.
> + */
> +static bool nfsd3_time_in_range(const struct iattr *iap)
> +{
> +	if ((iap->ia_valid & ATTR_ATIME_SET) &&
> +	    (unsigned long)iap->ia_atime.tv_nsec >= NSEC_PER_SEC)
> +		return false;
> +	if ((iap->ia_valid & ATTR_MTIME_SET) &&
> +	    (unsigned long)iap->ia_mtime.tv_nsec >= NSEC_PER_SEC)
> +		return false;
> +	return true;
> +}
> +
>  static __be32 nfsd3_map_status(__be32 status)
>  {
>  	switch (status) {
> @@ -101,9 +120,14 @@ nfsd3_proc_setattr(struct svc_rqst *rqstp)
>  				SVCFH_fmt(&argp->fh));
>  
>  	fh_copy(&resp->fh, &argp->fh);
> +	if (!nfsd3_time_in_range(&argp->attrs)) {
> +		resp->status = nfserr_inval;
> +		goto out;
> +	}
>  	if (argp->check_guard)
>  		guardtime = &argp->guardtime;
>  	resp->status = nfsd_setattr(rqstp, &resp->fh, &attrs, guardtime);
> +out:
>  	resp->status = nfsd3_map_status(resp->status);
>  	return rpc_success;
>  }
> @@ -265,6 +289,8 @@ nfsd3_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  
>  	trace_nfsd_vfs_create(rqstp, fhp, S_IFREG, argp->name, argp->len);
>  
> +	if (!nfsd3_time_in_range(iap))
> +		return nfserr_inval;
>  	if (isdotent(argp->name, argp->len))
>  		return nfserr_exist;
>  	if (!(iap->ia_valid & ATTR_MODE))
> @@ -400,8 +426,13 @@ nfsd3_proc_mkdir(struct svc_rqst *rqstp)
>  	argp->attrs.ia_valid &= ~ATTR_SIZE;
>  	fh_copy(&resp->dirfh, &argp->fh);
>  	fh_init(&resp->fh, NFS3_FHSIZE);
> +	if (!nfsd3_time_in_range(&argp->attrs)) {
> +		resp->status = nfserr_inval;
> +		goto out;
> +	}
>  	resp->status = nfsd_create(rqstp, &resp->dirfh, argp->name, argp->len,
>  				   &attrs, S_IFDIR, 0, &resp->fh);
> +out:
>  	resp->status = nfsd3_map_status(resp->status);
>  	return rpc_success;
>  }
> @@ -415,6 +446,10 @@ nfsd3_proc_symlink(struct svc_rqst *rqstp)
>  		.na_iattr	= &argp->attrs,
>  	};
>  
> +	if (!nfsd3_time_in_range(&argp->attrs)) {
> +		resp->status = nfserr_inval;
> +		goto out;
> +	}
>  	if (argp->tlen == 0) {
>  		resp->status = nfserr_inval;
>  		goto out;
> @@ -471,6 +506,11 @@ nfsd3_proc_mknod(struct svc_rqst *rqstp)
>  		goto out;
>  	}
>  
> +	if (!nfsd3_time_in_range(&argp->attrs)) {
> +		resp->status = nfserr_inval;
> +		goto out;
> +	}
> +
>  	type = nfs3_ftypes[argp->ftype];
>  	resp->status = nfsd_create(rqstp, &resp->dirfh, argp->name, argp->len,
>  				   &attrs, type, rdev, &resp->fh);

Reviewed-by: Jeff Layton <[email protected]>