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

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 co=
re.
- [High] Arithmetic overflow when converting user input to degrees in `sg20=
42_mcu_write` allows bypassing maximum limits.
- [Medium] The I2C probe function checks for `I2C_FUNC_SMBUS_BLOCK_DATA` bu=
t 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 <[email protected]>
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: [email protected]

> 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 !=3D sizeof(time_val))
> +		return -EIO;
> =20
>  	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 =3D val / 1000;
    int hyst_temp, crit_temp;
    u8 reg;

    temp =3D 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 ma=
ximum
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[] =3D {
        &sg2042_mcu_attr_group,
        NULL
    };

    static struct i2c_driver sg2042_mcu_driver =3D {
        .driver =3D {
            ...
            .dev_groups =3D 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 poi=
nter,
does this make it structurally impossible for them to use hwmon_lock() for
serialization, violating the subsystem locking guidelines?

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260802130728.2572=
[email protected]?part=3D1