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 thorsten.blum@linux.dev --- 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); + 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; }
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); + p = endp + 1; + temp = simple_strtoll(p, &endp, 10); + if (p == 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; } - /* Prepare to cast to short by eliminating out of range values */ th = int_to_short(temp);
@@ -1905,7 +1886,7 @@ static ssize_t alarms_store(struct device *device, dev_info(device, "%s: error reading from the slave device %d\n", __func__, ret); - goto free_m; + goto err; }
/* Write data in the device RAM */ @@ -1913,7 +1894,7 @@ static ssize_t alarms_store(struct device *device, dev_info(device, "%s: Device not supported by the driver %d\n", __func__, -ENODEV); - goto free_m; + goto err; }
ret = SLAVE_SPECIFIC_FUNC(sl)->write_data(sl, new_config_register); @@ -1922,10 +1903,7 @@ static ssize_t alarms_store(struct device *device, "%s: error writing to the slave device %d\n", __func__, ret);
-free_m: - /* free allocated memory */ - kfree(orig); - +err: return size; }
Hi Krzysztof,
On 11. Nov 2025, at 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 thorsten.blum@linux.dev
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
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(-) [...]
Could you take another look at v4?
Thanks, Thorsten
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 thorsten.blum@linux.dev
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()?
- if (p == endp || *endp != ' ')
ret = -EINVAL;- else if (temp < INT_MIN || temp > INT_MAX)
ret = -ERANGE;
goto free_m;
goto err;
So this is just return size.
Best regards, Krzysztof
On Tue, 16 Dec 2025 08:11:13 +0100 Krzysztof Kozlowski krzk@kernel.org 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 thorsten.blum@linux.dev
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;
goto free_m;
goto err;So this is just return size.
Best regards, Krzysztof
On 16. Dec 2025, at 08:11, 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 thorsten.blum@linux.dev
[...]
- if (p == endp || *endp != ' ')
ret = -EINVAL;- else if (temp < INT_MIN || temp > INT_MAX)
ret = -ERANGE;
goto free_m;
goto err;So this is just return size.
Yes, all 'goto err' could be replaced with 'return size'. I only renamed the label to keep the changes minimal.
On 16/12/2025 13:30, Thorsten Blum wrote:
On 16. Dec 2025, at 08:11, 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 thorsten.blum@linux.dev
[...]
- if (p == endp || *endp != ' ')
ret = -EINVAL;- else if (temp < INT_MIN || temp > INT_MAX)
ret = -ERANGE;
goto free_m;
goto err;So this is just return size.
Yes, all 'goto err' could be replaced with 'return size'. I only renamed the label to keep the changes minimal.
You do not write commits to have minimal changes. That's not the goal. You organize commits in logical chunks doing one thing and doing it correctly. Empty goto label is not correct, thus should not stay.
Best regards, Krzysztof
On 16/12/2025 10:28, David Laight wrote:
On Tue, 16 Dec 2025 08:11:13 +0100 Krzysztof Kozlowski krzk@kernel.org 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 thorsten.blum@linux.dev
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.
Yes, there are two values here, so obviously this is right.
Best regards, Krzysztof
linux-stable-mirror@lists.linaro.org
-
David Laight -
Krzysztof Kozlowski -
Thorsten Blum