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

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

[Ali Ahmet Memis]

uptime_show() asks for two bytes and only rejects a negative return:

	u8 time_val[2];

	ret = i2c_smbus_read_i2c_block_data(mcu->client, REG_UPTIME,
					    sizeof(time_val), time_val);
	if (ret < 0)
		return ret;

	return sprintf(buf, "%d\n",
		       (time_val[0]) | (time_val[1] << 8));

i2c_smbus_read_i2c_block_data() returns the number of bytes the transfer
actually produced, which the device supplies and which can be shorter
than the length asked for:

	memcpy(values, &data.block[1], data.block[0]);
	return data.block[0];

time_val is not initialised, so a reply of one byte leaves the high half
of the reported uptime as whatever was on the stack, and a reply of zero
bytes leaks both halves. Either way the value ends up in sysfs.

Require the full two bytes. The other registers this driver reads go
through i2c_smbus_read_byte_data(), which returns the byte itself, so
the existing negative-only checks are right there.

Fixes: 758b62e562f2 ("hwmon: Add sophgo SG2042 external hardware monitor support")
Cc: stable@vger.kernel.org
Signed-off-by: Ali Ahmet Memis <ali@iusegentoo.com>
---
 drivers/hwmon/sg2042-mcu.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/hwmon/sg2042-mcu.c b/drivers/hwmon/sg2042-mcu.c
index 591f5f572fe4..10292417e766 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);
 	if (ret < 0)
 		return ret;
+	if (ret != sizeof(time_val))
+		return -EIO;
 
 	return sprintf(buf, "%d\n",
 		       (time_val[0]) | (time_val[1] << 8));

base-commit: 2d2338c93da79b3bfe4b6099a931d9468d539952
-- 
2.55.0

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

[sashiko-bot]

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

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

[Guenter Roeck]

On 8/2/26 06:24, sashiko-bot@kernel.org wrote:
> 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.

Ok because the atributes are not hwmon attributes but associated with the MCU.

> - [High] Arithmetic overflow when converting user input to degrees in `sg2042_mcu_write` allows bypassing maximum limits.

Correct
> - [Medium] The I2C probe function checks for `I2C_FUNC_SMBUS_BLOCK_DATA` but the driver requires `I2C_FUNC_SMBUS_READ_I2C_BLOCK`.

Correct, and common issue.

Guenter

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

[Guenter Roeck]

On 8/2/26 06:07, Ali Ahmet Memis wrote:
> uptime_show() asks for two bytes and only rejects a negative return:
> 
> 	u8 time_val[2];
> 
> 	ret = i2c_smbus_read_i2c_block_data(mcu->client, REG_UPTIME,
> 					    sizeof(time_val), time_val);
> 	if (ret < 0)
> 		return ret;
> 
> 	return sprintf(buf, "%d\n",
> 		       (time_val[0]) | (time_val[1] << 8));
> 
> i2c_smbus_read_i2c_block_data() returns the number of bytes the transfer
> actually produced, which the device supplies and which can be shorter
> than the length asked for:
> 
> 	memcpy(values, &data.block[1], data.block[0]);
> 	return data.block[0];
> 
> time_val is not initialised, so a reply of one byte leaves the high half
> of the reported uptime as whatever was on the stack, and a reply of zero
> bytes leaks both halves. Either way the value ends up in sysfs.
> 
> Require the full two bytes. The other registers this driver reads go
> through i2c_smbus_read_byte_data(), which returns the byte itself, so
> the existing negative-only checks are right there.
> 

I ended up asking AI:

Does i2c_smbus_read_i2c_block_data() in ./drivers/i2c/i2c-core-smbus.c ever return less than the requested number of bytes unless there is an error ?

Here is what it tells me:

   No, as long as the requested length is <= I2C_SMBUS_BLOCK_MAX (32 bytes), i2c-core-smbus.c will never return fewer
bytes than requested unless a negative error code is returned.

   The only scenario where it returns a positive byte count less than the caller's requested length is if the caller
passes length > 32, because the function clamps the request to I2C_SMBUS_BLOCK_MAX.

   ### Why this is the case

   1. No Count Byte in I2C Block Read (I2C_SMBUS_I2C_BLOCK_DATA):
   Unlike an SMBus Block Read (smbus-protocol.rst), where the slave device sends a length/count byte as the first byte of
  data transferred, an I2C Block Read does not include a length byte from the target
   device. The master reads a fixed number of bytes determined by the caller.
   2. In i2c-core-smbus.c:
     if (length > I2C_SMBUS_BLOCK_MAX)
             length = I2C_SMBUS_BLOCK_MAX;
     data.block[0] = length;
     status = i2c_smbus_xfer(client->adapter, client->addr, client->flags,
                             I2C_SMBUS_READ, command,
                             I2C_SMBUS_I2C_BLOCK_DATA, &data);
     if (status < 0)
             return status;

     memcpy(values, &data.block[1], data.block[0]);
     return data.block[0];
     Before calling i2c_smbus_xfer(), data.block[0] is initialized to the requested length (clamped to 32). Upon success
(status >= 0), it returns data.block[0].
   3. In I2C Emulation (i2c-core-smbus.c):
       • The read message is set up for exactly data->block[0] bytes (msg[1].len = data->block[0];).
       • Unlike SMBus Block Data, it does not set I2C_M_RECV_LEN.
       • When copying the buffer back upon completion (i2c-core-smbus.c), data->block[0] is never modified and remains
exactly equal to the requested length.
   4. In Native Adapter Drivers (adapter->algo->smbus_xfer):
       • None of the bus drivers in drivers/i2c/busses/ implementing I2C_SMBUS_I2C_BLOCK_DATA (i2c-i801, i2c-viapro,
i2c-amd8111, i2c-ismt, i2c-mlxbf, etc.) modify data->block[0] to a smaller value on success.
       • If a target device NAKs before the requested length bytes are read, the controller driver fails the transaction
and returns a negative errno (e.g., -ENXIO, -EIO, or -EPROTO).

   Consequently, any successful call such as i2c_smbus_read_i2c_block_data(client, reg, I2C_SMBUS_BLOCK_MAX, buf) will
always return exactly 32 (I2C_SMBUS_BLOCK_MAX) on success.

Please refrain from sending fixes for non-issues.

Thanks,
Guenter

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

[Ali Ahmet Memis]

On Sun, Aug 02 2026, Guenter Roeck wrote:

> Please refrain from sending fixes for non-issues.

You are right. Please drop it.

I checked what i2c_smbus_read_i2c_block_data() returns without checking
which protocol it runs, and the two block protocols do not behave the
same. In i2c_smbus_xfer_emulated(), I2C_SMBUS_BLOCK_DATA sets
I2C_M_RECV_LEN and reads back with

	memcpy(data->block, msg[1].buf, msg[1].buf[0] + 1);

so there the device supplies the count and a short reply is a real
thing. I2C_SMBUS_I2C_BLOCK_DATA, which is what this call uses, sets no
I2C_M_RECV_LEN, sends msg[1].len = data->block[0] and reads back with

	memcpy(data->block + 1, msg[1].buf, data->block[0]);

which never touches data->block[0]. An I2C block read carries no length
byte on the wire, so nothing can shorten it, and a transfer that is cut
short fails with an errno rather than returning a smaller count. time_val
is fully written whenever the call succeeds.

I should have read that before sending, rather than reasoning from the
return value alone. Sorry for the noise, and for the stable Cc on top of
it.

--
Ali