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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20251128193720.0716cc6d@pumpkin>
Date: Fri, 28 Nov 2025 19:37:20 +0000
From: david laight <david.laight@...box.com>
To: Gui-Dong Han <hanguidong02@...il.com>
Cc: linux@...ck-us.net, 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 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.

	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