[PATCH v3] w1: therm: Fix off-by-one buffer overflow in alarms_store

[PATCH v3] w1: therm: Fix off-by-one buffer overflow in alarms_store

[Thorsten Blum]

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_strtol()
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 <thorsten.blum@linux.dev>
---
Compile-tested only.

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 | 102 ++++++++++++-----------------------
 1 file changed, 35 insertions(+), 67 deletions(-)

diff --git a/drivers/w1/slaves/w1_therm.c b/drivers/w1/slaves/w1_therm.c
index 9ccedb3264fb..1dad9fa1ec4a 100644
--- a/drivers/w1/slaves/w1_therm.c
+++ b/drivers/w1/slaves/w1_therm.c
@@ -1836,59 +1836,32 @@ 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 temp;
+	int ret;
 	s8 tl, th;	/* 1 byte per value + temp ring order */
-	char *p_args, *orig;
+	const char *p = buf;
+	char *endp;
 
-	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;
+	temp = simple_strtol(p, &endp, 10);
+	if (temp < INT_MIN || temp > INT_MAX || p == endp || *endp != ' ') {
+		dev_info(device, "%s: error parsing args %d\n",
+			 __func__, -EINVAL);
+		goto err;
 	}
-	strcpy(p_args, buf);
-
-	/* Split string using space char */
-	token = strsep(&p_args, " ");
+	/* Cast to short to eliminate out of range values */
+	tl = int_to_short((int)temp);
 
-	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);
-	if (ret) {
-		dev_info(device,
-			"%s: error parsing args %d\n", __func__, ret);
-		goto free_m;
-	}
-
-	tl = int_to_short(temp);
-
-	/* 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 2nd entry to int */
-	ret = kstrtoint (token, 10, &temp);
-	if (ret) {
-		dev_info(device,
-			"%s: error parsing args %d\n", __func__, ret);
-		goto free_m;
+	p = endp + 1;
+	temp = simple_strtol(p, &endp, 10);
+	if (temp < INT_MIN || temp > INT_MAX || p == endp) {
+		dev_info(device, "%s: error parsing args %d\n",
+			 __func__, -EINVAL);
+		goto err;
 	}
+	/* Cast to short to eliminate out of range values */
+	th = int_to_short((int)temp);
 
-	/* Prepare to cast to short by eliminating out of range values */
-	th = int_to_short(temp);
-
-	/* Reorder if required th and tl */
+	/* Reorder if required */
 	if (tl > th)
 		swap(tl, th);
 
@@ -1897,35 +1870,30 @@ static ssize_t alarms_store(struct device *device,
 	 * (th : byte 2 - tl: byte 3)
 	 */
 	ret = read_scratchpad(sl, &info);
-	if (!ret) {
-		new_config_register[0] = th;	/* Byte 2 */
-		new_config_register[1] = tl;	/* Byte 3 */
-		new_config_register[2] = info.rom[4];/* Byte 4 */
-	} else {
-		dev_info(device,
-			"%s: error reading from the slave device %d\n",
-			__func__, ret);
-		goto free_m;
+	if (ret) {
+		dev_info(device, "%s: error reading from the slave device %d\n",
+			 __func__, ret);
+		goto err;
 	}
+	new_config_register[0] = th;		/* Byte 2 */
+	new_config_register[1] = tl;		/* Byte 3 */
+	new_config_register[2] = info.rom[4];	/* Byte 4 */
 
 	/* Write data in the device RAM */
 	if (!SLAVE_SPECIFIC_FUNC(sl)) {
-		dev_info(device,
-			"%s: Device not supported by the driver %d\n",
-			__func__, -ENODEV);
-		goto free_m;
+		dev_info(device, "%s: Device not supported by the driver %d\n",
+			 __func__, -ENODEV);
+		goto err;
 	}
 
 	ret = SLAVE_SPECIFIC_FUNC(sl)->write_data(sl, new_config_register);
-	if (ret)
-		dev_info(device,
-			"%s: error writing to the slave device %d\n",
+	if (ret) {
+		dev_info(device, "%s: error writing to the slave device %d\n",
 			__func__, ret);
+		goto err;
+	}
 
-free_m:
-	/* free allocated memory */
-	kfree(orig);
-
+err:
 	return size;
 }
 
-- 
2.51.0

Re: [PATCH v3] w1: therm: Fix off-by-one buffer overflow in alarms_store

[kernel test robot]

BCC: lkp@intel.com
CC: oe-kbuild-all@lists.linux.dev
In-Reply-To: <20251030155614.447905-1-thorsten.blum@linux.dev>
References: <20251030155614.447905-1-thorsten.blum@linux.dev>
TO: Thorsten Blum <thorsten.blum@linux.dev>
TO: David Laight <david.laight.linux@gmail.com>
TO: Krzysztof Kozlowski <krzk@kernel.org>
TO: Huisong Li <lihuisong@huawei.com>
TO: Akira Shimahara <akira215corp@gmail.com>
TO: "Greg Kroah-Hartman" <gregkh@linuxfoundation.org>
CC: Thorsten Blum <thorsten.blum@linux.dev>
CC: stable@vger.kernel.org
CC: linux-kernel@vger.kernel.org

Hi Thorsten,

kernel test robot noticed the following build warnings:

[auto build test WARNING on krzk-w1/for-next]
[also build test WARNING on linus/master v6.18-rc3 next-20251031]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Thorsten-Blum/w1-therm-Fix-off-by-one-buffer-overflow-in-alarms_store/20251031-000306
base:   https://git.kernel.org/pub/scm/linux/kernel/git/krzk/linux-w1.git for-next
patch link:    https://lore.kernel.org/r/20251030155614.447905-1-thorsten.blum%40linux.dev
patch subject: [PATCH v3] w1: therm: Fix off-by-one buffer overflow in alarms_store
:::::: branch date: 2 days ago
:::::: commit date: 2 days ago
config: i386-randconfig-141-20251101 (https://download.01.org/0day-ci/archive/20251102/202511020405.22bJ76JV-lkp@intel.com/config)
compiler: clang version 20.1.8 (https://github.com/llvm/llvm-project 87f0227cb60147a26a1eeb4fb06e3b505e9c7261)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <error27@gmail.com>
| Closes: https://lore.kernel.org/r/202511020405.22bJ76JV-lkp@intel.com/

New smatch warnings:
drivers/w1/slaves/w1_therm.c:1846 alarms_store() warn: impossible condition '(temp < (-((~0 >> 1)) - 1)) => (s32min-s32max < s32min)'
drivers/w1/slaves/w1_therm.c:1846 alarms_store() warn: impossible condition '(temp > ((~0 >> 1))) => (s32min-s32max > s32max)'

Old smatch warnings:
drivers/w1/slaves/w1_therm.c:1856 alarms_store() warn: impossible condition '(temp < (-((~0 >> 1)) - 1)) => (s32min-s32max < s32min)'
drivers/w1/slaves/w1_therm.c:1856 alarms_store() warn: impossible condition '(temp > ((~0 >> 1))) => (s32min-s32max > s32max)'

vim +1846 drivers/w1/slaves/w1_therm.c

e2c94d6f572079 Akira Shimahara 2020-05-11  1832  
e2c94d6f572079 Akira Shimahara 2020-05-11  1833  static ssize_t alarms_store(struct device *device,
e2c94d6f572079 Akira Shimahara 2020-05-11  1834  	struct device_attribute *attr, const char *buf, size_t size)
e2c94d6f572079 Akira Shimahara 2020-05-11  1835  {
e2c94d6f572079 Akira Shimahara 2020-05-11  1836  	struct w1_slave *sl = dev_to_w1_slave(device);
e2c94d6f572079 Akira Shimahara 2020-05-11  1837  	struct therm_info info;
e2c94d6f572079 Akira Shimahara 2020-05-11  1838  	u8 new_config_register[3];	/* array of data to be written */
fb05eaeed51f89 Thorsten Blum   2025-10-30  1839  	long temp;
fb05eaeed51f89 Thorsten Blum   2025-10-30  1840  	int ret;
e233897b1f7a85 Yang Guang      2021-12-21  1841  	s8 tl, th;	/* 1 byte per value + temp ring order */
fb05eaeed51f89 Thorsten Blum   2025-10-30  1842  	const char *p = buf;
fb05eaeed51f89 Thorsten Blum   2025-10-30  1843  	char *endp;
e2c94d6f572079 Akira Shimahara 2020-05-11  1844  
fb05eaeed51f89 Thorsten Blum   2025-10-30  1845  	temp = simple_strtol(p, &endp, 10);
fb05eaeed51f89 Thorsten Blum   2025-10-30 @1846  	if (temp < INT_MIN || temp > INT_MAX || p == endp || *endp != ' ') {
fb05eaeed51f89 Thorsten Blum   2025-10-30  1847  		dev_info(device, "%s: error parsing args %d\n",
fb05eaeed51f89 Thorsten Blum   2025-10-30  1848  			 __func__, -EINVAL);
fb05eaeed51f89 Thorsten Blum   2025-10-30  1849  		goto err;
e2c94d6f572079 Akira Shimahara 2020-05-11  1850  	}
fb05eaeed51f89 Thorsten Blum   2025-10-30  1851  	/* Cast to short to eliminate out of range values */
fb05eaeed51f89 Thorsten Blum   2025-10-30  1852  	tl = int_to_short((int)temp);
e2c94d6f572079 Akira Shimahara 2020-05-11  1853  
fb05eaeed51f89 Thorsten Blum   2025-10-30  1854  	p = endp + 1;
fb05eaeed51f89 Thorsten Blum   2025-10-30  1855  	temp = simple_strtol(p, &endp, 10);
fb05eaeed51f89 Thorsten Blum   2025-10-30  1856  	if (temp < INT_MIN || temp > INT_MAX || p == endp) {
fb05eaeed51f89 Thorsten Blum   2025-10-30  1857  		dev_info(device, "%s: error parsing args %d\n",
fb05eaeed51f89 Thorsten Blum   2025-10-30  1858  			 __func__, -EINVAL);
fb05eaeed51f89 Thorsten Blum   2025-10-30  1859  		goto err;
e2c94d6f572079 Akira Shimahara 2020-05-11  1860  	}
fb05eaeed51f89 Thorsten Blum   2025-10-30  1861  	/* Cast to short to eliminate out of range values */
fb05eaeed51f89 Thorsten Blum   2025-10-30  1862  	th = int_to_short((int)temp);
e2c94d6f572079 Akira Shimahara 2020-05-11  1863  
fb05eaeed51f89 Thorsten Blum   2025-10-30  1864  	/* Reorder if required */
e233897b1f7a85 Yang Guang      2021-12-21  1865  	if (tl > th)
e233897b1f7a85 Yang Guang      2021-12-21  1866  		swap(tl, th);
e2c94d6f572079 Akira Shimahara 2020-05-11  1867  
e2c94d6f572079 Akira Shimahara 2020-05-11  1868  	/*
e2c94d6f572079 Akira Shimahara 2020-05-11  1869  	 * Read the scratchpad to change only the required bits
e2c94d6f572079 Akira Shimahara 2020-05-11  1870  	 * (th : byte 2 - tl: byte 3)
e2c94d6f572079 Akira Shimahara 2020-05-11  1871  	 */
e2c94d6f572079 Akira Shimahara 2020-05-11  1872  	ret = read_scratchpad(sl, &info);
fb05eaeed51f89 Thorsten Blum   2025-10-30  1873  	if (ret) {
fb05eaeed51f89 Thorsten Blum   2025-10-30  1874  		dev_info(device, "%s: error reading from the slave device %d\n",
fb05eaeed51f89 Thorsten Blum   2025-10-30  1875  			 __func__, ret);
fb05eaeed51f89 Thorsten Blum   2025-10-30  1876  		goto err;
fb05eaeed51f89 Thorsten Blum   2025-10-30  1877  	}
e2c94d6f572079 Akira Shimahara 2020-05-11  1878  	new_config_register[0] = th;		/* Byte 2 */
e2c94d6f572079 Akira Shimahara 2020-05-11  1879  	new_config_register[1] = tl;		/* Byte 3 */
e2c94d6f572079 Akira Shimahara 2020-05-11  1880  	new_config_register[2] = info.rom[4];	/* Byte 4 */
e2c94d6f572079 Akira Shimahara 2020-05-11  1881  
e2c94d6f572079 Akira Shimahara 2020-05-11  1882  	/* Write data in the device RAM */
e2c94d6f572079 Akira Shimahara 2020-05-11  1883  	if (!SLAVE_SPECIFIC_FUNC(sl)) {
fb05eaeed51f89 Thorsten Blum   2025-10-30  1884  		dev_info(device, "%s: Device not supported by the driver %d\n",
e2c94d6f572079 Akira Shimahara 2020-05-11  1885  			 __func__, -ENODEV);
fb05eaeed51f89 Thorsten Blum   2025-10-30  1886  		goto err;
e2c94d6f572079 Akira Shimahara 2020-05-11  1887  	}
e2c94d6f572079 Akira Shimahara 2020-05-11  1888  
e2c94d6f572079 Akira Shimahara 2020-05-11  1889  	ret = SLAVE_SPECIFIC_FUNC(sl)->write_data(sl, new_config_register);
fb05eaeed51f89 Thorsten Blum   2025-10-30  1890  	if (ret) {
fb05eaeed51f89 Thorsten Blum   2025-10-30  1891  		dev_info(device, "%s: error writing to the slave device %d\n",
e2c94d6f572079 Akira Shimahara 2020-05-11  1892  			__func__, ret);
fb05eaeed51f89 Thorsten Blum   2025-10-30  1893  		goto err;
fb05eaeed51f89 Thorsten Blum   2025-10-30  1894  	}
e2c94d6f572079 Akira Shimahara 2020-05-11  1895  
fb05eaeed51f89 Thorsten Blum   2025-10-30  1896  err:
e2c94d6f572079 Akira Shimahara 2020-05-11  1897  	return size;
e2c94d6f572079 Akira Shimahara 2020-05-11  1898  }
e2c94d6f572079 Akira Shimahara 2020-05-11  1899  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v3] w1: therm: Fix off-by-one buffer overflow in alarms_store

[Krzysztof Kozlowski]

On 30/10/2025 16:56, Thorsten Blum wrote:
> -	/* Convert 2nd entry to int */
> -	ret = kstrtoint (token, 10, &temp);
> -	if (ret) {
> -		dev_info(device,
> -			"%s: error parsing args %d\n", __func__, ret);
> -		goto free_m;
> +	p = endp + 1;
> +	temp = simple_strtol(p, &endp, 10);
> +	if (temp < INT_MIN || temp > INT_MAX || p == endp) {
> +		dev_info(device, "%s: error parsing args %d\n",
> +			 __func__, -EINVAL);
> +		goto err;
>  	}
> +	/* Cast to short to eliminate out of range values */
> +	th = int_to_short((int)temp);
>  
> -	/* Prepare to cast to short by eliminating out of range values */
> -	th = int_to_short(temp);
> -
> -	/* Reorder if required th and tl */
> +	/* Reorder if required */
>  	if (tl > th)
>  		swap(tl, th);
>  
> @@ -1897,35 +1870,30 @@ static ssize_t alarms_store(struct device *device,
>  	 * (th : byte 2 - tl: byte 3)
>  	 */
>  	ret = read_scratchpad(sl, &info);
> -	if (!ret) {
> -		new_config_register[0] = th;	/* Byte 2 */
> -		new_config_register[1] = tl;	/* Byte 3 */
> -		new_config_register[2] = info.rom[4];/* Byte 4 */
> -	} else {
> -		dev_info(device,
> -			"%s: error reading from the slave device %d\n",
> -			__func__, ret);
> -		goto free_m;
> +	if (ret) {
> +		dev_info(device, "%s: error reading from the slave device %d\n",
> +			 __func__, ret);
> +		goto err;
>  	}
> +	new_config_register[0] = th;		/* Byte 2 */
> +	new_config_register[1] = tl;		/* Byte 3 */
> +	new_config_register[2] = info.rom[4];	/* Byte 4 */

How is this change related?

>  
>  	/* Write data in the device RAM */
>  	if (!SLAVE_SPECIFIC_FUNC(sl)) {
> -		dev_info(device,
> -			"%s: Device not supported by the driver %d\n",
> -			__func__, -ENODEV);
> -		goto free_m;
> +		dev_info(device, "%s: Device not supported by the driver %d\n",
> +			 __func__, -ENODEV);

Do not introduce other formatting changes. This patch is already
difficult to read.

> +		goto err;
>  	}

Best regards,
Krzysztof

Re: [PATCH v3] w1: therm: Fix off-by-one buffer overflow in alarms_store

[Thorsten Blum]

On 9. Nov 2025, at 19:29, Krzysztof Kozlowski wrote:
> On 30/10/2025 16:56, Thorsten Blum wrote:
>> -	/* Convert 2nd entry to int */
>> -	ret = kstrtoint (token, 10, &temp);
>> -	if (ret) {
>> -		dev_info(device,
>> -			"%s: error parsing args %d\n", __func__, ret);
>> -		goto free_m;
>> +	p = endp + 1;
>> +	temp = simple_strtol(p, &endp, 10);
>> +	if (temp < INT_MIN || temp > INT_MAX || p == endp) {
>> +		dev_info(device, "%s: error parsing args %d\n",
>> +			 __func__, -EINVAL);
>> +		goto err;
>> 	}
>> +	/* Cast to short to eliminate out of range values */
>> +	th = int_to_short((int)temp);
>> 
>> -	/* Prepare to cast to short by eliminating out of range values */
>> -	th = int_to_short(temp);
>> -
>> -	/* Reorder if required th and tl */
>> +	/* Reorder if required */
>> 	if (tl > th)
>> 		swap(tl, th);
>> 
>> @@ -1897,35 +1870,30 @@ static ssize_t alarms_store(struct device *device,
>> 	 * (th : byte 2 - tl: byte 3)
>> 	 */
>> 	ret = read_scratchpad(sl, &info);
>> -	if (!ret) {
>> -		new_config_register[0] = th;	/* Byte 2 */
>> -		new_config_register[1] = tl;	/* Byte 3 */
>> -		new_config_register[2] = info.rom[4];/* Byte 4 */
>> -	} else {
>> -		dev_info(device,
>> -			"%s: error reading from the slave device %d\n",
>> -			__func__, ret);
>> -		goto free_m;
>> +	if (ret) {
>> +		dev_info(device, "%s: error reading from the slave device %d\n",
>> +			 __func__, ret);
>> +		goto err;
>> 	}
>> +	new_config_register[0] = th;		/* Byte 2 */
>> +	new_config_register[1] = tl;		/* Byte 3 */
>> +	new_config_register[2] = info.rom[4];	/* Byte 4 *
> 
> How is this change related?

Not related, but I thought when I'm already rewriting 80% of the
function, I might as well just improve the indentation/formatting.

Thanks,
Thorsten

Re: [PATCH v3] w1: therm: Fix off-by-one buffer overflow in alarms_store

[David Laight]

On Thu, 30 Oct 2025 16:56:09 +0100
Thorsten Blum <thorsten.blum@linux.dev> 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_strtol()
> 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 <thorsten.blum@linux.dev>
> ---
> Compile-tested only.
> 
> 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 | 102 ++++++++++++-----------------------
>  1 file changed, 35 insertions(+), 67 deletions(-)
> 
> diff --git a/drivers/w1/slaves/w1_therm.c b/drivers/w1/slaves/w1_therm.c
> index 9ccedb3264fb..1dad9fa1ec4a 100644
> --- a/drivers/w1/slaves/w1_therm.c
> +++ b/drivers/w1/slaves/w1_therm.c
> @@ -1836,59 +1836,32 @@ 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 temp;
> +	int ret;
>  	s8 tl, th;	/* 1 byte per value + temp ring order */
> -	char *p_args, *orig;
> +	const char *p = buf;
> +	char *endp;
>  
> -	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;
> +	temp = simple_strtol(p, &endp, 10);
> +	if (temp < INT_MIN || temp > INT_MAX || p == endp || *endp != ' ') {
> +		dev_info(device, "%s: error parsing args %d\n",
> +			 __func__, -EINVAL);
> +		goto err;
>  	}
> -	strcpy(p_args, buf);
> -
> -	/* Split string using space char */
> -	token = strsep(&p_args, " ");
> +	/* Cast to short to eliminate out of range values */
> +	tl = int_to_short((int)temp);

What is that all about (still) ?
The function name doesn't match what it is doing at all.
The comment is completely 'left field'.
You seem to generating an error for values outside INT_MIN..INT_MAX and
then using clamp() to convert large -ve values to (probably) -128 and
large +ve ones to +127.
If that is what you want (rather than erroring values between 127 and
INT_MAX, or just clamping values above INT_MAX on 64bit systems) then
after the bound check just do:
	tl = clamp(temp, MIN_TEMP, MAX_TEMP);
then the same for 'th'.

A little later perhaps you want:
	if (tl < th) {
		new_config_register[0] = th;
		new_config_register[1] = tl;
	} else {
		new_config_register[0] = tl;
		new_config_register[1] = th;
	}
Probably even before determining info.rom[4].
The generated code will be better (especially on non-x86) if
both 'tl' and 'th' are 'int' (not s8).
In fact, just make them 'long' - probably as temp_hi and temp_lo
and kill the 'temp' variable completely.
		
	David



>  
> -	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);
> -	if (ret) {
> -		dev_info(device,
> -			"%s: error parsing args %d\n", __func__, ret);
> -		goto free_m;
> -	}
> -
> -	tl = int_to_short(temp);
> -
> -	/* 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 2nd entry to int */
> -	ret = kstrtoint (token, 10, &temp);
> -	if (ret) {
> -		dev_info(device,
> -			"%s: error parsing args %d\n", __func__, ret);
> -		goto free_m;
> +	p = endp + 1;
> +	temp = simple_strtol(p, &endp, 10);
> +	if (temp < INT_MIN || temp > INT_MAX || p == endp) {
> +		dev_info(device, "%s: error parsing args %d\n",
> +			 __func__, -EINVAL);
> +		goto err;
>  	}
> +	/* Cast to short to eliminate out of range values */
> +	th = int_to_short((int)temp);
>  
> -	/* Prepare to cast to short by eliminating out of range values */
> -	th = int_to_short(temp);
> -
> -	/* Reorder if required th and tl */
> +	/* Reorder if required */
>  	if (tl > th)
>  		swap(tl, th);
>  
> @@ -1897,35 +1870,30 @@ static ssize_t alarms_store(struct device *device,
>  	 * (th : byte 2 - tl: byte 3)
>  	 */
>  	ret = read_scratchpad(sl, &info);
> -	if (!ret) {
> -		new_config_register[0] = th;	/* Byte 2 */
> -		new_config_register[1] = tl;	/* Byte 3 */
> -		new_config_register[2] = info.rom[4];/* Byte 4 */
> -	} else {
> -		dev_info(device,
> -			"%s: error reading from the slave device %d\n",
> -			__func__, ret);
> -		goto free_m;
> +	if (ret) {
> +		dev_info(device, "%s: error reading from the slave device %d\n",
> +			 __func__, ret);
> +		goto err;
>  	}
> +	new_config_register[0] = th;		/* Byte 2 */
> +	new_config_register[1] = tl;		/* Byte 3 */
> +	new_config_register[2] = info.rom[4];	/* Byte 4 */
>  
>  	/* Write data in the device RAM */
>  	if (!SLAVE_SPECIFIC_FUNC(sl)) {
> -		dev_info(device,
> -			"%s: Device not supported by the driver %d\n",
> -			__func__, -ENODEV);
> -		goto free_m;
> +		dev_info(device, "%s: Device not supported by the driver %d\n",
> +			 __func__, -ENODEV);
> +		goto err;
>  	}
>  
>  	ret = SLAVE_SPECIFIC_FUNC(sl)->write_data(sl, new_config_register);
> -	if (ret)
> -		dev_info(device,
> -			"%s: error writing to the slave device %d\n",
> +	if (ret) {
> +		dev_info(device, "%s: error writing to the slave device %d\n",
>  			__func__, ret);
> +		goto err;
> +	}
>  
> -free_m:
> -	/* free allocated memory */
> -	kfree(orig);
> -
> +err:
>  	return size;
>  }
>  

Re: [PATCH v3] w1: therm: Fix off-by-one buffer overflow in alarms_store

[Krzysztof Kozlowski]

On 09/11/2025 23:11, Thorsten Blum wrote:
> On 9. Nov 2025, at 19:29, Krzysztof Kozlowski wrote:
>> On 30/10/2025 16:56, Thorsten Blum wrote:
>>> -	/* Convert 2nd entry to int */
>>> -	ret = kstrtoint (token, 10, &temp);
>>> -	if (ret) {
>>> -		dev_info(device,
>>> -			"%s: error parsing args %d\n", __func__, ret);
>>> -		goto free_m;
>>> +	p = endp + 1;
>>> +	temp = simple_strtol(p, &endp, 10);
>>> +	if (temp < INT_MIN || temp > INT_MAX || p == endp) {
>>> +		dev_info(device, "%s: error parsing args %d\n",
>>> +			 __func__, -EINVAL);
>>> +		goto err;
>>> 	}
>>> +	/* Cast to short to eliminate out of range values */
>>> +	th = int_to_short((int)temp);
>>>
>>> -	/* Prepare to cast to short by eliminating out of range values */
>>> -	th = int_to_short(temp);
>>> -
>>> -	/* Reorder if required th and tl */
>>> +	/* Reorder if required */
>>> 	if (tl > th)
>>> 		swap(tl, th);
>>>
>>> @@ -1897,35 +1870,30 @@ static ssize_t alarms_store(struct device *device,
>>> 	 * (th : byte 2 - tl: byte 3)
>>> 	 */
>>> 	ret = read_scratchpad(sl, &info);
>>> -	if (!ret) {
>>> -		new_config_register[0] = th;	/* Byte 2 */
>>> -		new_config_register[1] = tl;	/* Byte 3 */
>>> -		new_config_register[2] = info.rom[4];/* Byte 4 */
>>> -	} else {
>>> -		dev_info(device,
>>> -			"%s: error reading from the slave device %d\n",
>>> -			__func__, ret);
>>> -		goto free_m;
>>> +	if (ret) {
>>> +		dev_info(device, "%s: error reading from the slave device %d\n",
>>> +			 __func__, ret);
>>> +		goto err;
>>> 	}
>>> +	new_config_register[0] = th;		/* Byte 2 */
>>> +	new_config_register[1] = tl;		/* Byte 3 */
>>> +	new_config_register[2] = info.rom[4];	/* Byte 4 *
>>
>> How is this change related?
> 
> Not related, but I thought when I'm already rewriting 80% of the
> function, I might as well just improve the indentation/formatting.

Fix of buffer overflow should not contain any style changes. And
definitely changing if/else logic is just style.

Best regards,
Krzysztof