[PATCH v2 0/3] hwmon: Fix bugs in pt5161l, isl28022, and powerz

[PATCH v2 0/3] hwmon: Fix bugs in pt5161l, isl28022, and powerz

[Pradhan, Sanman]

From: Sanman Pradhan <psanman@juniper.net>

This series fixes bugs in drivers/hwmon/

1. pt5161l: Undersized stack buffer for SMBus block read, and
   unexpected positive return when retries are exhausted
2. isl28022: Integer overflow in power calculation on 32-bit
3. powerz: Use-after-free race on USB disconnect, and signal
   handling bug in completion wait

Changes since v1:
- Patch 1/3 (pt5161l): Also fix the case where
  i2c_smbus_read_block_data() returns a positive byte count on
  length mismatch after all retries are exhausted. Callers treated
  this as success. Return -EIO instead.
- Patch 2/3 (isl28022): Switch from s64/div_s64() to u64/div_u64()
  since power is inherently non-negative. This avoids implicit
  narrowing of the u32 shunt divisor to s32.
- Patch 3/3 (powerz): Also fix signal handling in
  wait_for_completion_interruptible_timeout(). The original code
  only handles timeout (ret==0) but not signals (ret==-ERESTARTSYS),
  which skips usb_kill_urb() and falls through to access stale URB
  data. Return -ENODEV instead of -EIO on disconnected device for
  clearer semantics.

Sanman Pradhan (3):
  hwmon: (pt5161l) Fix bugs in pt5161l_read_block_data()
  hwmon: (isl28022) Fix integer overflow in power calculation on 32-bit
  hwmon: (powerz) Fix use-after-free and signal handling on USB
    disconnect

 drivers/hwmon/isl28022.c |  7 +++++--
 drivers/hwmon/powerz.c   | 18 +++++++++++++-----
 drivers/hwmon/pt5161l.c  |  4 ++--
 3 files changed, 20 insertions(+), 9 deletions(-)

-- 
2.34.1

[PATCH v2 1/3] hwmon: (pt5161l) Fix bugs in pt5161l_read_block_data()

[Pradhan, Sanman]

From: Sanman Pradhan <psanman@juniper.net>

Fix two bugs in pt5161l_read_block_data():

1. Buffer overrun: The local buffer rbuf is declared as u8 rbuf[24],
   but i2c_smbus_read_block_data() can return up to
   I2C_SMBUS_BLOCK_MAX (32) bytes. The i2c-core copies the data into
   the caller's buffer before the return value can be checked, so
   the post-read length validation does not prevent a stack overrun
   if a device returns more than 24 bytes. Resize the buffer to
   I2C_SMBUS_BLOCK_MAX.

2. Unexpected positive return on length mismatch: When all three
   retries are exhausted because the device returns data with an
   unexpected length, i2c_smbus_read_block_data() returns a positive
   byte count. The function returns this directly, and callers treat
   any non-negative return as success, processing stale or incomplete
   buffer contents. Return -EIO when retries are exhausted with a
   positive return value, preserving the negative error code on I2C
   failure.

Fixes: 1b2ca93cd0592 ("hwmon: Add driver for Astera Labs PT5161L retimer")
Cc: stable@vger.kernel.org
Signed-off-by: Sanman Pradhan <psanman@juniper.net>
---
v2:
 - Also fix unexpected positive return when retries are exhausted
   due to length mismatch

 drivers/hwmon/pt5161l.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/pt5161l.c b/drivers/hwmon/pt5161l.c
index 20e3cfa625f1..89d4da8aa4c0 100644
--- a/drivers/hwmon/pt5161l.c
+++ b/drivers/hwmon/pt5161l.c
@@ -121,7 +121,7 @@ static int pt5161l_read_block_data(struct pt5161l_data *data, u32 address,
 	int ret, tries;
 	u8 remain_len = len;
 	u8 curr_len;
-	u8 wbuf[16], rbuf[24];
+	u8 wbuf[16], rbuf[I2C_SMBUS_BLOCK_MAX];
 	u8 cmd = 0x08; /* [7]:pec_en, [4:2]:func, [1]:start, [0]:end */
 	u8 config = 0x00; /* [6]:cfg_type, [4:1]:burst_len, [0]:address bit16 */
 
@@ -151,7 +151,7 @@ static int pt5161l_read_block_data(struct pt5161l_data *data, u32 address,
 				break;
 		}
 		if (tries >= 3)
-			return ret;
+			return ret < 0 ? ret : -EIO;
 
 		memcpy(val, rbuf, curr_len);
 		val += curr_len;
-- 
2.34.1

[PATCH v2 2/3] hwmon: (isl28022) Fix integer overflow in power calculation on 32-bit

[Pradhan, Sanman]

From: Sanman Pradhan <psanman@juniper.net>

isl28022_read_power() computes:

  *val = ((51200000L * ((long)data->gain)) /
          (long)data->shunt) * (long)regval;

On 32-bit platforms, 'long' is 32 bits. With gain=8 and shunt=10000
(the default configuration):

  (51200000 * 8) / 10000 = 40960
  40960 * 65535 = 2,684,313,600

This exceeds LONG_MAX (2,147,483,647), resulting in signed integer
overflow.

Additionally, dividing before multiplying by regval loses precision
unnecessarily.

Use u64 intermediates with div_u64() and multiply before dividing
to retain precision. Power is inherently non-negative, so unsigned
types are the natural fit. Clamp the result to LONG_MAX before
returning it through the hwmon callback, following the pattern used
by ina238.

Fixes: 39671a14df4f2 ("hwmon: (isl28022) new driver for ISL28022 power monitor")
Cc: stable@vger.kernel.org
Signed-off-by: Sanman Pradhan <psanman@juniper.net>
---
v2:
 - Switch from s64/div_s64() to u64/div_u64() since power is
   inherently non-negative, avoiding implicit u32-to-s32 narrowing
   of the shunt divisor

 drivers/hwmon/isl28022.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/isl28022.c b/drivers/hwmon/isl28022.c
index c2e559dde63f..d233a7b3f327 100644
--- a/drivers/hwmon/isl28022.c
+++ b/drivers/hwmon/isl28022.c
@@ -9,6 +9,7 @@
 #include <linux/err.h>
 #include <linux/hwmon.h>
 #include <linux/i2c.h>
+#include <linux/math64.h>
 #include <linux/module.h>
 #include <linux/regmap.h>
 
@@ -178,6 +179,7 @@ static int isl28022_read_power(struct device *dev, u32 attr, long *val)
 	struct isl28022_data *data = dev_get_drvdata(dev);
 	unsigned int regval;
 	int err;
+	u64 tmp;
 
 	switch (attr) {
 	case hwmon_power_input:
@@ -185,8 +187,9 @@ static int isl28022_read_power(struct device *dev, u32 attr, long *val)
 				  ISL28022_REG_POWER, &regval);
 		if (err < 0)
 			return err;
-		*val = ((51200000L * ((long)data->gain)) /
-			(long)data->shunt) * (long)regval;
+		tmp = (u64)51200000 * data->gain * regval;
+		tmp = div_u64(tmp, data->shunt);
+		*val = clamp_val(tmp, 0, LONG_MAX);
 		break;
 	default:
 		return -EOPNOTSUPP;
-- 
2.34.1

Re: [PATCH v2 2/3] hwmon: (isl28022) Fix integer overflow in power calculation on 32-bit

[David Laight]

On Tue, 7 Apr 2026 17:38:01 +0000
"Pradhan, Sanman" <sanman.pradhan@hpe.com> wrote:

> From: Sanman Pradhan <psanman@juniper.net>
> 
> isl28022_read_power() computes:
> 
>   *val = ((51200000L * ((long)data->gain)) /
>           (long)data->shunt) * (long)regval;
> 
> On 32-bit platforms, 'long' is 32 bits. With gain=8 and shunt=10000
> (the default configuration):
> 
>   (51200000 * 8) / 10000 = 40960
>   40960 * 65535 = 2,684,313,600
> 
> This exceeds LONG_MAX (2,147,483,647), resulting in signed integer
> overflow.
> 
> Additionally, dividing before multiplying by regval loses precision
> unnecessarily.
> 
> Use u64 intermediates with div_u64() and multiply before dividing
> to retain precision. Power is inherently non-negative, so unsigned
> types are the natural fit. Clamp the result to LONG_MAX before
> returning it through the hwmon callback, following the pattern used
> by ina238.
> 
> Fixes: 39671a14df4f2 ("hwmon: (isl28022) new driver for ISL28022 power monitor")
> Cc: stable@vger.kernel.org
> Signed-off-by: Sanman Pradhan <psanman@juniper.net>
> ---
> v2:
>  - Switch from s64/div_s64() to u64/div_u64() since power is
>    inherently non-negative, avoiding implicit u32-to-s32 narrowing
>    of the shunt divisor

There is no such thing as u32-to-s32 narrowing.
Basically nothing happens to the bit pattern.
But the values are almost certainly never negative.

> 
>  drivers/hwmon/isl28022.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hwmon/isl28022.c b/drivers/hwmon/isl28022.c
> index c2e559dde63f..d233a7b3f327 100644
> --- a/drivers/hwmon/isl28022.c
> +++ b/drivers/hwmon/isl28022.c
> @@ -9,6 +9,7 @@
>  #include <linux/err.h>
>  #include <linux/hwmon.h>
>  #include <linux/i2c.h>
> +#include <linux/math64.h>
>  #include <linux/module.h>
>  #include <linux/regmap.h>
>  
> @@ -178,6 +179,7 @@ static int isl28022_read_power(struct device *dev, u32 attr, long *val)
>  	struct isl28022_data *data = dev_get_drvdata(dev);
>  	unsigned int regval;
>  	int err;
> +	u64 tmp;
>  
>  	switch (attr) {
>  	case hwmon_power_input:
> @@ -185,8 +187,9 @@ static int isl28022_read_power(struct device *dev, u32 attr, long *val)
>  				  ISL28022_REG_POWER, &regval);
>  		if (err < 0)
>  			return err;
> -		*val = ((51200000L * ((long)data->gain)) /
> -			(long)data->shunt) * (long)regval;
> +		tmp = (u64)51200000 * data->gain * regval;
> +		tmp = div_u64(tmp, data->shunt);
> +		*val = clamp_val(tmp, 0, LONG_MAX);

Don't use clamp_val(), you don't need to and it is completely
broken by design.
Just use min().
You could just write:
	*val = min(div_u64(51200000ULL * data->gain * regval, data->shunt), LONG_MAX);

Have you checked that the multiply can't overflow 64bits?
That might be why the last multiply was done after the divide.

	David   


>  		break;
>  	default:
>  		return -EOPNOTSUPP;

Re: [PATCH v2 2/3] hwmon: (isl28022) Fix integer overflow in power calculation on 32-bit

[Pradhan, Sanman]

From: Sanman Pradhan <psanman@juniper.net>

Thanks for the review.

Yes, I checked this.

In this driver, gain is limited to {1, 2, 4, 8} by
isl28022_read_properties(), and regval is a 16-bit register value
(max 65535). The worst-case numerator is:

  51200000 * 8 * 65535 = 26843136000000

which is well below U64_MAX, so the multiply cannot overflow.

I'll switch to min_t(u64, ...) here to make the type handling explicit
for the u64 result of div_u64(), if that's ok, and send a v3.

Thank you.

Regards,
Sanman Pradhan

[PATCH v2 3/3] hwmon: (powerz) Fix use-after-free and signal handling on USB disconnect

[Pradhan, Sanman]

From: Sanman Pradhan <psanman@juniper.net>

Fix several issues in the powerz driver related to USB disconnect
races and signal handling:

1. Use-after-free: When hwmon sysfs attributes are read concurrently
   with USB disconnect, powerz_read() obtains a pointer to the
   private data via usb_get_intfdata(), then powerz_disconnect() runs
   and frees the URB while powerz_read_data() may still reference it.
   Fix by:
   - Moving usb_set_intfdata() before hwmon registration so the
     disconnect handler can always find the priv pointer.
   - Clearing intfdata and NULLing the URB pointer in disconnect
     under the mutex.
   - Guarding powerz_read_data() with a !priv->urb check.

2. Signal handling: wait_for_completion_interruptible_timeout()
   returns -ERESTARTSYS on signal, 0 on timeout, or positive on
   completion. The original code tests !ret, which only catches
   timeout. On signal delivery (-ERESTARTSYS), !ret is false so the
   function skips usb_kill_urb() and falls through, accessing
   potentially stale URB data. Capture the return value and handle
   both the signal (negative) and timeout (zero) cases explicitly.

Fixes: 4381a36abdf1c ("hwmon: add POWER-Z driver")
Cc: stable@vger.kernel.org
Signed-off-by: Sanman Pradhan <psanman@juniper.net>
---
v2:
 - Also fix signal handling in wait_for_completion_interruptible_timeout();
   original code only handled timeout (ret==0) but not signals
   (ret==-ERESTARTSYS)
 - Return -ENODEV instead of -EIO for disconnected device

 drivers/hwmon/powerz.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/hwmon/powerz.c b/drivers/hwmon/powerz.c
index 4e663d5b4e33..0b38fdf42ddb 100644
--- a/drivers/hwmon/powerz.c
+++ b/drivers/hwmon/powerz.c
@@ -108,6 +108,9 @@ static int powerz_read_data(struct usb_device *udev, struct powerz_priv *priv)
 {
 	int ret;
 
+	if (!priv->urb)
+		return -ENODEV;
+
 	priv->status = -ETIMEDOUT;
 	reinit_completion(&priv->completion);
 
@@ -124,10 +127,11 @@ static int powerz_read_data(struct usb_device *udev, struct powerz_priv *priv)
 	if (ret)
 		return ret;
 
-	if (!wait_for_completion_interruptible_timeout
-	    (&priv->completion, msecs_to_jiffies(5))) {
+	ret = wait_for_completion_interruptible_timeout(&priv->completion,
+							msecs_to_jiffies(5));
+	if (ret <= 0) {
 		usb_kill_urb(priv->urb);
-		return -EIO;
+		return ret ? ret : -EIO;
 	}
 
 	if (priv->urb->actual_length < sizeof(struct powerz_sensor_data))
@@ -224,16 +228,17 @@ static int powerz_probe(struct usb_interface *intf,
 	mutex_init(&priv->mutex);
 	init_completion(&priv->completion);
 
+	usb_set_intfdata(intf, priv);
+
 	hwmon_dev =
 	    devm_hwmon_device_register_with_info(parent, DRIVER_NAME, priv,
 						 &powerz_chip_info, NULL);
 	if (IS_ERR(hwmon_dev)) {
+		usb_set_intfdata(intf, NULL);
 		usb_free_urb(priv->urb);
 		return PTR_ERR(hwmon_dev);
 	}
 
-	usb_set_intfdata(intf, priv);
-
 	return 0;
 }
 
@@ -241,9 +246,12 @@ static void powerz_disconnect(struct usb_interface *intf)
 {
 	struct powerz_priv *priv = usb_get_intfdata(intf);
 
+	usb_set_intfdata(intf, NULL);
+
 	mutex_lock(&priv->mutex);
 	usb_kill_urb(priv->urb);
 	usb_free_urb(priv->urb);
+	priv->urb = NULL;
 	mutex_unlock(&priv->mutex);
 }
 
-- 
2.34.1

Re: [PATCH v2 3/3] hwmon: (powerz) Fix use-after-free and signal handling on USB disconnect

[Pradhan, Sanman]

From: Sanman Pradhan <psanman@juniper.net>

Good observation in the AI feedback, this would be a pre-existing issue
in the driver, as this patch does not change the struct layout or DMA
buffer usage.

I would prefer to keep this patch focused on the disconnect lifetime and
signal-handling fixes. Addressing potential cacheline sharing for the DMA
buffer likely requires a more deliberate change to the struct layout or
allocation strategy, so it would be better handled in a separate follow-up.

Happy to take a look at that separately if needed.

Thank you.

Regards,
Sanman Pradhan

Re: [PATCH v2 3/3] hwmon: (powerz) Fix use-after-free and signal handling on USB disconnect

[Guenter Roeck]

On 4/7/26 14:22, Pradhan, Sanman wrote:
> From: Sanman Pradhan <psanman@juniper.net>
> 
> Good observation in the AI feedback, this would be a pre-existing issue
> in the driver, as this patch does not change the struct layout or DMA
> buffer usage.
> 
> I would prefer to keep this patch focused on the disconnect lifetime and
> signal-handling fixes. Addressing potential cacheline sharing for the DMA
> buffer likely requires a more deliberate change to the struct layout or
> allocation strategy, so it would be better handled in a separate follow-up.
> 
> Happy to take a look at that separately if needed.
> 

Makes sense.

Thanks,
Guenter