[PATCH] hwmon: sht3x: Fix unaligned accesses

[PATCH] hwmon: sht3x: Fix unaligned accesses

[Guenter Roeck]

Sashiko reports:

In sht3x_update_client(), the 16-bit temperature and humidity values are
extracted from a stack-allocated byte array using be16_to_cpup(). The
pointers passed to this function are calculated as buf and buf + 3. Since
the difference between the two pointers is an odd number of bytes, at
least one of them is guaranteed to be at an unaligned offset.

This will trigger an alignment fault on strict-alignment architectures
such as ARMv5 or SPARC, resulting in a kernel panic.

Fix the problem by using get_unaligned_be16() instead of be16_to_cpup(),
and put_unaligned_be16() instead of cpu_to_be16().

Fixes: 7c84f7f80d6f ("hwmon: add support for Sensirion SHT3x sensors")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/sht3x.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/hwmon/sht3x.c b/drivers/hwmon/sht3x.c
index c2f6b73aa7f3..4d90f89a9929 100644
--- a/drivers/hwmon/sht3x.c
+++ b/drivers/hwmon/sht3x.c
@@ -21,6 +21,7 @@
 #include <linux/module.h>
 #include <linux/slab.h>
 #include <linux/jiffies.h>
+#include <linux/unaligned.h>
 
 /* commands (high repeatability mode) */
 static const unsigned char sht3x_cmd_measure_single_hpm[] = { 0x24, 0x00 };
@@ -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);
 		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);
 		humidity = sht3x_extract_humidity(raw & 0xfe00);
 		data->temperature_limits[index] = temperature;
@@ -389,7 +390,7 @@ static size_t limit_write(struct device *dev,
 	raw = ((u32)(temperature + 45000) * 24543) >> (16 + 7);
 	raw |= ((humidity * 42950) >> 16) & 0xfe00;
 
-	*((__be16 *)position) = cpu_to_be16(raw);
+	put_unaligned_be16(raw, position);
 	position += SHT3X_WORD_LEN;
 	*position = crc8(sht3x_crc8_table,
 			 position - SHT3X_WORD_LEN,
-- 
2.45.2

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

[sashiko-bot]

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