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

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

On 11/14/25 1:41 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...

> 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*...

> 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...

> 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?)...

>> +
>>  	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? :-)

[...]

MBR, Sergey