linux-kernel - Re: [PATCH] hwmon: (max16065) Use READ/WRITE_ONCE to avoid compiler optimization induced race

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: <20260207104308.1bc31102@pumpkin>
Date: Sat, 7 Feb 2026 10:43:08 +0000
From: David Laight <david.laight.linux@...il.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, Ben Hutchings
 <ben@...adent.org.uk>, stable@...r.kernel.org
Subject: Re: [PATCH] hwmon: (max16065) Use READ/WRITE_ONCE to avoid compiler
 optimization induced race

On Tue,  3 Feb 2026 20:14:43 +0800
Gui-Dong Han <hanguidong02@...il.com> wrote:

> Simply copying shared data to a local variable cannot prevent data
> races. The compiler is allowed to optimize away the local copy and
> re-read the shared memory, causing a Time-of-Check Time-of-Use (TOCTOU)
> issue if the data changes between the check and the usage.

While the compiler is allowed to do this, is there any indication
that either gcc or clang have ever done it?
ISTR someone saying that they never did - although I thought that
was the original justification for adding ACCESS_ONCE().

READ_ONCE() also includes barriers to guarantee ordering between cpu.
These are empty on x86 but add code to architectures where the cpu
can (IIRC) re-order writes.
This is worst on alpha but affects arm and probably ppc.

For these cases is it enough to add the compile-time barrier() after
reading the variable to a local.
That will also generate better code on x86.

The WRITE_ONCE() aren't needed at all, the compilers definitely
guarantee to do a single memory access for aligned accesses that are
less than the size of a word.

This all stinks of being an AI generated patch.

	David

> 
> To enforce the use of the local variable, use READ_ONCE() when reading
> the shared data and WRITE_ONCE() when updating it. Apply these macros to
> the three identified locations (curr_sense, adc, and fault) where local
> variables are used for error validation, ensuring the value remains
> consistent.
> 
> Reported-by: Ben Hutchings <ben@...adent.org.uk>
> Closes: https://lore.kernel.org/all/6fe17868327207e8b850cf9f88b7dc58b2021f73.camel@decadent.org.uk/
> Fixes: f5bae2642e3d ("hwmon: Driver for MAX16065 System Manager and compatibles")
> Fixes: b8d5acdcf525 ("hwmon: (max16065) Use local variable to avoid TOCTOU")
> Cc: stable@...r.kernel.org
> Signed-off-by: Gui-Dong Han <hanguidong02@...il.com>
> ---
>  drivers/hwmon/max16065.c | 26 +++++++++++++-------------
>  1 file changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/hwmon/max16065.c b/drivers/hwmon/max16065.c
> index 4c9e7892a73c..43fbb9b26b10 100644
> --- a/drivers/hwmon/max16065.c
> +++ b/drivers/hwmon/max16065.c
> @@ -151,27 +151,27 @@ static struct max16065_data *max16065_update_device(struct device *dev)
>  		int i;
>  
>  		for (i = 0; i < data->num_adc; i++)
> -			data->adc[i]
> -			  = max16065_read_adc(client, MAX16065_ADC(i));
> +			WRITE_ONCE(data->adc[i],
> +				   max16065_read_adc(client, MAX16065_ADC(i)));
>  
>  		if (data->have_current) {
> -			data->adc[MAX16065_NUM_ADC]
> -			  = max16065_read_adc(client, MAX16065_CSP_ADC);
> -			data->curr_sense
> -			  = i2c_smbus_read_byte_data(client,
> -						     MAX16065_CURR_SENSE);
> +			WRITE_ONCE(data->adc[MAX16065_NUM_ADC],
> +				   max16065_read_adc(client, MAX16065_CSP_ADC));
> +			WRITE_ONCE(data->curr_sense,
> +				   i2c_smbus_read_byte_data(client, MAX16065_CURR_SENSE));
>  		}
>  
>  		for (i = 0; i < 2; i++)
> -			data->fault[i]
> -			  = i2c_smbus_read_byte_data(client, MAX16065_FAULT(i));
> +			WRITE_ONCE(data->fault[i],
> +				   i2c_smbus_read_byte_data(client, MAX16065_FAULT(i)));
>  
>  		/*
>  		 * MAX16067 and MAX16068 have separate undervoltage and
>  		 * overvoltage alarm bits. Squash them together.
>  		 */
>  		if (data->chip == max16067 || data->chip == max16068)
> -			data->fault[0] |= data->fault[1];
> +			WRITE_ONCE(data->fault[0],
> +				   data->fault[0] | data->fault[1]);
>  
>  		data->last_updated = jiffies;
>  		data->valid = true;
> @@ -185,7 +185,7 @@ static ssize_t max16065_alarm_show(struct device *dev,
>  {
>  	struct sensor_device_attribute_2 *attr2 = to_sensor_dev_attr_2(da);
>  	struct max16065_data *data = max16065_update_device(dev);
> -	int val = data->fault[attr2->nr];
> +	int val = READ_ONCE(data->fault[attr2->nr]);
>  
>  	if (val < 0)
>  		return val;
> @@ -203,7 +203,7 @@ static ssize_t max16065_input_show(struct device *dev,
>  {
>  	struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
>  	struct max16065_data *data = max16065_update_device(dev);
> -	int adc = data->adc[attr->index];
> +	int adc = READ_ONCE(data->adc[attr->index]);
>  
>  	if (unlikely(adc < 0))
>  		return adc;
> @@ -216,7 +216,7 @@ static ssize_t max16065_current_show(struct device *dev,
>  				     struct device_attribute *da, char *buf)
>  {
>  	struct max16065_data *data = max16065_update_device(dev);
> -	int curr_sense = data->curr_sense;
> +	int curr_sense = READ_ONCE(data->curr_sense);
>  
>  	if (unlikely(curr_sense < 0))
>  		return curr_sense;