* Re: [PATCH] hwmon: sht3x: Fix unaligned accesses
2026-07-25 22:02 [PATCH] hwmon: sht3x: Fix unaligned accesses Guenter Roeck
@ 2026-07-25 22:22 ` sashiko-bot
0 siblings, 0 replies; 2+ messages in thread
From: sashiko-bot @ 2026-07-25 22:22 UTC (permalink / raw)
To: Guenter Roeck; +Cc: linux-hwmon
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
^ permalink raw reply [flat|nested] 2+ messages in thread