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