Re: [PATCH v3 1/3] nfsd: reject out-of-range useconds in NFSv2 SETATTR/CREATE

On Tue, 2026-06-16 at 13:39 +0800, robbieko wrote:
> From: Robbie Ko <robbieko-UelDjCVBxVpWk0Htik3J/[email protected]>
> 
> The NFSv2 sattr decoder converts the wire useconds to nanoseconds in
> svcxdr_decode_sattr():
> 
> 	iap->ia_atime.tv_nsec = tmp2 * NSEC_PER_USEC;
> 
> tmp2 is a u32 and NSEC_PER_USEC is 1000, so the product is computed in
> unsigned long. On ILP32 that is 32 bits, and an out-of-range useconds
> value such as 4294968 wraps to tv_nsec == 704. The corruption therefore
> happens during decode, before any proc function can inspect the value,
> and a later range check on tv_nsec would see an in-range result and
> accept it.
> 
> Guard the raw useconds before the multiplication and reject values
> greater than 1000000. useconds == 1000000 is kept: it is the Sun
> convention for "set to the current server time", and the in-tree Linux
> NFSv2 client emits it in both the atime and the mtime field for a plain
> touch / utimes(file, NULL) (see encode_sattr() and
> xdr_encode_current_server_time() in fs/nfs/nfs2xdr.c). Rejecting 1000000
> would turn that common operation into a hard decode failure for both
> SETATTR and CREATE. 1000000 * NSEC_PER_USEC is 10^9, which does not wrap
> on ILP32, so the Sun convention value passes through safely; only
> genuinely out-of-range values (> 1000000) are rejected. The atime and
> mtime guards are therefore symmetric.
> 
> The decoder only applied the Sun convention in the mtime block, which
> clears ATTR_ATIME_SET|ATTR_MTIME_SET when mtime useconds == 1000000. If a
> client puts 1000000 in the atime field but not in the mtime field, the
> atime block stored an out-of-range tv_nsec (10^9) and left ATTR_ATIME_SET
> set, so the bogus value reached the filesystem. Apply the convention in
> the atime block as well, clearing ATTR_ATIME_SET so the server uses its
> current time and ignores the value. Only ATTR_ATIME_SET is cleared there;
> the mtime block keeps its existing behavior, where 1000000 means "set
> both atime and mtime to now".
> 
> Signed-off-by: Robbie Ko <robbieko-UelDjCVBxVpWk0Htik3J/[email protected]>
> ---
>  fs/nfsd/nfsxdr.c | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/fs/nfsd/nfsxdr.c b/fs/nfsd/nfsxdr.c
> index ae71e0621317..48a4e89a5f41 100644
> --- a/fs/nfsd/nfsxdr.c
> +++ b/fs/nfsd/nfsxdr.c
> @@ -172,14 +172,45 @@ svcxdr_decode_sattr(struct svc_rqst *rqstp, struct xdr_stream *xdr,
>  	tmp1 = be32_to_cpup(p++);
>  	tmp2 = be32_to_cpup(p++);
>  	if (tmp1 != (u32)-1 && tmp2 != (u32)-1) {
> +		/*
> +		 * Guard the raw useconds before the unit conversion below.
> +		 * tmp2 * NSEC_PER_USEC is computed in unsigned long, which is
> +		 * 32 bits on ILP32, so an out-of-range value would wrap and
> +		 * silently produce a bogus in-range tv_nsec. useconds ==
> +		 * 1000000 is the Sun "set to current server time" convention
> +		 * (see the mtime block below); allow it and reject anything
> +		 * larger. Note 1000000 * NSEC_PER_USEC is 10^9, which does not
> +		 * wrap on ILP32.
> +		 */
> +		if (tmp2 > 1000000)
> +			return false;

The logic looks fine, but rather than having these verbose comments
above each use of 1000000, it'd be better to declare a constant and
document its use once above that.

Something like this (and consolidate the comments above that):

#define		SUN_V2_SET_TO_NOW	1000000

Though I think the current fashion is to use enums for this so they
show up in the debugger.

>  		iap->ia_valid |= ATTR_ATIME | ATTR_ATIME_SET;
>  		iap->ia_atime.tv_sec = tmp1;
>  		iap->ia_atime.tv_nsec = tmp2 * NSEC_PER_USEC;
> +		/*
> +		 * The Linux NFSv2 client emits useconds == 1000000 in the
> +		 * atime field too (touch / utimes(file, NULL) sets ATTR_ATIME
> +		 * without ATTR_ATIME_SET). Apply the Sun convention here so
> +		 * the server uses its current time and ignores the bogus
> +		 * tv_nsec, instead of storing an out-of-range value when the
> +		 * mtime field does not also carry 1000000. Only ATTR_ATIME_SET
> +		 * is cleared; the mtime block keeps its own handling, where
> +		 * 1000000 means "set both atime and mtime to now".
> +		 */
> +		if (tmp2 == 1000000)
> +			iap->ia_valid &= ~ATTR_ATIME_SET;
>  	}
>  
>  	tmp1 = be32_to_cpup(p++);
>  	tmp2 = be32_to_cpup(p++);
>  	if (tmp1 != (u32)-1 && tmp2 != (u32)-1) {
> +		/*
> +		 * useconds == 1000000 is a valid Sun convention here (see
> +		 * below); anything above that is out of range. Guard it before
> +		 * the unit conversion to avoid the ILP32 wrap described above.
> +		 */
> +		if (tmp2 > 1000000)
> +			return false;
>  		iap->ia_valid |= ATTR_MTIME | ATTR_MTIME_SET;
>  		iap->ia_mtime.tv_sec = tmp1;
>  		iap->ia_mtime.tv_nsec = tmp2 * NSEC_PER_USEC;