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

[PATCH v3 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 v2:
- Patch 2/3 (isl28022): Use min()/div_u64() one-liner instead of
  clamp_val() + tmp variable, per review feedback.
  Add overflow justification to commit message.
- Patches 1/3 and 3/3: No changes.

Changes since v1:
- Patch 1/3 (pt5161l): Also fix unexpected positive return when
  retries are exhausted due to length mismatch.
- Patch 2/3 (isl28022): Switch from s64/div_s64() to u64/div_u64()
  since power is inherently non-negative.
- Patch 3/3 (powerz): Also fix signal handling in
  wait_for_completion_interruptible_timeout(). Return -ENODEV
  instead of -EIO on disconnected device.

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 |  5 +++--
 drivers/hwmon/powerz.c   | 18 +++++++++++++-----
 drivers/hwmon/pt5161l.c  |  4 ++--
 3 files changed, 18 insertions(+), 9 deletions(-)

-- 
2.34.1

[PATCH v3 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>
---
v3:
 - No changes
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

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

[Pradhan, Sanman]

From: Sanman Pradhan <psanman@juniper.net>

I think in this case as well, the feedback from the AI, would be a broader
pre-existing locking question in the driver rather than something introduced
by this patch. This patch is only fixing the buffer size and unexpected
positive return in pt5161l_read_block_data().

I'd prefer to keep this fix focused, if that's ok, and look at the
debugfs/hwmon locking usage separately if needed.

Thank you.

Regards,
Sanman Pradhan

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

[Guenter Roeck]

On 4/8/26 10:22, Pradhan, Sanman wrote:
> From: Sanman Pradhan <psanman@juniper.net>
> 
> I think in this case as well, the feedback from the AI, would be a broader
> pre-existing locking question in the driver rather than something introduced
> by this patch. This patch is only fixing the buffer size and unexpected
> positive return in pt5161l_read_block_data().
> 
> I'd prefer to keep this fix focused, if that's ok, and look at the
> debugfs/hwmon locking usage separately if needed.
> 

Yes, that is ok.

Thanks,
Guenter

[PATCH v3 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 arithmetic with div_u64() and multiply before dividing.
The intermediate product cannot overflow u64
(worst case: 51200000 * 8 * 65535 = 26834432000000). Power is
inherently non-negative, so unsigned types are the natural fit.
Cap the result to LONG_MAX before returning it through the hwmon
callback.

Fixes: 39671a14df4f2 ("hwmon: (isl28022) new driver for ISL28022 power monitor")
Cc: stable@vger.kernel.org
Signed-off-by: Sanman Pradhan <psanman@juniper.net>
---
v3:
 - Use min()/div_u64() one-liner instead of clamp_val() + tmp
   variable, per review feedback
 - Add overflow justification to commit message
v2:
 - Switch from s64/div_s64() to u64/div_u64() since power is
   inherently non-negative

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

diff --git a/drivers/hwmon/isl28022.c b/drivers/hwmon/isl28022.c
index c2e559dde63f..c5a34ceedcdb 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>
 
@@ -185,8 +186,8 @@ 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;
+		*val = min(div_u64(51200000ULL * data->gain * regval,
+				   data->shunt), LONG_MAX);
 		break;
 	default:
 		return -EOPNOTSUPP;
-- 
2.34.1

[PATCH v3 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 before taking the mutex and NULLing the
     URB pointer in disconnect.
   - 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>
---
v3:
 - No changes
v2:
 - Also fix signal handling in
   wait_for_completion_interruptible_timeout()
 - Return -ENODEV instead of -EIO on 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 v3 3/3] hwmon: (powerz) Fix use-after-free and signal handling on USB disconnect

[Thomas Weißschuh]

Hi Sanman,

thanks for these fixes!

On 2026-04-08 16:31:29+0000, Pradhan, Sanman wrote:
> From: Sanman Pradhan <psanman@juniper.net>

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

Please split this into two patches.
It has no downside and makes everything else easier.

Also given that the series fixes completely different things in
different drivers, they could be submitted without a series.
This will reduce the spam to maintainers of unrelated drivers.
Multiple patches to a single driver can stay in a series.

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

powerz_read_data() is executed with the mutex held. powerz_disconnect()
will also take that mutex, so I don't see that race.

I do see the value of the urb = NULL assignment and the associated
check.

>    Fix by:
>    - Moving usb_set_intfdata() before hwmon registration so the
>      disconnect handler can always find the priv pointer.
>    - Clearing intfdata before taking the mutex and NULLing the
>      URB pointer in disconnect.
>    - Guarding powerz_read_data() with a !priv->urb check.

This also looks like it could be split into more patches.

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

What impact does the signal delivery have on URB validity?

> Fixes: 4381a36abdf1c ("hwmon: add POWER-Z driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Sanman Pradhan <psanman@juniper.net>
> ---
> v3:
>  - No changes
> v2:
>  - Also fix signal handling in
>    wait_for_completion_interruptible_timeout()
>  - Return -ENODEV instead of -EIO on 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;

Ack.

> +
>  	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));

Should use a long type.

> +	if (ret <= 0) {
>  		usb_kill_urb(priv->urb);
> -		return -EIO;
> +		return ret ? ret : -EIO;
>  	}

If these cases are to be handled differently I  would just split this
check into two parts.

if (ret < 0) {
	usb_kill_urb(priv->urb);
	return ret;
}

if (ret == 0) {
	usb_kill_urb(priv->urb);
	return -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);

Ack.

> +
>  	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);

Is this really necessary? If the probing fails I would expect the
underlying subsystem to clean this up anyways.

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

Why is this necessary? The intfdata is allocated to the parent device,
so it should not become unavailable.

> +
>  	mutex_lock(&priv->mutex);
>  	usb_kill_urb(priv->urb);
>  	usb_free_urb(priv->urb);
> +	priv->urb = NULL;

Ack.

>  	mutex_unlock(&priv->mutex);
>  }

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

[Pradhan, Sanman]

From: Sanman Pradhan <psanman@juniper.net>

On 2026-04-08 19:30+0000, Thomas Weißschuh wrote:
> > Fix several issues in the powerz driver related to USB disconnect
> > races and signal handling:
> 
> Please split this into two patches.
> It has no downside and makes everything else easier.

Agreed, will split into two patches for v4.

> Also given that the series fixes completely different things in
> different drivers, they could be submitted without a series.
> This will reduce the spam to maintainers of unrelated drivers.
> Multiple patches to a single driver can stay in a series.

Makes sense. For v4 I will submit the powerz patches as a standalone
2-patch mini-series and send patches to other drivers separately.

> > 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.
> 
> powerz_read_data() is executed with the mutex held. powerz_disconnect()
> will also take that mutex, so I don't see that race.

You are right, the mutex serializes concurrent access, so that
description was misleading.

The actual issue is that after powerz_disconnect() frees the URB and
releases the mutex, a subsequent powerz_read() can acquire the mutex
and call powerz_read_data(), which would then dereference the freed
URB pointer. Moving usb_set_intfdata() before registration and adding
priv->urb = NULL with the corresponding NULL check is sufficient to
prevent that. I will reword the commit message accordingly.

> I do see the value of the urb = NULL assignment and the associated
> check.

Ack.

> This also looks like it could be split into more patches.

I plan to keep the usb_set_intfdata() move together with the
priv->urb = NULL assignment and NULL check, since together they form
the disconnect-side fix.

> > 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.
> 
> What impact does the signal delivery have on URB validity?

My understanding is that on signal the URB may still be in flight: it
was submitted successfully, but the wait was interrupted before
completion was signaled.

In the original code, a negative return does not enter the timeout
path, so usb_kill_urb() is skipped and the code falls through to read
actual_length and transfer_buffer. It can also return with the URB
still pending. A subsequent powerz_read() then reinitializes the
completion and reuses transfer_buffer while the previous URB may still
reference it.

The URB may also complete later and signal the completion, which can
interfere with a subsequent read that reinitializes the same
completion structure.

Calling usb_kill_urb() in the signal case ensures the in-flight URB is
cancelled and its completion handler has finished before returning.

> > +   ret = wait_for_completion_interruptible_timeout(&priv->completion,
> > +							msecs_to_jiffies(5));
> 
> Should use a long type.

Right, wait_for_completion_interruptible_timeout() returns long.
Will fix.

> > +	if (ret <= 0) {
> >  		usb_kill_urb(priv->urb);
> > -		return -EIO;
> > +		return ret ? ret : -EIO;
> >  	}
> 
> If these cases are to be handled differently I  would just split this
> check into two parts.

Agreed, clearer that way. Will adopt:

	if (ret < 0) {
		usb_kill_urb(priv->urb);
		return ret;
	}

	if (ret == 0) {
		usb_kill_urb(priv->urb);
		return -EIO;
	}

> > +	usb_set_intfdata(intf, priv);
> 
> Ack.

> >  	if (IS_ERR(hwmon_dev)) {
> > +		usb_set_intfdata(intf, NULL);
> 
> Is this really necessary? If the probing fails I would expect the
> underlying subsystem to clean this up anyways.

You are right, disconnect is not called on probe failure. Will drop.

> > +	usb_set_intfdata(intf, NULL);
> 
> Why is this necessary? The intfdata is allocated to the parent device,
> so it should not become unavailable.

Agreed. The priv->urb = NULL check under the mutex is sufficient for
the post-disconnect case, so I will drop this as well.

Thanks Thomas for the thorough review.

Thank you.

Regards,
Sanman Pradhan

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

[Thomas Weißschuh]

On 2026-04-09 15:50:54+0000, Pradhan, Sanman wrote:
> On 2026-04-08 19:30+0000, Thomas Weißschuh wrote:

(...)

> > > 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.
> > 
> > powerz_read_data() is executed with the mutex held. powerz_disconnect()
> > will also take that mutex, so I don't see that race.
> 
> You are right, the mutex serializes concurrent access, so that
> description was misleading.
> 
> The actual issue is that after powerz_disconnect() frees the URB and
> releases the mutex, a subsequent powerz_read() can acquire the mutex
> and call powerz_read_data(), which would then dereference the freed
> URB pointer. Moving usb_set_intfdata() before registration and adding
> priv->urb = NULL with the corresponding NULL check is sufficient to
> prevent that. I will reword the commit message accordingly.

Agreed, that makes sense.

(...)

> > > 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.
> > 
> > What impact does the signal delivery have on URB validity?
> 
> My understanding is that on signal the URB may still be in flight: it
> was submitted successfully, but the wait was interrupted before
> completion was signaled.

I misunderstood the "stale URB data". This is *not* a use-after-free
but read from the buffer that has not yet been filled by the device.

> In the original code, a negative return does not enter the timeout
> path, so usb_kill_urb() is skipped and the code falls through to read
> actual_length and transfer_buffer. It can also return with the URB
> still pending. A subsequent powerz_read() then reinitializes the
> completion and reuses transfer_buffer while the previous URB may still
> reference it.
> 
> The URB may also complete later and signal the completion, which can
> interfere with a subsequent read that reinitializes the same
> completion structure.
> 
> Calling usb_kill_urb() in the signal case ensures the in-flight URB is
> cancelled and its completion handler has finished before returning.

Ack.

Maybe the description can be reworded to be a bit clearer.

"""
wait_for_completion_interruptible_timeout() returns -ERESTARTYSYS when
interrupted. This needs to abort the URB and return an error. No data
has been received from the device so any reads from the transfer buffer
are invalid.
"""

(...)


Thomas