| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20251109225024.186845bb@pumpkin>
Date: Sun, 9 Nov 2025 22:50:24 +0000
From: David Laight <david.laight.linux@...il.com>
To: Thorsten Blum <thorsten.blum@...ux.dev>
Cc: Krzysztof Kozlowski <krzk@...nel.org>, Huisong Li
<lihuisong@...wei.com>, Akira Shimahara <akira215corp@...il.com>, Greg
Kroah-Hartman <gregkh@...uxfoundation.org>, stable@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] w1: therm: Fix off-by-one buffer overflow in
alarms_store
On Thu, 30 Oct 2025 16:56:09 +0100
Thorsten Blum <thorsten.blum@...ux.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@...r.kernel.org
> Fixes: e2c94d6f5720 ("w1_therm: adding alarm sysfs entry")
> Signed-off-by: Thorsten Blum <thorsten.blum@...ux.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;
> }
>
Powered by blists - more mailing lists