linux-kernel - Re: [PATCH] hwmon: (w83l786ng) Convert macros to functions to avoid TOCTOU

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: <a989d07a-a473-4d9a-ae76-d39de6981e35@roeck-us.net>
Date: Sat, 29 Nov 2025 07:55:19 -0800
From: Guenter Roeck <linux@...ck-us.net>
To: david laight <david.laight@...box.com>,
 Gui-Dong Han <hanguidong02@...il.com>
Cc: linux-hwmon@...r.kernel.org, linux-kernel@...r.kernel.org,
 baijiaju1990@...il.com, stable@...r.kernel.org
Subject: Re: [PATCH] hwmon: (w83l786ng) Convert macros to functions to avoid
 TOCTOU

On 11/29/25 02:17, david laight wrote:
> On Sat, 29 Nov 2025 08:33:42 +0800
> Gui-Dong Han <hanguidong02@...il.com> wrote:
> 
>> On Sat, Nov 29, 2025 at 3:37 AM david laight <david.laight@...box.com> wrote:
>>>
>>> On Fri, 28 Nov 2025 20:38:16 +0800
>>> Gui-Dong Han <hanguidong02@...il.com> wrote:
>>>   
>>>> The macros FAN_FROM_REG and TEMP_FROM_REG evaluate their arguments
>>>> multiple times. When used in lockless contexts involving shared driver
>>>> data, this causes Time-of-Check to Time-of-Use (TOCTOU) race
>>>> conditions.
>>>>
>>>> Convert the macros to static functions. This guarantees that arguments
>>>> are evaluated only once (pass-by-value), preventing the race
>>>> conditions.
>>>>
>>>> Adhere to the principle of minimal changes by only converting macros
>>>> that evaluate arguments multiple times and are used in lockless
>>>> contexts.
>>>>
>>>> Link: https://lore.kernel.org/all/CALbr=LYJ_ehtp53HXEVkSpYoub+XYSTU8Rg=o1xxMJ8=5z8B-g@mail.gmail.com/
>>>> Fixes: 85f03bccd6e0 ("hwmon: Add support for Winbond W83L786NG/NR")
>>>> Cc: stable@...r.kernel.org
>>>> Signed-off-by: Gui-Dong Han <hanguidong02@...il.com>
>>>> ---
>>>> Based on the discussion in the link, I will submit a series of patches to
>>>> address TOCTOU issues in the hwmon subsystem by converting macros to
>>>> functions or adjusting locking where appropriate.
>>>> ---
>>>>   drivers/hwmon/w83l786ng.c | 26 ++++++++++++++++++--------
>>>>   1 file changed, 18 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/hwmon/w83l786ng.c b/drivers/hwmon/w83l786ng.c
>>>> index 9b81bd406e05..1d9109ca1585 100644
>>>> --- a/drivers/hwmon/w83l786ng.c
>>>> +++ b/drivers/hwmon/w83l786ng.c
>>>> @@ -76,15 +76,25 @@ FAN_TO_REG(long rpm, int div)
>>>>        return clamp_val((1350000 + rpm * div / 2) / (rpm * div), 1, 254);
>>>>   }
>>>>
>>>> -#define FAN_FROM_REG(val, div)       ((val) == 0   ? -1 : \
>>>> -                             ((val) == 255 ? 0 : \
>>>> -                             1350000 / ((val) * (div))))
>>>> +static int fan_from_reg(int val, int div)
>>>> +{
>>>> +     if (val == 0)
>>>> +             return -1;
>>>> +     if (val == 255)
>>>> +             return 0;
>>>> +     return 1350000 / (val * div);
>>>> +}
>>>>
>>>>   /* for temp */
>>>>   #define TEMP_TO_REG(val)     (clamp_val(((val) < 0 ? (val) + 0x100 * 1000 \
>>>>                                                      : (val)) / 1000, 0, 0xff))
>>>
>>> Can you change TEMP_TO_REG() as well.
>>> And just use plain clamp() while you are at it.
>>> Both these temperature conversion functions have to work with negative temperatures.
>>> But the signed-ness gets passed through from the parameter - which may not be right.
>>> IIRC some come from FIELD_GET() and will be 'unsigned long' unless cast somewhere.
>>> The function parameter 'corrects' the type to a signed one.
>>>
>>> So you are fixing potential bugs as well.
>>
>> Hi David,
>>
>> Thanks for your feedback on TEMP_TO_REG and the detailed explanation
>> regarding macro risks.
>>
>> Guenter has already applied this patch.
> 
> Patches are supposed to be posted for review and applied a few days later.
> 

As long as I am the maintainer of this code, if and when to apply a patch
after it was posted is my call to make.

>> Since the primary scope here
>> was strictly addressing TOCTOU race conditions (and TEMP_TO_REG is not
>> used in lockless contexts), it wasn't included.
>>
>> However, I appreciate your point regarding type safety. I will look
>> into addressing that in a future separate patch.
> 
> It's not just type safety, and #define that evaluates an argument more
> than one is just a bug waiting to happen.
> We've been removing (or trying not to write) those since the 1980s.
> 
> You also just didn't read the code:
> 

That is just a claim. It could be seen as insult, so I would kindly
ask you to refrain from such comments.

> -#define TEMP_FROM_REG(val)	(((val) & 0x80 ? \
> -				  (val) - 0x100 : (val)) * 1000)
> +
> +static int temp_from_reg(int val)
> +{
> +	if (val & 0x80)
> +		return (val - 0x100) * 1000;
> +	return val * 1000;
> +}
> 
> Both those only work if 'val' is 8 bits.

Yes, and it is. What exactly is your point ? It says "temp_from_reg",
and reg is an 8-bit value. Passing it as int doesn't change that.

> They are just ((s8)(val) * 1000) and generate a milli-centigrade
> value from the 8-bit signed centigrade value the hardware provides.

Exactly, and the code above does that. Yes, I know, "((s8)(val) * 1000)"
would have been more efficient, but, again, it is my call to decide if
I request that or if I think that the patch I accepted is good enough.

Thanks,
Guenter