Re: [PATCH v4 3/4] dt-bindings: hwmon: pmbus/tps25990: Add TPS1689

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] The driver incorrectly uses `i2c_get_clientdata()` to retrieve the `pmbus_driver_info` pointer, resulting in a type mismatch and out-of-bounds memory access.
--

commit 11131ea034ce953233470df60e4f6a1c34cca609
Author: Stoyan Bogdanov <sbogdanov@baylibre.com>

hwmon: (pmbus/tps25990): Rework TPS25990 direct conversion handling

Rework the existing implementation of direct format conversion for TPS25990
non-standard parameters to improve code reusability. It introduces structures
for per-device coefficients and uses generic PMBus conversion helpers.

> diff --git a/drivers/hwmon/pmbus/tps25990.c b/drivers/hwmon/pmbus/tps25990.c
> index 05c6288ecafcc..1e252844217b8 100644
> --- a/drivers/hwmon/pmbus/tps25990.c
> +++ b/drivers/hwmon/pmbus/tps25990.c

[ ... ]

> @@ -36,17 +36,63 @@
>  #define  TPS25990_UNLOCKED		BIT(7)
>  
>  #define TPS25990_8B_SHIFT		2

[ ... ]

> +struct tps25990_data {
> +	struct pmbus_driver_info info;
> +	struct tps25990_local_direct_value info_local;
> +};
> +
> +static s64 tps25990_reg2data_direct(struct i2c_client *client, int param, s32 raw)
> +{
> +	struct pmbus_driver_info *info = i2c_get_clientdata(client);
> +	struct tps25990_data *data = container_of(info, struct tps25990_data, info);

[Severity: Critical]
Does this code correctly retrieve the pmbus_driver_info pointer?

In pmbus_core.c, pmbus_do_probe() sets the I2C client data to a newly
allocated struct pmbus_data object, not struct pmbus_driver_info.

Because info is the first member of struct tps25990_data, container_of()
evaluates to the same pointer, causing the driver to mistakenly interpret the
pmbus_data object as a tps25990_data object.

When accessing the info_local member, it reads from an offset, yielding garbage
values for the calculation constants m, b, and R. If the garbage m value
happens to be zero, reading sensors via sysfs could cause a divide-by-zero
kernel panic in pmbus_reg2data_direct_calc().

Should this use pmbus_get_driver_info(client) instead of i2c_get_clientdata()?

> +	struct tps25990_local_direct_value *info_local = &data->info_local;
> +	s64 b, val;
> +	s32 m, R;
> +
> +	val = (s16)raw;
> +	m = info_local->m[param];
> +	b = info_local->b[param];
> +	R = info_local->R[param];
> +
> +	return pmbus_reg2data_direct_calc(val, b, m, R);
> +}
> +
> +static u16 tps25990_data2reg_direct(struct i2c_client *client, int param, s64 val)
> +{
> +	struct pmbus_driver_info *info = i2c_get_clientdata(client);
> +	struct tps25990_data *data = container_of(info, struct tps25990_data, info);

[Severity: Critical]
Does this face the same type mismatch regression?

Since the I2C client data points to a struct pmbus_data, using
i2c_get_clientdata() here will also misinterpret the object and result in
out-of-bounds memory access when reading info_local.

Could this be updated to use pmbus_get_driver_info(client) as well?

> +	struct tps25990_local_direct_value *info_local = &data->info_local;
> +	s32 m, R;
> +	s64 b;
> +
> +	m = info_local->m[param];
> +	b = info_local->b[param];
> +	R = info_local->R[param];
> +
> +	return pmbus_data2reg_direct_calc(val, b, m, R);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260522082349.2749970-1-sbogdanov@baylibre.com?part=3