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

[Guenter Roeck <linux@roeck-us.net>]

On 8/4/26 02:05, Nuno Sá wrote:
> On Tue, Jul 28, 2026 at 11:13:26AM -0700, Guenter Roeck wrote:
>> On 7/28/26 09:27, sashiko-bot@kernel.org 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á <nuno.sa@analog.com>
>>>
>>> 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?
> 

Point was that the comment isn't accurate. The data buffer length must be at least
I2C_SMBUS_BLOCK_MAX, not I2C_SMBUS_BLOCK_MAX + 2, and the function must never return
more than I2C_SMBUS_BLOCK_MAX bytes of data (and the number of bytes as return value).

Having a comment is fine, only it must reflect the actual API.

Thanks,
Guenter