Re: [PATCH] hwmon: sht3x: Fix unaligned accesses

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

Pre-existing issues:
- [High] 32-bit unsigned integer overflow in `limit_write()` when calculating raw limits for maximum valid inputs.
- [High] Missing `hwmon_lock()`, lockless state mutation, and missing sensor command update in `repeatability_store()`.
- [High] I2C protocol violation: driver requests 6 bytes when reading 3-byte limit registers.
- [Medium] Sensor read operations ignore hardware-provided CRC bytes, risking silent data corruption.
- [Medium] Concurrent device probes trigger an unsynchronized data race on the global `sht3x_crc8_table`.
--

--- Patch [1]: [PATCH] hwmon: sht3x: Fix unaligned accesses ---
commit 3b16c04f4d9f2de96142ecc5295ce7b48737c0c2
Author: Guenter Roeck <linux@roeck-us.net>

hwmon: sht3x: Fix unaligned accesses

In sht3x_update_client(), extracting 16-bit values from an unaligned byte
array using be16_to_cpup() triggers alignment faults on architectures like
ARMv5 or SPARC. This patch fixes the problem by replacing the unaligned
pointers with get_unaligned_be16() and put_unaligned_be16().

> diff --git a/drivers/hwmon/sht3x.c b/drivers/hwmon/sht3x.c
> index c2f6b73aa7f34..4d90f89a99297 100644
> --- a/drivers/hwmon/sht3x.c
> +++ b/drivers/hwmon/sht3x.c

[ ... ]

> @@ -276,9 +277,9 @@ static struct sht3x_data *sht3x_update_client(struct device *dev)
>  		if (ret)
>  			goto out;
>  
> -		val = be16_to_cpup((__be16 *)buf);
> +		val = get_unaligned_be16(buf);
>  		data->temperature = sht3x_extract_temperature(val);
> -		val = be16_to_cpup((__be16 *)(buf + 3));
> +		val = get_unaligned_be16(buf + 3);

[Severity: Medium]
This is a pre-existing issue, but does this ignore the hardware-provided CRC
bytes?

The SHT3x sensor returns 6 bytes per measurement (Temperature, CRC, Humidity,
CRC). Using offsets 0 and 3 completely skips the interleaved CRC bytes at
indices 2 and 5 without validation, risking silent data corruption from I2C bus
interference during sht3x_update_client().

>  		data->humidity = sht3x_extract_humidity(val);
>  		data->last_update = jiffies;
>  	}

[ ... ]

> @@ -336,7 +337,7 @@ static int limits_update(struct sht3x_data *data)
>  		if (ret)
>  			return ret;
>  
> -		raw = be16_to_cpup((__be16 *)buffer);
> +		raw = get_unaligned_be16(buffer);
>  		temperature = sht3x_extract_temperature((raw & 0x01ff) << 7);

[Severity: High]
This is a pre-existing issue, but does this read 6 bytes for a hardware
command that only returns 3?

In limits_update(), the driver calls sht3x_read_from_command() with
SHT3X_RESPONSE_LENGTH (6 bytes). However, the limit read command (e.g. 0xE11F)
only returns a single 16-bit word and an 8-bit CRC (3 bytes total).

Requesting 6 bytes forces the I2C master to clock 3 extra bytes after the
sensor stops transmitting, which can cause I2C protocol errors or NAK timeouts
and abort the driver probe on strict I2C controllers.

>  		humidity = sht3x_extract_humidity(raw & 0xfe00);
>  		data->temperature_limits[index] = temperature;
>  		data->humidity_limits[index] = humidity;

[ ... ]

> @@ -389,7 +390,7 @@ static size_t limit_write(struct device *dev,
>  	raw = ((u32)(temperature + 45000) * 24543) >> (16 + 7);
>  	raw |= ((humidity * 42950) >> 16) & 0xfe00;
>  

[Severity: High]
This isn't a bug introduced by this patch, but does this calculation overflow
for maximum valid inputs?

When userspace writes the valid maximum temperature (130000) or humidity
(100000) to limit_write(), the 32-bit integer arithmetic
((u32)(temperature + 45000) * 24543) equals 4,295,025,000, and
(humidity * 42950) equals 4,295,000,000.

Both exceed U32_MAX, silently wrapping around before the bitwise shifts. This
results in configuring the sensor to extreme minimums instead of maximums,
breaking thermal management logic.

> -	*((__be16 *)position) = cpu_to_be16(raw);
> +	put_unaligned_be16(raw, position);
>  	position += SHT3X_WORD_LEN;
>  	*position = crc8(sht3x_crc8_table,

[Severity: High]
This is a pre-existing issue, but does this driver fail to use hwmon_lock()
for sysfs attributes?

The hwmon subsystem guidelines require hwmon_lock() for attributes registered
outside the info structure, but heater_enable_store() uses a private lock:

drivers/hwmon/sht3x.c:heater_enable_store() {
    ...
	mutex_lock(&data->i2c_lock);

	if (status)
		ret = i2c_master_send(client, (char *)&sht3x_cmd_heater_on,
    ...
}

Additionally, repeatability_store() fails to take any lock, mutating
data->repeatability locklessly:

drivers/hwmon/sht3x.c:repeatability_store() {
    ...
	if (val > 2)
		return -EINVAL;

	data->repeatability = val;

	return count;
}

It also fails to call sht3x_select_command() after updating the value. For
single-shot measurements, sht3x_update_client() relies on data->command, which
becomes stale, meaning the repeatability change takes no effect until another
attribute is modified.

[Severity: Medium]
This is a pre-existing issue, but does this unconditional population of a
global array cause a data race?

The global sht3x_crc8_table is populated on every probe invocation without
DO_ONCE or lock protection:

drivers/hwmon/sht3x.c:sht3x_probe() {
    ...
	data->client = client;
	data->chip_id = (uintptr_t)i2c_get_match_data(client);
	crc8_populate_msb(sht3x_crc8_table, SHT3X_CRC8_POLYNOMIAL);

	sht3x_select_command(data);
    ...
}

Concurrent driver probes will write to this shared global array
simultaneously, which could lead to data races.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260725220205.1931189-1-linux@roeck-us.net?part=1