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

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

On 9/18/24 11:52 PM, Sergey Shtylyov 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)
[...]
>>> +	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...

   Alternatively, we could set the lease variable on overflow to MAX_JIFFY_OFFSET
which seems to be a max timeout in jiffies...

[...]

MBR, Sergey