From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-lf1-f42.google.com (mail-lf1-f42.google.com [209.85.167.42]) (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 809EC341069 for ; Tue, 16 Dec 2025 11:22:22 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.167.42 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1765884145; cv=none; b=HYsaMEAm0GIXlSTHE2llXGkk1q4eYrfJHTZ6Dnb62Vse7vqMdVI4KgnRCjXaDJpl/sIs9Abt2fHpkJacwbBaF14sgLL1XhKro7ebxqnaYr0aVmgYt3k0bYYWZGfWaokNjaPA414x2PItA4+tG9/LZafuVE1Xi0V7Gqvuuj8DMZ8= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1765884145; c=relaxed/simple; bh=5FpYV2hyThaP6gxo9Uz3Gaum8K2+2MpBuxtaKA8b8Tc=; h=Date:From:To:Cc:Subject:Message-ID:In-Reply-To:References: MIME-Version:Content-Type; b=fOaxDjBcmwF2Jwq5WgHrNo1o76G8S+G6emBuYX/fUavbzIOvwms0YHMRkTL+SWWVZ2vidlzZ3rEMS9oCkY66IomkdVhGF5Xv2F15La2vjkSsAsa/0SUI6r4xEgaGy4FdR2E5LoNTCIF+D3xalqd/td/xVa26gT4oXOmL08Jp+5w= 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=GvJhtB6u; arc=none smtp.client-ip=209.85.167.42 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="GvJhtB6u" Received: by mail-lf1-f42.google.com with SMTP id 2adb3069b0e04-59911cb8c3cso300686e87.2 for ; Tue, 16 Dec 2025 03:22:22 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1765884141; x=1766488941; 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=3dC+oh+upH5G4XaoqKb2c8YIUZVE9MRCLwR8vX6V5s8=; b=GvJhtB6uo87V64IRTVzuVg0ArLxuAJNIFVRbLo9X9BgY6shQbGxUAzSyb0XWdo14mp bSeRJbUnDaXBXlWV8hms78jMIpuyXHeDdYtnkRSFL2jgOnhtaElo0fPelQhDGJx8kOu6 SAFaEgOYF4U089b124uDfpyoGbe25J2wnJGs0HTbL8RW21QU2XFSptgjNIVFUBDDx/v0 KnJ5iP9cWh24ZmHchsJ9HQ+JHlJ0VhKDKhJxC7mdAUDvi71bL6sC0BjL7G5UfoQQV1I5 ehAfvySiQiG/LvOkRX5z/ZdBjI4h1XqyU3FQVA2Awnshcl8ecP3cGvONajzQx/EWlg/K IDIw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1765884141; x=1766488941; 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=3dC+oh+upH5G4XaoqKb2c8YIUZVE9MRCLwR8vX6V5s8=; b=N5be6TLIfheeek3VnzQ3nDF2k24mUoO4gq6A6WHlf1EU9aHK2BhPeoKITXoh7KBkXN UhoDYGmF1df/htu/XKD6GHEtQX/MLjH1HPnzFAoueQzlbnoke99R08TvsBuTejbAUNRR owWNVC8QvJ8LQsciAck+0OnMFNzxGFUxdRN9ZOuwPHn+g/y8f5fh0fWzHpD2AXvxBX55 MPdTNahtIAmqF0PLxPhfKHx4icUjEHnsNZLbqHq3ZFMFy86PQojch0ru01z/UNyUCN1D mcQm9MHLvjUFd+DfKxSuUMHtjOlRG9cNWNY0sztg5CXGD9NJcSof8hdt7oAu0b0o5R6/ TJJw== X-Forwarded-Encrypted: i=1; AJvYcCXje+EL6jzdmngqYkAxKgzReKhAK3AA9S0VEEjRUnHH5xrIqMBRDauLRIkXzMGvCy6aIbDYxgyV7mX24QM=@vger.kernel.org X-Gm-Message-State: AOJu0YyIFjY4vJmJCrscFsFDxdYqBVO3yJrNlyOs3DmVJbKZTojjC7oE QyoS5ipni72QIEIINExkE2fQw/J8sURSN1aCjZOTDlzkQ0Kq4nRh/onkqzI07w== X-Gm-Gg: AY/fxX5S+bhOJr56cNzpfq7EbWD1BXV4rzGrqT7YIqT6WFjQlMYp4CCrtXH6C0IdSfo 7rjQwOe898Y/ZgKtoQpa4aGjGs/K8xtGQE6+X+Hmev3T1Iagz/CMaRZt77gx3RtzkYvguhTFfz5 wfsR8AFrpmoxZjAWGoZzoF3W3r0QJvP/fKithM7j9tJETm60sXj+syA7AIsG3NWpu00i7YYq6l9 B2MqBpl6hMi7nvdre+rngXJmVIFF4nOvSPvQ5oD5zAczzhYoK7Tio9HS5L1RVaaEZCsDNKPK5Qd 5pgL6bp1aXnFhTQcRijGDcvi8gfYP5zjzaUrA2eLDz3BL8XZT9S9/yFNhJm30/WNT0RGLCPH2R+ hoWlUI2xTwD5JThZbu7OUdDAVhrHLf52+2brSv9Q05KWRdzXOLxhiFve3diHZomqLsz3WQwyYFq 9k1FR4HDrHlpHIYGy3LQJP8it87MoT+nxgX6JKP3IztrwTtZVc+ZwD X-Google-Smtp-Source: AGHT+IEUQefTteAyd6IImKCfTuCbjbOTUUOkytdsSBdw7FpMzi30wrUOr1L+Fr4XI2IVzYyunRrh6Q== X-Received: by 2002:a05:6000:2083:b0:431:16d:63a3 with SMTP id ffacd0b85a97d-431016d695fmr5642674f8f.46.1765877291648; Tue, 16 Dec 2025 01:28:11 -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-42ff626b591sm22236958f8f.15.2025.12.16.01.28.10 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 16 Dec 2025 01:28:11 -0800 (PST) Date: Tue, 16 Dec 2025 09:28:09 +0000 From: David Laight To: Krzysztof Kozlowski Cc: Thorsten Blum , Huisong Li , Akira Shimahara , Greg Kroah-Hartman , stable@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v4] w1: therm: Fix off-by-one buffer overflow in alarms_store Message-ID: <20251216092809.2e9b153d@pumpkin> In-Reply-To: <243ec26f-1fe1-4b3c-ab24-a6ebab163cde@kernel.org> References: <20251111204422.41993-2-thorsten.blum@linux.dev> <243ec26f-1fe1-4b3c-ab24-a6ebab163cde@kernel.org> 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, 16 Dec 2025 08:11:13 +0100 Krzysztof Kozlowski wrote: > On 11/11/2025 21:44, Thorsten Blum wrote: > > The sysfs buffer passed to alarms_store() is allocated with 'size + 1' > > bytes and a NUL terminator is appended. However, the 'size' argument > > does not account for this extra byte. The original code then allocated > > 'size' bytes and used strcpy() to copy 'buf', which always writes one > > byte past the allocated buffer since strcpy() copies until the NUL > > terminator at index 'size'. > > > > Fix this by parsing the 'buf' parameter directly using simple_strtoll() > > without allocating any intermediate memory or string copying. This > > removes the overflow while simplifying the code. > > > > Cc: stable@vger.kernel.org > > Fixes: e2c94d6f5720 ("w1_therm: adding alarm sysfs entry") > > Signed-off-by: Thorsten Blum > > --- > > Compile-tested only. > > > > Changes in v4: > > - Use simple_strtoll because kstrtoint also parses long long internally > > - Return -ERANGE in addition to -EINVAL to match kstrtoint's behavior > > - Remove any changes unrelated to fixing the buffer overflow (Krzysztof) > > while maintaining the same behavior and return values as before > > - Link to v3: https://lore.kernel.org/lkml/20251030155614.447905-1-thorsten.blum@linux.dev/ > > > > Changes in v3: > > - Add integer range check for 'temp' to match kstrtoint() behavior > > - Explicitly cast 'temp' to int when calling int_to_short() > > - Link to v2: https://lore.kernel.org/lkml/20251029130045.70127-2-thorsten.blum@linux.dev/ > > > > Changes in v2: > > - Fix buffer overflow instead of truncating the copy using strscpy() > > - Parse buffer directly using simple_strtol() as suggested by David > > - Update patch subject and description > > - Link to v1: https://lore.kernel.org/lkml/20251017170047.114224-2-thorsten.blum@linux.dev/ > > --- > > drivers/w1/slaves/w1_therm.c | 64 ++++++++++++------------------------ > > 1 file changed, 21 insertions(+), 43 deletions(-) > > > > diff --git a/drivers/w1/slaves/w1_therm.c b/drivers/w1/slaves/w1_therm.c > > index 9ccedb3264fb..5707fa34e804 100644 > > --- a/drivers/w1/slaves/w1_therm.c > > +++ b/drivers/w1/slaves/w1_therm.c > > @@ -1836,55 +1836,36 @@ static ssize_t alarms_store(struct device *device, > > struct w1_slave *sl = dev_to_w1_slave(device); > > struct therm_info info; > > u8 new_config_register[3]; /* array of data to be written */ > > - int temp, ret; > > - char *token = NULL; > > + long long temp; > > + int ret = 0; > > s8 tl, th; /* 1 byte per value + temp ring order */ > > - char *p_args, *orig; > > - > > - p_args = orig = kmalloc(size, GFP_KERNEL); > > - /* Safe string copys as buf is const */ > > - if (!p_args) { > > - dev_warn(device, > > - "%s: error unable to allocate memory %d\n", > > - __func__, -ENOMEM); > > - return size; > > - } > > - strcpy(p_args, buf); > > - > > - /* Split string using space char */ > > - token = strsep(&p_args, " "); > > - > > - if (!token) { > > - dev_info(device, > > - "%s: error parsing args %d\n", __func__, -EINVAL); > > - goto free_m; > > - } > > - > > - /* Convert 1st entry to int */ > > - ret = kstrtoint (token, 10, &temp); > > + const char *p = buf; > > + char *endp; > > + > > + temp = simple_strtoll(p, &endp, 10); > > Why using this, instead of explicitly encouraged kstrtoll()? Because the code needs to look at the terminating character. The kstrtoxxx() family only support buffers that contain a single value. While they return an indication of 'overflow' they are useless for more general parameter parsing. The simple_strtoxxx() could detect overflow and then set 'endp' to the digit that make the value too big - which should give an error provided the callers checks the separator. I don't know the full history of these functions... David > > > + if (p == endp || *endp != ' ') > > + ret = -EINVAL; > > + else if (temp < INT_MIN || temp > INT_MAX) > > + ret = -ERANGE; > > if (ret) { > > dev_info(device, > > "%s: error parsing args %d\n", __func__, ret); > > - goto free_m; > > + goto err; > > So this is just return size. > > > Best regards, > Krzysztof