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