From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f43.google.com (mail-wm1-f43.google.com [209.85.128.43]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 1EF752E7624 for ; Thu, 13 Nov 2025 22:41:18 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.128.43 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1763073680; cv=none; b=j5+bt7xZKOI2Gdan9hmPLstnrfLwCIHQ/SEEXE4qzNvC1X26AkR50osYMZT/EYwI8J1SbK5m+9NIQTVP2J4LOdPEmONpLp1X3mx8+8qBGzvriLeBSzMTSHMUuwJpYXgAlqTd+6qIGbm4c/ALLWRN1K9eNOr+8DfnzEhv+kRLJH4= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1763073680; c=relaxed/simple; bh=wuhUrBkpYHNqOmOsc3jAamBz7TmQEaj2DohsfZdyVio=; h=Date:From:To:Cc:Subject:Message-ID:In-Reply-To:References: MIME-Version:Content-Type; b=bTT9M50fciSReiZp8P0fMPBZWbZWQhS3ydPsUVdeySS6ZYJ93Mra6n4fnOZJTKQJGNRTjwtJYtWuEsapYUoQi2+J78eUXTOOWej2wBfrAXD+qIueai/XipbeeRD9hqZwwLDs49DeDygaxEepar9iS3hx9XdOxo3laaM+/usYG/0= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=LRo45SHI; arc=none smtp.client-ip=209.85.128.43 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="LRo45SHI" Received: by mail-wm1-f43.google.com with SMTP id 5b1f17b1804b1-47755de027eso10010055e9.0 for ; Thu, 13 Nov 2025 14:41:18 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1763073677; x=1763678477; darn=vger.kernel.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:subject:cc:to:from:date:from:to:cc:subject:date :message-id:reply-to; bh=i1g5Jnwv1NM4epHCfRkfVMg4AiQoQf6OvEpUjAmOBrU=; b=LRo45SHI+mQZ0M1fxMQjVNYawddFcXKmHCdv6Rup1Zm57qi8TYysywZR2yrH5aw+XU boeK5b0zAAfaXo1Az2+IaMmEfJzD+QVc2uHHhpB9jWaqRSqJ8t2C6KTumwfZ+PaURbsD lJ3pfDm4tMKZImho8Kb5+6MRU7wp14ChLTEU8+7IxN9o0ZumG4wRY7ahg7EA0Oz0yvBt PD/bJD5yzhpM8SbBjtedOb8VEjM1/HLFp18tnCd11VvYH5tP/zP7duGzHloOhYcu4fF2 ZNxAoQTomt0+qoHr4MW9whFbIIyj8MZ1y/lB2dqA3IuyiGpUHSHJpiv55QrSD32XpNnb LJdw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1763073677; x=1763678477; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:subject:cc:to:from:date:x-gm-gg:x-gm-message-state:from :to:cc:subject:date:message-id:reply-to; bh=i1g5Jnwv1NM4epHCfRkfVMg4AiQoQf6OvEpUjAmOBrU=; b=gX6bVx2/TGRGTr5MUGn5S/pFcQWF3wG5CBPvtePpa+HhTGf3UUrYT0QtjgkRmECZGx qZDQXY6pmyWYZFX2PL69uCzmZPsDnzU6S+uOOLngMtGOQ3VFPgoSzTvoP2TyBmduWxvq D6Cs9IsZxCde07OKYH8F3fwTkEqoI31fAD43hqfqqOO8JD1D3TOss36mqINc5sLpVqNy OAasx9c9d7EEAiVzLtgIaf/ut+n4J3K68ZFMbHwRs43TqR9g3RADfmRmYBQBJ2u1cwti vh7wx0JKoc7/TH/B5/qBqslmBe1kejiCmcdovUc4m+HmaQKOTVeDVpbPKA/wsM6aWV2W XhuA== X-Forwarded-Encrypted: i=1; AJvYcCVkVCFNTLoKlRUCzsww9yFADAr5wafpvkbtsHebK5/g1I9Qd2cyRXp2K0UWKxJEEuEA2MdfFm29SDz0bWc=@vger.kernel.org X-Gm-Message-State: AOJu0YxavSpzrabU6NlHEq3PCCfDH927Mo+UCn5oMO/45j+pd0K2a1M4 3l8wukspxFWQhNjlubC60nPHjzFje73oXZEKGfPT1w1VQMC2FRg87KFZ X-Gm-Gg: ASbGnctO6uMnvj+Gm1+pRiH/ubfuU3Bmhzy7/YBO58CIJiS9o8AEV1wLQBDGfhj4VVJ oF0iWIAoBGDKIE9NV2OA9rSbijEZxIpQv5QGwcS9rZ0kzrfUWIyTTUTWFiEqYStLEYkGq02jXdV znCATetQJg5hkL/yc4yUjHO3C6azgrMdmN2K0E2mfdOXeyB1+Cck5O7dqWQppFFTygn6D20nDXu GkcdbVLjoeog3z3ZuJDUWV9jGK4gVB44S/ibp5i3yPf3MkndGbX19vBqt8gbTztYD0Dtqrz5tJU 7aiGIr6FEN1CWYTTtpvZ3LK5ubNnNXjfF9DwB3LeEYp4aEALvRCUCWBH5LZkynbAj7rTuE51QAs hF9C1GQwrPzolR01bHvWW812RGRXRZRXggCvANkWRbnXKGeIyy8B0ZEDjwf4JRlC9hTjmYesyna oOYzaKczQmD86r4Bgqw+VEmpE6xz4ycoUXQ5JvjJ1NUi+giW0JJjKIItFjhjS5O/M= X-Google-Smtp-Source: AGHT+IH2BFZsetaKWD12tbOxB3F3a9438JU7fo2sbGwmxQg+1hfuR4AZeJjpUN53s3HuHom2sH7TWw== X-Received: by 2002:a5d:5d0b:0:b0:42b:4267:83e9 with SMTP id ffacd0b85a97d-42b59342cb8mr797340f8f.2.1763073677140; Thu, 13 Nov 2025 14:41:17 -0800 (PST) Received: from pumpkin (82-69-66-36.dsl.in-addr.zen.co.uk. [82.69.66.36]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-42b53f0b622sm6129433f8f.29.2025.11.13.14.41.16 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 13 Nov 2025 14:41:16 -0800 (PST) Date: Thu, 13 Nov 2025 22:41:13 +0000 From: David Laight To: Sergey Shtylyov Cc: , Trond Myklebust , Anna Schumaker , Subject: Re: [PATCH v3] NFSv4: prevent integer overflow while calling nfs4_set_lease_period() Message-ID: <20251113224113.4f752ccc@pumpkin> In-Reply-To: References: X-Mailer: Claws Mail 4.1.1 (GTK 3.24.38; arm-unknown-linux-gnueabihf) Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit On Thu, 13 Nov 2025 23:37:03 +0300 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 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 > Cc: stable@vger.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); > } > >