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

[David Laight <david.laight.linux@gmail.com>]

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;
>  }
>