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

[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

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

[David Laight]

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

> 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.

No, use min() not min_t().

min_t() doesn't make the type handling 'explicit', it just casts both
values to the specified type and lets you live with any consequences.
min() attempts to stop you doing 'really silly thing' (mostly trying
to stop negative signed values becoming very large signed values).

Even if the compiler generates a signedness warning from min()
replacing it with min_t() really ought to be a last resort.
Not the least of the problems is that people have a habit of using
the type they want for the result, so you'll find:
	x = clamp_t(u8, y, 0, 255);
That is just:
	x = clamp((u8)y, (u8)0, (u8)255);
which just masks the high bits instead of the intended saturation.

	David

> 
> 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