linux-kernel - Re: [PATCH v5 0/8] hwmon: (ina3221) Various improvement and add support for SQ52210

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <956b92bd-950b-44fc-af85-6f76ed60656f@roeck-us.net>
Date: Tue, 27 Jan 2026 10:07:48 -0800
From: Guenter Roeck <linux@...ck-us.net>
To: Wenliang Yan <wenliang202407@....com>
Cc: Jean Delvare <jdelvare@...e.com>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Jonathan Corbet <corbet@....net>, linux-hwmon@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 0/8] hwmon: (ina3221) Various improvement and add
 support for SQ52210

Hi,

On Mon, Jan 19, 2026 at 07:14:38AM -0500, Wenliang Yan wrote:
> Make modifications according to the guidance provided in the reply.
> 
> 1.Modify the description for power[123]_input(PATCH 5).
> 
> 2.Re-annotate the significance of the limit value calculation
> and the use of register masks in the sq52210_alert_limit_write.
> Modify the calculation process to resolve arithmetic overflow issues.
> The limit values SOL, BOL, and BUL are all stored using the upper
> 13 bits of the register, so shifting is required. In contrast,
> the POL value is configured by setting the lower three bits
> to 0 directly(PATCH 6).
> 
> 3.Resolve arithmetic overflow issues in the ina3221_read_power(PATCH 7).
> 
> 4.Resolve arithmetic overflow issues in the ina3221_read_curr,
> and validate channel indices in ina3221_write_in(PATCH 8).
> 

AI review of the series provided the feedback below. Please fix or explain
false positives.

Thanks,
Guenter

---
Analysis of Top 8 Commits in drivers/hwmon/ina3221.c

Commit: a0f07f4a272b hwmon: (ina3221) Modify write/read functions for 'in' and 'curr' attribute
--------------------------------------------------------------------------------------------
Change Categories:
- CHANGE-1: Added read/write support for inX_crit/lcrit/alarm
- CHANGE-2: Added read/write support for currX_lcrit/alarm
- CHANGE-3: Refactored ina3221_write_curr

Regressions/Bugs:
1. MAJOR BUG: In `ina3221_write_curr`, `hwmon_curr_lcrit` passes current value (in mA) directly to `sq52210_alert_limit_write`. `sq52210_alert_limit_write` treats the value as Shunt Voltage (in uV) for `SQ52210_ALERT_SUL` (divides by 5). This fails to account for the shunt resistor value. The alert limit will be incorrect for any shunt resistor other than 1 mOhm.
   Evidence:
   `ina3221_write_curr`: `return sq52210_alert_limit_write(ina, SQ52210_ALERT_SUL, channel, val);`
   `sq52210_alert_limit_write`: `regval = DIV_ROUND_CLOSEST(val, 5) & 0xfff8;`

Commit: ef198f703062 hwmon: (ina3221) Support write/read functions for 'power' attribute
----------------------------------------------------------------------------------------
Change Categories:
- CHANGE-1: Added read/write support for powerX attributes
- CHANGE-2: Updated ina3221_is_visible

Regressions/Bugs:
1. BUG: `ina3221_is_visible` incorrectly enables `hwmon_power_crit` and `hwmon_power_crit_alarm` based solely on `has_alerts`. Standard INA3221 devices have `has_alerts` set to true but do not support power measurement (`has_power` is false). This exposes unsupported power attributes to userspace for INA3221.
   Evidence:
   ```c
   case hwmon_power_crit:
       if (has_alerts)
           return 0644;
       return 0;
   ```

Commit: 61e453283222 hwmon: (ina3221) Support for writing alert limit values and modify the 'ina3221_read_value' function
-------------------------------------------------------------------------------------------------------------------------
Change Categories:
- CHANGE-1: Refactored `ina3221_read_value` to handle SQ52210 registers
- CHANGE-2: Added `sq52210_alert_limit_write`

Regressions/Bugs:
1. CRITICAL BUG: `ina3221_read_value` leaves `*val` uninitialized when reading `SQ52210_ALERT_LIMITx` registers if `ina->alert_type_select` is 0 (default). `ina3221_probe` initializes `alert_type_select` to 0 but does not sync it with the device's `SQ52210_ALERT_CONFIG` register. Reading these attributes before writing them results in returning garbage stack data to userspace.
   Evidence:
   ```c
   switch (reg) {
   ...
   case SQ52210_ALERT_LIMIT3:
       if (ina->alert_type_select & SQ52210_ALERT_ALL_SUL_MASK)
           *val = ...
       else if (...)
           *val = ...
       break;
   }
   return 0;
   ```
   If no mask matches, `*val` is never assigned.

Summary:
Multiple significant regressions found in the series. The logic for SUL (Shunt Under Limit) handling for Current attributes is fundamentally broken regarding unit conversion (mA vs uV) and shunt resistance. Visibility checks for power attributes are too permissive for legacy devices. Uninitialized variable usage in read path poses a stability/correctness risk.