hwmon: (w83l786ng) Convert macros to functions to avoid TOCTOU

[PATCH] hwmon: (w83l786ng) Convert macros to functions to avoid TOCTOU

Posted by Gui-Dong Han 3 days, 8 hours ago

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@vger.kernel.org
Signed-off-by: Gui-Dong Han <hanguidong02@gmail.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))
-#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;
+}
 
 /*
  * The analog voltage inputs have 8mV LSB. Since the sysfs output is
@@ -280,7 +290,7 @@ static ssize_t show_##reg(struct device *dev, struct device_attribute *attr, \
 	int nr = to_sensor_dev_attr(attr)->index; \
 	struct w83l786ng_data *data = w83l786ng_update_device(dev); \
 	return sprintf(buf, "%d\n", \
-		FAN_FROM_REG(data->reg[nr], DIV_FROM_REG(data->fan_div[nr]))); \
+		fan_from_reg(data->reg[nr], DIV_FROM_REG(data->fan_div[nr]))); \
 }
 
 show_fan_reg(fan);
@@ -347,7 +357,7 @@ store_fan_div(struct device *dev, struct device_attribute *attr,
 
 	/* Save fan_min */
 	mutex_lock(&data->update_lock);
-	min = FAN_FROM_REG(data->fan_min[nr], DIV_FROM_REG(data->fan_div[nr]));
+	min = fan_from_reg(data->fan_min[nr], DIV_FROM_REG(data->fan_div[nr]));
 
 	data->fan_div[nr] = DIV_TO_REG(val);
 
@@ -409,7 +419,7 @@ show_temp(struct device *dev, struct device_attribute *attr, char *buf)
 	int nr = sensor_attr->nr;
 	int index = sensor_attr->index;
 	struct w83l786ng_data *data = w83l786ng_update_device(dev);
-	return sprintf(buf, "%d\n", TEMP_FROM_REG(data->temp[nr][index]));
+	return sprintf(buf, "%d\n", temp_from_reg(data->temp[nr][index]));
 }
 
 static ssize_t
-- 
2.43.0

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

Posted by david laight 3 days, 1 hour ago

On Fri, 28 Nov 2025 20:38:16 +0800
Gui-Dong Han <hanguidong02@gmail.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@vger.kernel.org
> Signed-off-by: Gui-Dong Han <hanguidong02@gmail.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.

	David

> -#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;
> +}
>  
>  /*
>   * The analog voltage inputs have 8mV LSB. Since the sysfs output is
> @@ -280,7 +290,7 @@ static ssize_t show_##reg(struct device *dev, struct device_attribute *attr, \
>  	int nr = to_sensor_dev_attr(attr)->index; \
>  	struct w83l786ng_data *data = w83l786ng_update_device(dev); \
>  	return sprintf(buf, "%d\n", \
> -		FAN_FROM_REG(data->reg[nr], DIV_FROM_REG(data->fan_div[nr]))); \
> +		fan_from_reg(data->reg[nr], DIV_FROM_REG(data->fan_div[nr]))); \
>  }
>  
>  show_fan_reg(fan);
> @@ -347,7 +357,7 @@ store_fan_div(struct device *dev, struct device_attribute *attr,
>  
>  	/* Save fan_min */
>  	mutex_lock(&data->update_lock);
> -	min = FAN_FROM_REG(data->fan_min[nr], DIV_FROM_REG(data->fan_div[nr]));
> +	min = fan_from_reg(data->fan_min[nr], DIV_FROM_REG(data->fan_div[nr]));
>  
>  	data->fan_div[nr] = DIV_TO_REG(val);
>  
> @@ -409,7 +419,7 @@ show_temp(struct device *dev, struct device_attribute *attr, char *buf)
>  	int nr = sensor_attr->nr;
>  	int index = sensor_attr->index;
>  	struct w83l786ng_data *data = w83l786ng_update_device(dev);
> -	return sprintf(buf, "%d\n", TEMP_FROM_REG(data->temp[nr][index]));
> +	return sprintf(buf, "%d\n", temp_from_reg(data->temp[nr][index]));
>  }
>  
>  static ssize_t

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

Posted by Gui-Dong Han 2 days, 21 hours ago

On Sat, Nov 29, 2025 at 3:37 AM david laight <david.laight@runbox.com> wrote:
>
> On Fri, 28 Nov 2025 20:38:16 +0800
> Gui-Dong Han <hanguidong02@gmail.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@vger.kernel.org
> > Signed-off-by: Gui-Dong Han <hanguidong02@gmail.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. 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.

Best regards,
Gui-Dong Han

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

Posted by david laight 2 days, 11 hours ago

On Sat, 29 Nov 2025 08:33:42 +0800
Gui-Dong Han <hanguidong02@gmail.com> wrote:

> On Sat, Nov 29, 2025 at 3:37 AM david laight <david.laight@runbox.com> wrote:
> >
> > On Fri, 28 Nov 2025 20:38:16 +0800
> > Gui-Dong Han <hanguidong02@gmail.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@vger.kernel.org
> > > Signed-off-by: Gui-Dong Han <hanguidong02@gmail.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.

> 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:

-#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.
They are just ((s8)(val) * 1000) and generate a milli-centigrade
value from the 8-bit signed centigrade value the hardware provides.

TEMP_TO_REG() is the opposite, so is:
	(u8)clamp((val) / 1000, -128, 127)
That is subtly different since it truncates negative values towards
zero rather than -infinity.
The more complicated:
		(u8)(clamp(((val) + 128 * 1000)/1000, 0, 0xff) - 128)
would exactly match the original (and generate less code) but I suspect
it doesn't matter here.

	David

> 
> Best regards,
> Gui-Dong Han
>

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

Posted by Guenter Roeck 2 days, 5 hours ago

On 11/29/25 02:17, david laight wrote:
> On Sat, 29 Nov 2025 08:33:42 +0800
> Gui-Dong Han <hanguidong02@gmail.com> wrote:
> 
>> On Sat, Nov 29, 2025 at 3:37 AM david laight <david.laight@runbox.com> wrote:
>>>
>>> On Fri, 28 Nov 2025 20:38:16 +0800
>>> Gui-Dong Han <hanguidong02@gmail.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@vger.kernel.org
>>>> Signed-off-by: Gui-Dong Han <hanguidong02@gmail.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

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

Posted by Guenter Roeck 3 days, 5 hours ago

On Fri, Nov 28, 2025 at 08:38:16PM +0800, Gui-Dong Han 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@vger.kernel.org
> Signed-off-by: Gui-Dong Han <hanguidong02@gmail.com>
> ---

Applied.

Thanks,
Guenter