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

On Tue, Jun 16, 2026, at 3:18 PM, Jeff Layton wrote:
> 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.

The applied version of this patch defines such a symbolic constant.
Robbie, no need to resend.


-- 
Chuck Lever