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: <20251113224113.4f752ccc@pumpkin>
Date: Thu, 13 Nov 2025 22:41:13 +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 Thu, 13 Nov 2025 23:37:03 +0300
Sergey Shtylyov <s.shtylyov@....ru> 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
> 
> ---
> The patch is against the master branch of Trond Myklebust's linux-nfs.git repo.
> 
> Changes in version #3:
> - changed the lease period overflow fallback to 1 hour, updated the patch
>   description accordingly.
> 
> 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.
> 
>  fs/nfs/nfs4_fs.h    |    3 +--
>  fs/nfs/nfs4proc.c   |    2 +-
>  fs/nfs/nfs4renewd.c |   10 +++++++---
>  fs/nfs/nfs4state.c  |    2 +-
>  4 files changed, 10 insertions(+), 7 deletions(-)
> 
> Index: linux-nfs/fs/nfs/nfs4_fs.h
> ===================================================================
> --- linux-nfs.orig/fs/nfs/nfs4_fs.h
> +++ linux-nfs/fs/nfs/nfs4_fs.h
> @@ -464,8 +464,7 @@ struct nfs_client *nfs4_alloc_client(con
>  extern void nfs4_schedule_state_renewal(struct nfs_client *);
>  extern void nfs4_kill_renewd(struct nfs_client *);
>  extern void nfs4_renew_state(struct work_struct *);
> -extern void nfs4_set_lease_period(struct nfs_client *clp, unsigned long lease);
> -
> +extern void nfs4_set_lease_period(struct nfs_client *clp, u32 period);
>  
>  /* nfs4state.c */
>  extern const nfs4_stateid current_stateid;
> Index: linux-nfs/fs/nfs/nfs4proc.c
> ===================================================================
> --- linux-nfs.orig/fs/nfs/nfs4proc.c
> +++ linux-nfs/fs/nfs/nfs4proc.c
> @@ -5478,7 +5478,7 @@ static int nfs4_do_fsinfo(struct nfs_ser
>  		err = _nfs4_do_fsinfo(server, fhandle, fsinfo);
>  		trace_nfs4_fsinfo(server, fhandle, fsinfo->fattr, err);
>  		if (err == 0) {
> -			nfs4_set_lease_period(server->nfs_client, fsinfo->lease_time * HZ);
> +			nfs4_set_lease_period(server->nfs_client, fsinfo->lease_time);
>  			break;
>  		}
>  		err = nfs4_handle_exception(server, err, &exception);
> 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;

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'.
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.
I've no idea what normal/sane values are (lower as well as upper).
But perhaps you want:
	/* For sanity clamp between 10 mins and 100 hours */
	lease = clamp(period, 10 * 60, 100 * 60 * 60) * HZ;

> +
>  	spin_lock(&clp->cl_lock);
>  	clp->cl_lease_time = lease;
>  	spin_unlock(&clp->cl_lock);

Do I see a lock that doesn't?

	David

> Index: linux-nfs/fs/nfs/nfs4state.c
> ===================================================================
> --- linux-nfs.orig/fs/nfs/nfs4state.c
> +++ linux-nfs/fs/nfs/nfs4state.c
> @@ -103,7 +103,7 @@ static int nfs4_setup_state_renewal(stru
>  
>  	status = nfs4_proc_get_lease_time(clp, &fsinfo);
>  	if (status == 0) {
> -		nfs4_set_lease_period(clp, fsinfo.lease_time * HZ);
> +		nfs4_set_lease_period(clp, fsinfo.lease_time);
>  		nfs4_schedule_state_renewal(clp);
>  	}
>  
>