Re: [PATCH 3/5] hwmon: (pmbus/core) Add mapping function to pmbus_read_block_data()

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

New issues:
- [High] Stack buffer overflow in `pmbus_debugfs_block_read` due to unbounded return value from custom `read_block_data` callback.

Pre-existing issues:
- [High] TOCTOU data race on PMBus hardware page selection during probe.
--

--- Patch [3]: [PATCH 3/5] hwmon: (pmbus/core) Add mapping function to pmbus_read_block_data() ---
commit c4bd4d03203a9fd14a7a2bcff69ec842b441fa68
Author: Nuno Sá <[email protected]>

hwmon: (pmbus/core) Add mapping function to pmbus_read_block_data()

This commit introduces a mapping function to pmbus_read_block_data() in
preparation for adding support to a device which needs to use its own
read_block implementation.

> diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
> index 3d5586c67f840..d697939ec8921 100644
> --- a/drivers/hwmon/pmbus/pmbus.h
> +++ b/drivers/hwmon/pmbus/pmbus.h
> @@ -459,6 +459,9 @@ struct pmbus_driver_info {
>  	int (*read_byte_data)(struct i2c_client *client, int page, int reg);
>  	int (*read_word_data)(struct i2c_client *client, int page, int phase,
>  			      int reg);
> +	/* size of data_buf is I2C_SMBUS_BLOCK_MAX + 2 */
> +	int (*read_block_data)(struct i2c_client *client, int page, u8 reg,
> +			       char *data_buf);

[Severity: High]
Does this API need a size limit parameter? The comment specifies the buffer
size is 34, without providing a limit parameter to the callback, which might
encourage callbacks to return values up to 34.

[ ... ]

> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index 7b58f71985741..ff4572c473b70 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
[ ... ]
> @@ -678,7 +698,7 @@ static bool __maybe_unused pmbus_check_block_register(struct i2c_client *client,
>  	struct pmbus_data *data = i2c_get_clientdata(client);
>  	char data_buf[I2C_SMBUS_BLOCK_MAX + 2];
>  
> -	rv = pmbus_read_block_data(client, page, reg, data_buf);
> +	rv = _pmbus_read_block_data(client, page, reg, data_buf);

[Severity: High]
This is a pre-existing issue, but can this lead to a race condition with
sysfs accesses during probe?

Looking at pmbus_init_debugfs(), it traverses pages and reads registers
without taking the pmbus_lock. This indirectly invokes pmbus_set_page()
through pmbus_check_block_register():

pmbus_init_debugfs()
  pmbus_check_block_register()
    _pmbus_read_block_data()
      pmbus_read_block_data()
        pmbus_set_page()

If userspace reads or writes to a sysfs attribute concurrently, it acquires
the lock and sets the hardware page. Could the lockless call from
pmbus_init_debugfs() interleave between the sysfs thread's pmbus_set_page()
and its subsequent I2C operation, causing the sysfs thread to hit the wrong
PMBus page?

>  	if (rv >= 0 && !(data->flags & PMBUS_SKIP_STATUS_CHECK))
>  		rv = pmbus_check_status_cml(client);
[ ... ]
> @@ -3564,7 +3584,7 @@ static ssize_t pmbus_debugfs_block_read(struct file *file, char __user *buf,
>  	char data[I2C_SMBUS_BLOCK_MAX + 2] = { 0 };
>  
>  	scoped_guard(pmbus_lock, client) {
> -		rc = pmbus_read_block_data(client, entry->page, entry->reg, data);
> +		rc = _pmbus_read_block_data(client, entry->page, entry->reg, data);
>  		if (rc < 0)
>  			return rc;
>  	}

[Severity: High]
Can the new callback return a value large enough to cause an out-of-bounds
write here?

If _pmbus_read_block_data() delegates to the new read_block_data callback,
and that callback returns a size of 34 (I2C_SMBUS_BLOCK_MAX + 2) or greater,
would it cause a buffer overflow?

Just below this hunk in pmbus_debugfs_block_read(), the code does:

    data[rc] = '\n';

Since the size of the data array is 34, an rc of 34 would write past the end
of the array. Is a bounds check needed on rc before using it as an index?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3