Re: [PATCH v7 3/4] hwmon: (sht3x) Add devicetree support

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Unaligned memory access when extracting the humidity value causes a kernel panic on strict-alignment architectures.
- [High] Integer overflow in `limit_write()` calculating raw limit values for temperature and humidity, resulting in limits wrapping around to 0.
- [High] Missing `hwmon_lock()` synchronization for extra sysfs attributes (`heater_enable_store`, `repeatability_store`).
- [High] Changing the 'repeatability' sysfs attribute fails to update the active measurement command, rendering the change ineffective until the update interval is toggled.
- [High] `heater_enable_store()` returns the I2C transfer length instead of the consumed buffer length, breaking sysfs writes.
--

--- Patch [3]: [PATCH v7 3/4] hwmon: (sht3x) Add devicetree support ---
commit 3205e8775fc638eb1ac86ddf6feacc8304401779
Author: Zaixiang Xu <zaixiang.xu.dev@gmail.com>

hwmon: (sht3x) Add devicetree support

Add an of_device_id table to support devicetree based instantiation of the
supported sensors. The match data distinguishes the humidity and temperature
parts (SHT3x) from the temperature-only parts (STS3x). Enum sht3x_chips is
started at 1 to prevent i2c_get_match_data() from falling back to device
id matching on devicetree names.

> diff --git a/drivers/hwmon/sht3x.c b/drivers/hwmon/sht3x.c
> index c2f6b73aa7f34..6d47cfd340f83 100644
> --- a/drivers/hwmon/sht3x.c
> +++ b/drivers/hwmon/sht3x.c
> @@ -61,7 +61,7 @@ static const unsigned char sht3x_cmd_read_serial_number[]      = { 0x37, 0x80 };
>  #define SHT3X_MAX_HUMIDITY     100000
>  
>  enum sht3x_chips {
> -	sht3x,
> +	sht3x = 1,
>  	sts3x,
>  };

[ ... ]

> @@ -939,8 +939,19 @@ static const struct i2c_device_id sht3x_ids[] = {
>  
>  MODULE_DEVICE_TABLE(i2c, sht3x_ids);
>  
> +static const struct of_device_id sht3x_of_match[] = {
> +	{ .compatible = "sensirion,sht30", .data = (void *)(uintptr_t)sht3x },
> +	{ .compatible = "sensirion,sts30", .data = (void *)(uintptr_t)sts3x },
> +	{ }
> +};
> +
> +MODULE_DEVICE_TABLE(of, sht3x_of_match);

While reviewing this patch, I noticed a few pre-existing regressions in this
file that might be worth addressing.

[Severity: Critical]
This is a pre-existing issue, but does this unaligned memory access crash
strict-alignment architectures?

drivers/hwmon/sht3x.c:sht3x_update_client() {
    ...
		val = be16_to_cpup((__be16 *)buf);
		data->temperature = sht3x_extract_temperature(val);
		val = be16_to_cpup((__be16 *)(buf + 3));
		data->humidity = sht3x_extract_humidity(val);
		data->last_update = jiffies;
	}
    ...
}

Since buf is a stack-allocated byte array, buf + 3 is at an unaligned odd
offset. Dereferencing this pointer triggers an alignment fault on strict
alignment architectures like ARMv5 or SPARC. 

Since these sysfs attributes are world-readable, could an unprivileged local
user repeatedly read them to trigger a denial of service?


[Severity: High]
This isn't a bug introduced by this patch, but does this integer arithmetic
overflow?

drivers/hwmon/sht3x.c:limit_write() {
    ...
	/*
	 * ST = (T + 45) / 175 * 2^16
	 * SRH = RH / 100 * 2^16
	 * adapted for fixed point arithmetic and packed the same as
	 * in limit_read()
	 */
	raw = ((u32)(temperature + 45000) * 24543) >> (16 + 7);
	raw |= ((humidity * 42950) >> 16) & 0xfe00;
    ...
}

Writing the maximum allowed temperature or humidity limits, such as 130000
for temp1_max, causes the expression ((u32)(temperature + 45000) * 24543)
to become 175000 * 24543 = 4295025000.

This overflows the 32-bit unsigned integer maximum. Does this result in a
calculated raw limit of 0, corresponding to the hardware's minimum bounds,
and incorrectly trigger a spurious hardware alert?


[Severity: High]
This is a pre-existing issue, but do these attributes violate the hwmon core
locking rules?

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

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

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

	data->repeatability = val;

	return count;
}

Attributes registered via the extra_groups parameter, such as heater_enable
and repeatability, do not have automatic hwmon core serialization.

The hwmon subsystem guidelines explicitly require drivers to implement
locking for extra attributes using hwmon_lock(). Should these functions use
hwmon_lock() to prevent concurrent state modifications?


[Severity: High]
This isn't a bug introduced by this patch, but does repeatability_store()
fail to update the active measurement command?

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

	data->repeatability = val;

	return count;
}

When a user writes to the repeatability attribute, this updates the internal
data->repeatability variable but fails to call sht3x_select_command() or
send the required I2C commands to the device.

Does the driver continue to use the old command and wait time for all
subsequent measurements in single-shot mode, rendering the change
ineffective?


[Severity: High]
This is a pre-existing issue, but does heater_enable_store() break sysfs
writes by returning the I2C transfer length?

drivers/hwmon/sht3x.c:heater_enable_store() {
    ...
	if (status)
		ret = i2c_master_send(client, (char *)&sht3x_cmd_heater_on,
				      SHT3X_CMD_LENGTH);
	else
		ret = i2c_master_send(client, (char *)&sht3x_cmd_heater_off,
				      SHT3X_CMD_LENGTH);

	mutex_unlock(&data->i2c_lock);

	return ret;
}

This returns the result of i2c_master_send(), which is 2 (the length of the
I2C command) instead of the consumed buffer length expected by sysfs.

If a user writes a string longer than 2 bytes, such as 'echo true', does
the sysfs core assume a partial write and invoke the store function again
with the remaining bytes, resulting in a spurious -EINVAL error?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713074559.12196-1-zaixiang.xu.dev@gmail.com?part=3