linux-kernel - Re: [PATCH v3] NFSv4: prevent integer overflow while calling nfs4_set_lease

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20251118211719.14879eaf@pumpkin>
Date: Tue, 18 Nov 2025 21:17:19 +0000
From: David Laight <david.laight.linux@...il.com>
To: Sergey Shtylyov <s.shtylyov@....ru>
Cc: <linux-nfs@...r.kernel.org>, Trond Myklebust <trondmy@...nel.org>, Anna
 Schumaker <anna@...nel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3] NFSv4: prevent integer overflow while calling
 nfs4_set_lease_period()

On Tue, 18 Nov 2025 22:51:09 +0300
Sergey Shtylyov <s.shtylyov@....ru> wrote:

> 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@....ru>
> >> Cc: stable@...r.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*...

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.

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

If 1 hour might be a reasonable value, then clamp to something much bigger
that won't break the code.
In the past I've used at least twice the limit from the 'standard'.
I've also had methods of setting values outside the limits (useful
for testing timers that are supposed to be at least 10 minutes.

> 
> >> +
> >>  	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().

	David

> 
> [...]
> 
> MBR, Sergey