[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

[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