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

[Trond Myklebust]

On Fri, 2025-07-25 at 23:19 +0300, Sergey Shtylyov wrote:
> On 7/2/25 7:47 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 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
> > > capping result
> > > at ULONG_MAX on actual overflow...
> > > 
> > > Found by Linux Verification Center (linuxtesting.org) with the
> > > Svace static
> > > analysis tool.
> > > 
> > > Signed-off-by: Sergey Shtylyov <s.shtylyov@xxxxxx>
> > > Cc: stable@xxxxxxxxxxxxxxx
> > > 
> > > ---
> > > The patch is against the master branch of Trond Myklebust's
> > > linux-nfs.git repo.
> > > 
> > > Changes in version 2:
> > > - made use of check_mul_overflow() instead of mul_u32_u32();
> > > - capped the multiplication result at ULONG_MAX instead of
> > > returning -ERANGE,
> > >   keeping nfs4_set_lease_period() *void*;
> > > - rewrote the patch description accordingly.
> > 
> >    Forgot to say that I had to adjust the patch description to make
> > it clear
> > that the overflow happens on 64-bit arches as well...
> 
>    Gentle ping!
>    Anna, do you agree with this approach?
> 
> > [...]
> 
> MBR, Sergey
> 

NACK. If you're going to bound check the lease time, then you can at
least make it a sensible value, not a random number chosen by some
peddler of silicon.

48 days (a.k.a. 2^32/HZ_1000 seconds) is not a sensible value for a
NFSv4 lease, and 571 megayears (a.k.a 2^64 / HZ_1000 seconds) even less
so.

Just bound it at 1 hour. If some use case turns for making the value
larger than that, then we can consider making the limit configurable.
Even 1 hour is a long time to wait for a file lock or delegation to
expire when the client that held it dies.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trondmy@xxxxxxxxxx, trond.myklebust@xxxxxxxxxxxxxxx