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

On Tue, Jul 28, 2026 at 11:13:26AM -0700, Guenter Roeck wrote:
> On 7/28/26 09:27, [email protected] wrote:
> > 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.
> > 
> 
> The maximum amount of data returned must be 32 bytes, not 34. Callers define
> a larger buffer size to be able to append, for example, '\n\0'. The function
> itself, however, must not return more than 32 bytes of data, plus the data
> length in the first byte.

Curious, do you want me to drop the comment or your reply is also about
giving AI context so it does not come up again with the comment?

- Nuno Sá

> 
> Guenter
> > [ ... ]
> > 
> > > 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?
> > 
>