Re: [PATCH 6/7] hwmon: adm1275: Support ROHM BD12790

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The driver compares the device-returned PMBUS_MFR_MODEL string using strncasecmp without verifying the returned length, leading to uninitialized stack memory reads.
- [High] Multiplication of power and current coefficients by `shunt` overflows a 32-bit unsigned integer for larger shunt resistors.
--

commit 8dff4bf5659b589eabe7fc876665de9339faae1f
Author: Matti Vaittinen <mazziesaccount@gmail.com>

hwmon: adm1275: Support ROHM BD12790

Add support for ROHM BD12790 hot-swap controller which is largely
similar to Analog Devices adm1272.

> diff --git a/drivers/hwmon/pmbus/adm1275.c b/drivers/hwmon/pmbus/adm1275.c
> --- a/drivers/hwmon/pmbus/adm1275.c
> +++ b/drivers/hwmon/pmbus/adm1275.c
[ ... ]
> @@ -581,7 +606,8 @@ static int adm1275_probe(struct i2c_client *client)
>  	if (mid->driver_data == adm1272 || mid->driver_data == adm1273 ||
>  	    mid->driver_data == adm1278 || mid->driver_data == adm1281 ||
>  	    mid->driver_data == adm1293 || mid->driver_data == adm1294 ||
> -	    mid->driver_data == bd12780 || mid->driver_data == sq24905c)
> +	    mid->driver_data == bd12780 || mid->driver_data == bd12790 ||
> +	    mid->driver_data == sq24905c)
>  		config_read_fn = i2c_smbus_read_word_data;
>  	else

[Severity: High]
This is a pre-existing issue, but I noticed that just before this block in
adm1275_probe(), there might be an out-of-bounds stack read:

    ret = i2c_smbus_read_block_data(client, PMBUS_MFR_MODEL, block_buffer);
    if (ret < 0) { ... }
    for (mid = adm1275_id; mid->name[0]; mid++) {
            if (!strncasecmp(mid->name, block_buffer, strlen(mid->name)))
                    break;
    }

Does this code read uninitialized stack memory if the device returns a
string that matches a valid model name prefix, but is shorter than the valid
model name length?

Since i2c_smbus_read_block_data() reads up to 32 bytes into the uninitialized
stack array block_buffer without appending a null terminator, strncasecmp()
could read past the valid bytes returned in ret.

For example, if the device returns a shorter string like "adm12", checking
it against "adm1275" up to the length of "adm1275" will continue reading
into uninitialized stack bounds.

[ ... ]
> @@ -685,12 +722,23 @@ static int adm1275_probe(struct i2c_client *client)
>  		break;
>  	case adm1272:
>  	case adm1273:
> +	case bd12790:
> +	{
[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but looking further down in
adm1275_probe(), could the coefficient calculations for current and power
overflow a 32-bit unsigned integer?

    info->m[PSC_CURRENT_OUT] =
        coefficients[cindex].m * shunt / 1000;

    info->m[PSC_POWER] =
        coefficients[pindex].m * shunt / 1000;

If a larger shunt resistor value is provided via device tree
(e.g., 200,000 micro-ohms to measure lower currents), multiplying this by
larger coefficients like 30631 (used for ADM1293) evaluates to 6,126,200,000.

Since shunt is a u32 and the coefficient promotes to int, this operation uses
32-bit unsigned arithmetic. 

Does this wrap-around the maximum 32-bit value of 4,294,967,295 and result
in drastically incorrect telemetry scaling factors?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1781591132.git.mazziesaccount@gmail.com?part=6