[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()

[sashiko-bot]

Sashiko AI review found 1 potential issue(s):
- [High] The driver incorrectly defines and uses a custom mutex instead of the required hwmon locking API.
--

commit 9e1f24f685e2634190aef8a25e65494baf85ed6c
Author: Sanman Pradhan <psanman@juniper.net>

    hwmon: (pt5161l) Fix bugs in pt5161l_read_block_data()
    
    This patch fixes a buffer overrun by resizing the local read buffer to
    I2C_SMBUS_BLOCK_MAX. It also ensures the function returns -EIO when
    all retries are exhausted with an unexpected length instead of returning
    a positive byte count.

> diff --git a/drivers/hwmon/pt5161l.c b/drivers/hwmon/pt5161l.c
> index 20e3cfa625f17..89d4da8aa4c09 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];

[ ... ]

This isn't a bug introduced by this patch, but looking at the pt5161l driver
as a whole, it appears to define and use a custom mutex instead of the required
hwmon locking API.

The subsystem guidelines mandate that drivers must use hwmon_lock() and
hwmon_unlock() to implement locking for attributes registered by means other
than devm_hwmon_device_register_with_info() (such as debugfs attributes).

Instead, this driver implements its own custom struct mutex lock inside
struct pt5161l_data and uses mutex_lock(&data->lock) to protect I2C operations
for debugfs attributes like fw_ver.

To comply with the architectural rules of the subsystem, should the driver save
the hwmon_dev pointer and utilize the standard hwmon_lock() and hwmon_unlock()
functions?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260408163029.353777-1-sanman.pradhan@hpe.com?part=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

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

[sashiko-bot]

Sashiko AI review found 1 potential issue(s):
- [Low] The use of the `min()` macro with mixed types (`u64` and `long`) will cause build failures when this patch is backported to older stable kernels.
--

commit 7d032fe46f91e450d244ac30f5851808fee10667
Author: Sanman Pradhan <psanman@juniper.net>

hwmon: (isl28022) Fix integer overflow in power calculation on 32-bit

This commit fixes an integer overflow on 32-bit systems in the power
calculation of the isl28022 driver by switching to 64-bit arithmetic
and using div_u64().

> diff --git a/drivers/hwmon/isl28022.c b/drivers/hwmon/isl28022.c
> index c2e559dde63f6..c5a34ceedcdb2 100644
> --- a/drivers/hwmon/isl28022.c
> +++ b/drivers/hwmon/isl28022.c

[ ... ]

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

Is there a risk of build failures when this patch is backported to older
stable kernels?

Since this patch is marked with "Cc: stable@vger.kernel.org", it will likely
be backported to older LTS kernels like 5.15 or 6.1. While modern kernels
have relaxed the type-checking in the min() macro to allow safely comparable
mixed types, older kernels strictly enforce identical types via pointer
comparison.

Comparing a u64 (from div_u64()) and a long (LONG_MAX) might trigger a
"comparison of distinct pointer types lacks a cast" compiler error during
backporting.

Would it be better to use min_t(u64, div_u64(...), LONG_MAX) to avoid this
issue on older trees?

>  		break;
>  	default:
>  		return -EOPNOTSUPP;
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260408163029.353777-1-sanman.pradhan@hpe.com?part=2

[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

[sashiko-bot]

Sashiko AI review found 1 potential issue(s):
- [Critical] Unsafe DMA buffer allocation resulting in cacheline sharing and memory corruption
--

commit e52b87f2333c018b83f387af6cae9b706448d9b1
Author: Sanman Pradhan <psanman@juniper.net>

hwmon: (powerz) Fix use-after-free and signal handling on USB disconnect

This patch fixes several issues in the powerz driver related to USB disconnect
races and signal handling, including a use-after-free and incorrect return
value checks.

> diff --git a/drivers/hwmon/powerz.c b/drivers/hwmon/powerz.c
> index 4e663d5b4e330..0b38fdf42ddbe 100644
> --- a/drivers/hwmon/powerz.c
> +++ b/drivers/hwmon/powerz.c
[ ... ]
> @@ -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);

This isn't a regression introduced by this patch, but is there a pre-existing
cacheline sharing issue between priv->transfer_buffer and priv->mutex?

The transfer_buffer is a 64-byte array embedded at the start of
struct powerz_priv, immediately followed by the mutex and completion:

struct powerz_priv {
	char transfer_buffer[64];
	...

While devm_kzalloc() guarantees ARCH_DMA_MINALIGN alignment for the start
of the structure, it does not guarantee padding. On architectures where
ARCH_DMA_MINALIGN is greater than 64 bytes (such as 128 bytes on some
ARM64 systems), transfer_buffer and the mutex will share the same CPU
cacheline.

During a USB read, transfer_buffer is mapped for DMA_FROM_DEVICE. When the
transfer finishes, the DMA API invalidates the cacheline. If another CPU
modifies priv->mutex here in powerz_disconnect() concurrently, those
modifications could reside in the CPU's dirty cacheline and be silently
erased by the invalidation.

Does this violate the DMA-API rule that DMA buffers must not share cachelines
with other variables, leading to silent lock state or wait queue corruption?

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260408163029.353777-1-sanman.pradhan@hpe.com?part=3

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