Re: [PATCH] hwmon: (sg2042-mcu) reject short uptime reads

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Architectural and locking model violation: custom sysfs attributes are registered directly on the I2C device rather than through the hwmon core.
- [High] Arithmetic overflow when converting user input to degrees in `sg2042_mcu_write` allows bypassing maximum limits.
- [Medium] The I2C probe function checks for `I2C_FUNC_SMBUS_BLOCK_DATA` but the driver requires `I2C_FUNC_SMBUS_READ_I2C_BLOCK`.
--

--- Patch [1]: [PATCH] hwmon: (sg2042-mcu) reject short uptime reads ---
commit bcdb3b0b98b7a3e73302e366dc592d3472a7ff7b
Author: Ali Ahmet Memis <ali@iusegentoo.com>
hwmon: (sg2042-mcu) reject short uptime reads

This commit updates uptime_show() to ensure it receives exactly two bytes
when reading the uptime register. It rejects short reads to prevent
uninitialized stack memory from leaking into the sysfs output.

Cc: stable@vger.kernel.org

> diff --git a/drivers/hwmon/sg2042-mcu.c b/drivers/hwmon/sg2042-mcu.c
> index 591f5f572fe4c..10292417e7661 100644
> --- a/drivers/hwmon/sg2042-mcu.c
> +++ b/drivers/hwmon/sg2042-mcu.c
> @@ -79,6 +79,8 @@ static ssize_t uptime_show(struct device *dev,
>  					    sizeof(time_val), time_val);

[Severity: Medium]
This is a pre-existing issue, but since this function uses
i2c_smbus_read_i2c_block_data(), could the adapter functionality check in
sg2042_mcu_i2c_probe() be incorrect?

drivers/hwmon/sg2042-mcu.c:sg2042_mcu_i2c_probe() {
    ...
    if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_BYTE_DATA |
                                                  I2C_FUNC_SMBUS_BLOCK_DATA))
        return -ENODEV;
    ...
}

It checks for I2C_FUNC_SMBUS_BLOCK_DATA instead of
I2C_FUNC_SMBUS_READ_I2C_BLOCK.

Does this allow the driver to probe on adapters that support I2C blocks but not
SMBus blocks, or fail to probe on adapters lacking SMBus block support?

>  	if (ret < 0)
>  		return ret;
> +	if (ret != sizeof(time_val))
> +		return -EIO;
>  
>  	return sprintf(buf, "%d\n",
>  		       (time_val[0]) | (time_val[1] << 8));

[Severity: High]
This is a pre-existing issue, but does sg2042_mcu_write() have an arithmetic
overflow regression when scaling user input?

drivers/hwmon/sg2042-mcu.c:sg2042_mcu_write() {
    ...
    int temp = val / 1000;
    int hyst_temp, crit_temp;
    u8 reg;

    temp = clamp_val(temp, 0, MCU_POWER_MAX);
    ...
}

If a user writes a sufficiently large 64-bit integer (e.g., > 2.14 * 10^12) to
a threshold file, does the division result in a value that overflows the 32-bit
signed int temp?

If it overflows into a negative number, wouldn't clamp_val()
evaluate the negative integer against 0 and clamp it to 0 instead of the maximum
limit, incorrectly bypassing hardware safety limits?

[Severity: High]
This is a pre-existing issue, but are custom sysfs attributes registered
directly on the I2C device rather than through the hwmon core?

drivers/hwmon/sg2042-mcu.c:
    static const struct attribute_group *sg2042_mcu_groups[] = {
        &sg2042_mcu_attr_group,
        NULL
    };

    static struct i2c_driver sg2042_mcu_driver = {
        .driver = {
            ...
            .dev_groups = sg2042_mcu_groups,
        },
        ...
    };

By registering these directly on the I2C device, does this regression bypass
the hwmon subsystem layering?

Additionally, since these attributes only have access to the I2C device pointer,
does this make it structurally impossible for them to use hwmon_lock() for
serialization, violating the subsystem locking guidelines?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260802130728.25721-1-ali@iusegentoo.com?part=1