linux-kernel - Re: [PATCH v4] 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: <1f17a863-20c9-4004-ae8a-08cab5bde237@kernel.org>
Date: Tue, 16 Dec 2025 14:17:15 +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 v4] w1: therm: Fix off-by-one buffer overflow in
 alarms_store

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@...r.kernel.org
>>> Fixes: e2c94d6f5720 ("w1_therm: adding alarm sysfs entry")
>>> Signed-off-by: Thorsten Blum <thorsten.blum@...ux.dev>
>>> ---
>>> [...]
>>>
>>> +	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;
>>
>> 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