Re: [PATCH v3] NFSv4: prevent integer overflow while calling nfs4_set_lease_period()

[Sergey Shtylyov <s.shtylyov@omp.ru>]

On 11/19/25 12:17 AM, David Laight wrote:
[...]

>>>> The nfs_client::cl_lease_time field (as well as the jiffies variable it's
>>>> used with) is declared as *unsigned long*, which is 32-bit type on 32-bit
>>>> arches and 64-bit type on 64-bit arches. When nfs4_set_lease_period() that
>>>> sets nfs_client::cl_lease_time is called, 32-bit nfs_fsinfo::lease_time
>>>> field is multiplied by HZ -- that might overflow before being implicitly
>>>> cast to *unsigned long*. Actually, there's no need to multiply by HZ at all
>>>> the call sites of nfs4_set_lease_period() -- it makes more sense to do that
>>>> once, inside that function, calling check_mul_overflow() and falling back
>>>> to 1 hour on an actual overflow...
>>>>
>>>> Found by Linux Verification Center (linuxtesting.org) with the Svace static
>>>> analysis tool.
>>>>
>>>> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
>>>> Cc: stable@vger.kernel.org  
>>
[...]
>>>> Index: linux-nfs/fs/nfs/nfs4renewd.c
>>>> ===================================================================
>>>> --- linux-nfs.orig/fs/nfs/nfs4renewd.c
>>>> +++ linux-nfs/fs/nfs/nfs4renewd.c
>>>> @@ -137,11 +137,15 @@ nfs4_kill_renewd(struct nfs_client *clp)
>>>>   * nfs4_set_lease_period - Sets the lease period on a nfs_client
>>>>   *
>>>>   * @clp: pointer to nfs_client
>>>> - * @lease: new value for lease period
>>>> + * @period: new value for lease period (in seconds)
>>>>   */
>>>> -void nfs4_set_lease_period(struct nfs_client *clp,
>>>> -		unsigned long lease)
>>>> +void nfs4_set_lease_period(struct nfs_client *clp, u32 period)
>>>>  {
>>>> +	unsigned long lease;
>>>> +
>>>> +	if (check_mul_overflow(period, HZ, &lease))
>>>> +		lease = 60UL * 60UL * HZ; /* one hour */  
>>>
>>> That isn't good enough, just a few lines higher there is:
>>> 	timeout = (2 * clp->cl_lease_time) / 3 + (long)clp->cl_last_renewal
>>> 		- (long)jiffies;  
>>    Indeed, I should have probably capped period at 3600 secs as well...

   That's one hour.

>>> So the value has to be multipliable by 2 without overflowing.
>>> I also suspect the modulo arithmetic also only works if the values
>>> are 'much smaller than long'.
>>
>>    You mean the jiffies-relative math? I think it should work with any
>> values, with either 32- or 64-bit *unsigned long*...
> 
> There might be tests of the form (signed long)(jiffies - value) > 0.
> They only work if the interval is less than half (the time) of jiffies.
> Such values are insane - but you are applying a cap that isn't strong enough.

*   3600 seconds, you mean?

>>> With HZ = 1000 and a requested period of 1000 hours the multiply (just)
>>> fits in 32 bits - but none of the code is going to work at all.
>>>
>>> It would be simpler and safer to just put a sanity upper limit on period.  
>>
>>    Yes.
>>
>>> I've no idea what normal/sane values are (lower as well as upper).  
>>
>>    The RFCs don't have any, it seems...

   Nether max nor min. It's on;y said that lease_time is a 32-bit unsigned
value...

>>> But perhaps you want:
>>> 	/* For sanity clamp between 10 mins and 100 hours */
>>> 	lease = clamp(period, 10 * 60, 100 * 60 * 60) * HZ;  
>>
>>    Trond was talking about 1-hour period... And I don't think we need the
>> lower bound (except maybe 1 second?)...
> 
> If 1 hour might be a reasonable value, then clamp to something much bigger
> that won't break the code.

   Trond said that even that seems too much for the file lock lease period...

[...]

>>>> +
>>>>  	spin_lock(&clp->cl_lock);
>>>>  	clp->cl_lease_time = lease;
>>>>  	spin_unlock(&clp->cl_lock);  
>>>
>>> Do I see a lock that doesn't?  
>>
>>    Doesn't do anything useful, you mean? :-)
> 
> You can think of a 'lock' as something that locks two or more
> operations together - often just a compare and update.
> 
> That one is (at most) a WRITE_ONCE().

   Yes, probably... :-)

[..]

MBR, Sergey