From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f43.google.com (mail-wr1-f43.google.com [209.85.221.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 9182E370305 for ; Tue, 18 Nov 2025 21:17:22 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.221.43 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1763500644; cv=none; b=PyvgPt3/AQ0KkzXdMOIOwI2tJNSkCSkG0nk5elIxOeKGSa1x14XUY8AoeArKhdnHoixlfPjxndxsYMnsis2gP4eJoq5FZAJqMV2Fee8Gosqlsouoex3mD8FA9noad/tSeNoHZaKijge4HS++QRXTbMRmXIUQdcOAhT003V0lMxk= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1763500644; c=relaxed/simple; bh=CHGNxnP2dRC5gRiz5jo1RtLaXI0EgEg0QfSjGRkegwE=; h=Date:From:To:Cc:Subject:Message-ID:In-Reply-To:References: MIME-Version:Content-Type; b=h5khm/1UFCL9oao8YGQqZ5obUIqAucBsUwKL6elwrKpOt3DpIYoah47wzvvxSds7DNnG9ckkYC+an+CZyTcHZWKMQZ6++vK62t7+PLzyzujFGuWWW3tgxO6gq3O0gY3bH3IpxkFNC/7IghlDmJwzrVA6sBAymSPqfCwPxm9fCvg= 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=cVeopj1Y; arc=none smtp.client-ip=209.85.221.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="cVeopj1Y" Received: by mail-wr1-f43.google.com with SMTP id ffacd0b85a97d-429c82bf86bso3516341f8f.1 for ; Tue, 18 Nov 2025 13:17:22 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1763500641; x=1764105441; 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=A/1B0ohMDe9gTMm64AEJT/zRWlaJlSs1dMj20p5G84w=; b=cVeopj1YzNnFvWAsNpTaT3uikGeDyP4ioUsYZ5sb1BbRsT3xDf8Mu3H1RUUYZz1rNT gmHVwhHFSCZ0R2J9dY2Pu7SduxD7hLCOO/4izzl+Wz7qs8Lw3MtZNLCuwkF4VtzlvqVW 4UMe7G3avmj+tUocvwldcTXo/gWdQNeQBq5w7InYjJ6yTG6L9aatgOrm+2oLVAbCBTVN gWzx2i+S29kiQY4OCQFB/HRMQrb3cGHcwcxtYhFKuJdXu5V5PsdZj0Ng4xHzP8RhFad4 itzImBYMfmJS4W1BdaXtiWd8rj75GczhqfmI4eggsNQ2vkVAYhMRcVoYIuF5ke7NNesS wxLg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1763500641; x=1764105441; 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=A/1B0ohMDe9gTMm64AEJT/zRWlaJlSs1dMj20p5G84w=; b=DyZMo2Oi42FKvd9AjxJzYn2g/mHoSuBY66pIsj7VpVzCrZT97J3I1rwNumw8x4uR6U uWbFYSneRt/sJgsa4Fo4N47vJpsG6MWGNZz77F6IzX+zi2+Q/nnK/ruZEY2KX2aogNwd fibjCwT2i0n8rZ0HIgwyy7TCT8pw+/6l2r7PrDtVP/UPiM3ph0EbIkMaugL5JBJbwMJm C1sezmAD6z7u5zgW0rPpSOAUQkoCCuG82CCOju5cFOEVGPK/A7wzRLajfjn+YDs1mF1l Estbqk+MDBm1lzuUApqAoCV/cf/sFrCHmDC6z+ymHgfCqXfLIquPIURKnjumgysEtQKt Z0hA== X-Forwarded-Encrypted: i=1; AJvYcCVOtmQpC7p1i4R36Ph38GZpHYWPAN+OwAaVQzpQSrCMURZboATLGkH7uHU1O3bwgXaZRcsxKnk3IxnLsJc=@vger.kernel.org X-Gm-Message-State: AOJu0YwYqbTDwsjU94+YJS9xyXUGczjiC6HkeYmPjfqKXyF/uznmxLNW 1LefqXKXrwuaUxmwdGG675kJkXY9oQ7qXzskAP0R4w7BQ/gZ0ht92QVn X-Gm-Gg: ASbGnctnfCemrh03tDUGI+MTSC/1penM88tjDTJtMJGNloNwpmMKjggGElBZ4zm5kZ3 2iHdHGRnPMzZCp4cKD4m7XbcEn0L++3ab+4pOKdp865B1ri4gn78lylhvylmf+6pN0UBDY8cJYe 9aQ6jeJJ6SSlnBc1eiun1H0DoGM2FdfgkbX2mBu3q/IaoYM0z57mPdABnN5KIZBcXr+7+HWWuDb QEAjOEeaZWMlPFYXxVIfGub79Dn/MJkdDqcUfG2KLmVy3HkEYIsw5nhfR/MrhhRQINyyL+b0ypK 2KWlfy9prl2uWjiBhe/+tLPqOqiys3MDaEA1j2Xq2MyoPSj9a/LdNQCXL6dp6DjOYHUKR5l4OtS LuhYvlu+Ep68khAZMFOLrZIdU/2oF8cHntuZxbVkEIBQk4c10/ydV7hTJlSPMrVTiZSrcRhtMvO dQ2Lf62KzmD9Ols05T8Dcow6gGYCBT4EGfIZkqUIsKLAL7iI8mbp0ZNIRpAoPMKgM= X-Google-Smtp-Source: AGHT+IEy8hQJw+8xUd0JEomG1xKZACwDL8RYHGd+36FCWbFJAajR9Gdiv+7updTaEhFfK0Jumlsdtw== X-Received: by 2002:a05:6000:2f81:b0:42b:2c53:3aba with SMTP id ffacd0b85a97d-42b59339417mr16166770f8f.10.1763500640615; Tue, 18 Nov 2025 13:17:20 -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-42b53e7aea7sm33855555f8f.1.2025.11.18.13.17.20 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 18 Nov 2025 13:17:20 -0800 (PST) Date: Tue, 18 Nov 2025 21:17:19 +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: <20251118211719.14879eaf@pumpkin> In-Reply-To: References: <20251113224113.4f752ccc@pumpkin> 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 Tue, 18 Nov 2025 22:51:09 +0300 Sergey Shtylyov 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 > >> 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,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