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

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

On 9/16/24 22:44, Anna Schumaker 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 *unsigned long* on 32-bit
>> arches.  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 (using mul_u32_u32(), as it produces a better code on 32-bit
>> x86 arch), also checking for an overflow there and returning -ERANGE if it
>> does happen (we're also making that function *int* instead of *void* and
>> adding the result checks to its callers)...
>>
>> 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,15 +137,22 @@ 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)
>> +int nfs4_set_lease_period(struct nfs_client *clp, u32 period)
>>  {
>> +	u64 result = mul_u32_u32(period, HZ);
> 
> Thanks for the patch! Sorry it took a couple of weeks for me to look at.

   NP, better late than never. :-)

> I do like this change for doing the calculation in a single place, rather
 than relying on callers to do it first.
> 
>> +	unsigned long lease = result;
>> +
>> +	/* First see if period * HZ actually fits into unsigned long... */
>> +	if (result > ULONG_MAX)
>> +		return -ERANGE;
> 
> However, I'm not sold that this should be an error condition. I wonder if it would be better to change the clp->cl_lease_time field to a u64 so it has the same size on 32bit and 64bit architectures?

   That would be much more invasive change. And pretty fruitless, I think, because nfs_client::cl_lease_time is only involved in the jiffies math,
and since jiffies is *unsigned long* (i.e. 32-bit on a 32-bit arch), the
results of our u64 math would get truncated to 32 bits anyway in the end...
   Looking at time_{after|before}(), I even suspected they could complain
at compile time -- however I wasn't able to trigger that so far...

> Thanks,
> Anna

[...]

MBR, Sergey