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

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <10440fd8-aa78-4d73-a927-1f808fbc42c7@kernel.org>
Date: Tue, 11 Nov 2025 10:37:16 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Thorsten Blum <thorsten.blum@...ux.dev>
Cc: David Laight <david.laight.linux@...il.com>,
 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 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