Re: [PATCH 4/4] hwmon: (ina2xx) Return 0 on inactive alarm/limit reads

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Writing `0` to an inactive alert limit attribute unintentionally disables the currently active alert limit of a completely different type.
- [High] In `ina2xx_get_value`, calculating `INA2XX_POWER` can overflow a 32-bit signed `int` for small shunt resistor values, resulting in negative power readings.
- [High] In `ina226_alert_to_reg`, `INA2XX_SHUNT_VOLTAGE` limits can overflow a 32-bit signed `long`, causing large limits to be incorrectly clamped to 0 on 32-bit systems.
- [High] In `ina226_alert_to_reg`, `INA2XX_POWER` limits always evaluate to 0 on 32-bit systems because `UINT_MAX` is cast to a negative `long` in the clamping macro.
- [Medium] The `shunt_resistor_show` function accesses shared driver state locklessly, violating HWMON subsystem locking guidelines.
--

--- Patch [4]: [PATCH 4/4] hwmon: (ina2xx) Return 0 on inactive alarm/limit reads ---
commit 4e821bd1003ecc2b04b0b58f9702178e2bd3a2eb
Author: Jared Kangas <jkangas@redhat.com>

hwmon: (ina2xx) Return 0 on inactive alarm/limit reads

Guard alarm/limit reads with a check that returns zero if the active
alarm is for a different type to avoid unintentionally clearing alarms.

> diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
> index 8ba0dce1f9e31..5521490deba98 100644
> --- a/drivers/hwmon/ina2xx.c
> +++ b/drivers/hwmon/ina2xx.c
> @@ -479,6 +479,12 @@ static int ina226_alert_limit_read(struct ina2xx_data *data, enum ina2xx_alert_t
>  	u32 mask;
>  	int ret;
>  
> +	/* Avoid nonzero reads from inactive alerts caused by shared limit register */
> +	if (data->active_alert != alert) {
> +		*val = 0;
> +		return 0;
> +	}
> +

[Severity: High]
This isn't a bug introduced by this patch, but since we are fixing the
read path for inactive alerts here, what happens on the write path?

If a user or monitoring daemon reads an inactive alert attribute (which
now correctly returns 0) and subsequently writes 0 back to it, will
ina226_alert_limit_write() unconditionally wipe the active alert?

Looking at ina226_alert_limit_write():

	ret = regmap_update_bits(regmap, INA226_MASK_ENABLE,
				 INA226_ALERT_CONFIG_MASK, 0);
	...
	data->active_alert = val ? alert : INA2XX_ALERT_NONE;

Does this inadvertently destroy the configuration of the currently active
alert of a different type without checking if the disable operation applies
to it?

[ ... ]

[Severity: High]
This is a pre-existing issue, but I noticed a potential overflow when
calculating power values.

In ina2xx_get_value() for INA2XX_POWER:

	case INA2XX_POWER:
		val = regval * data->power_lsb_uW;
		break;

If a user configures a small shunt resistor (e.g., 1 mOhm), power_lsb_uW
becomes large. When the hardware measures high power, regval can also be
quite large. 

Since val is a 32-bit signed integer, could this multiplication overflow
on 32-bit architectures and result in incorrect negative power readings
being reported to userspace?

[Severity: High]
This is a pre-existing issue, but could we run into an overflow on 32-bit
architectures when setting shunt voltage limits?

In ina226_alert_to_reg():

	case INA2XX_SHUNT_VOLTAGE:
		val = clamp_val(val, 0, SHRT_MAX * data->config->shunt_div);
		val *= data->config->shunt_div;

On a 32-bit system, long is 32 bits. If a user inputs a large limit, and
it passes the first clamp, the multiplication by shunt_div could exceed
the maximum value of a signed 32-bit integer.

Does this overflow the signed long, wrapping to a negative value that
then gets incorrectly clamped to 0 by the final clamp_val()?

[Severity: High]
This is a pre-existing issue, but setting power limits on 32-bit systems
seems to always evaluate to 0.

In ina226_alert_to_reg():

	case INA2XX_POWER:
		val = clamp_val(val, 0, UINT_MAX - data->power_lsb_uW);

The clamp_val macro casts bounds to typeof(val) which is a signed long. 

On 32-bit systems, does UINT_MAX - data->power_lsb_uW evaluate to a
large positive unsigned value that becomes negative when cast to a
signed long?

If the upper bound is negative, does this force clamp_val to rewrite all
positive user inputs to negative values, guaranteeing the function always
returns 0?

[Severity: Medium]
This is a pre-existing issue, but there appears to be a locking violation
when reading the shunt resistor.

The hwmon subsystem guidelines require drivers to use hwmon_lock() for
attributes registered outside the core info parameter. 

Looking at shunt_resistor_show():

static ssize_t shunt_resistor_show(struct device *dev,
				   struct device_attribute *da, char *buf)
{
	struct ina2xx_data *data = dev_get_drvdata(dev);

	return sysfs_emit(buf, "%li\n", data->rshunt);
}

Since shunt_resistor_store() modifies data->rshunt, does this show
function need to acquire the hwmon_lock to prevent unprotected concurrent
accesses?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260729-upstream-ina2xx-in0-curr1-alarms-v1-0-349f7b2f1df8@redhat.com?part=4