[PATCH v4 0/2] hwmon: (powerz) Fix disconnect and signal handling bugs

[PATCH v4 0/2] hwmon: (powerz) Fix disconnect and signal handling bugs

[Pradhan, Sanman]

From: Sanman Pradhan <psanman@juniper.net>

Fix two independent bugs in the powerz USB hwmon driver:

1. Use-after-free: After USB disconnect frees the URB, a subsequent
   sysfs read can dereference the freed pointer.
2. Missing usb_kill_urb() on signal: When
   wait_for_completion_interruptible_timeout() is interrupted by a
   signal, the in-flight URB is not cancelled.

Changes since v3:
- Patch 1/2: Split from combined patch, reword commit message,
  drop unnecessary usb_set_intfdata(NULL) calls.
- Patch 2/2: Split from combined patch, use long type for wait
  return value, split into separate signal/timeout checks.

Sanman Pradhan (2):
  hwmon: (powerz) Fix use-after-free on USB disconnect
  hwmon: (powerz) Fix missing usb_kill_urb() on signal interrupt

 drivers/hwmon/powerz.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

-- 
2.34.1

[PATCH v4 1/2] hwmon: (powerz) Fix use-after-free on USB disconnect

[Pradhan, Sanman]

From: Sanman Pradhan <psanman@juniper.net>

After powerz_disconnect() frees the URB and releases the mutex, a
subsequent powerz_read() call can acquire the mutex and call
powerz_read_data(), which dereferences the freed URB pointer.

Fix by:
 - Setting priv->urb to NULL in powerz_disconnect() so that
   powerz_read_data() can detect the disconnected state.
 - Adding a !priv->urb check at the start of powerz_read_data()
   to return -ENODEV on a disconnected device.
 - Moving usb_set_intfdata() before hwmon registration so the
   disconnect handler can always find the priv pointer.

Fixes: 4381a36abdf1c ("hwmon: add POWER-Z driver")
Cc: stable@vger.kernel.org
Signed-off-by: Sanman Pradhan <psanman@juniper.net>
---
v4:
 - Split from combined patch into standalone disconnect fix
 - Reword commit message per review feedback
 - Drop usb_set_intfdata(intf, NULL) in probe error path
   and disconnect (unnecessary)
v1-v3:
 - Part of combined patch "Fix use-after-free and signal
   handling on USB disconnect"

 drivers/hwmon/powerz.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/powerz.c b/drivers/hwmon/powerz.c
index 4e663d5b4e33..a75b941bd6e2 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);
 
@@ -224,6 +227,8 @@ 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);
@@ -232,8 +237,6 @@ static int powerz_probe(struct usb_interface *intf,
 		return PTR_ERR(hwmon_dev);
 	}
 
-	usb_set_intfdata(intf, priv);
-
 	return 0;
 }
 
@@ -244,6 +247,7 @@ static void powerz_disconnect(struct usb_interface *intf)
 	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 v4 1/2] hwmon: (powerz) Fix use-after-free on USB disconnect

[sashiko-bot]

Sashiko AI review found 1 potential issue(s):
- [High] Unsafe DMA Buffer Cacheline Sharing (Pre-existing Bug)
--

commit f830fe65a5d61278ed5e2ee2dbcd0c937471cfd4
Author: Sanman Pradhan <psanman@juniper.net>
hwmon: (powerz) Fix use-after-free on USB disconnect

This commit prevents a use-after-free by setting the URB pointer to NULL on
device disconnect, and checking for it before starting any read operations. It
also moves the hardware monitoring registration after the private data is set.

> diff --git a/drivers/hwmon/powerz.c b/drivers/hwmon/powerz.c
> --- 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);

This isn't a bug introduced by this patch, but looking at the structure being
modified here:

struct powerz_priv {
        char transfer_buffer[64];
        struct mutex mutex;
        struct completion completion;
        struct urb *urb;
        int status;
};

Can this layout cause memory corruption during DMA?

The transfer_buffer is embedded at the start of the structure and is exactly 64
bytes long. On architectures where the CPU cacheline size or DMA minimum
alignment exceeds 64 bytes (such as 128 bytes on many arm64 platforms),
transfer_buffer will share a cacheline with the mutex and completion members.

When the USB core unmaps the buffer after an inbound transfer, the cache
invalidation operates on the entire cacheline. If the CPU concurrently modifies
the mutex or completion structures (for example, when a thread is placed on
the wait queue via wait_for_completion_interruptible_timeout), will those CPU
writes be destroyed by the DMA cache invalidation, or conversely corrupt the
inbound DMA data?

Should transfer_buffer be allocated separately, or forced to an explicit
alignment to ensure safe DMA?

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

Re: [PATCH v4 1/2] hwmon: (powerz) Fix use-after-free on USB disconnect

[Guenter Roeck]

On Fri, Apr 10, 2026 at 12:25:35AM +0000, Pradhan, Sanman wrote:
> From: Sanman Pradhan <psanman@juniper.net>
> 
> After powerz_disconnect() frees the URB and releases the mutex, a
> subsequent powerz_read() call can acquire the mutex and call
> powerz_read_data(), which dereferences the freed URB pointer.
> 
> Fix by:
>  - Setting priv->urb to NULL in powerz_disconnect() so that
>    powerz_read_data() can detect the disconnected state.
>  - Adding a !priv->urb check at the start of powerz_read_data()
>    to return -ENODEV on a disconnected device.
>  - Moving usb_set_intfdata() before hwmon registration so the
>    disconnect handler can always find the priv pointer.
> 
> Fixes: 4381a36abdf1c ("hwmon: add POWER-Z driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Sanman Pradhan <psanman@juniper.net>

Applied.

Thanks,
Guenter

[PATCH v4 2/2] hwmon: (powerz) Fix missing usb_kill_urb() on signal interrupt

[Pradhan, Sanman]

From: Sanman Pradhan <psanman@juniper.net>

wait_for_completion_interruptible_timeout() returns -ERESTARTSYS 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.

The original code tests !ret, which only catches the timeout case (0).
On signal delivery (-ERESTARTSYS), !ret is false so the function skips
usb_kill_urb() and falls through to read from the unfilled transfer
buffer.

Fix by capturing the return value into a long (matching the function
return type) and handling signal (negative) and timeout (zero) cases
with separate checks that both call usb_kill_urb() before returning.

Fixes: 4381a36abdf1c ("hwmon: add POWER-Z driver")
Cc: stable@vger.kernel.org
Signed-off-by: Sanman Pradhan <psanman@juniper.net>
---
v4:
 - Split from combined patch into standalone signal fix
 - Use long type for wait_for_completion return value
 - Split ret <= 0 check into separate if (ret < 0) and
   if (ret == 0) blocks per review feedback
 - Reword commit message per review feedback
v1-v3:
 - Part of combined patch "Fix use-after-free and signal
   handling on USB disconnect"

 drivers/hwmon/powerz.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/powerz.c b/drivers/hwmon/powerz.c
index a75b941bd6e2..96438f5f05d4 100644
--- a/drivers/hwmon/powerz.c
+++ b/drivers/hwmon/powerz.c
@@ -106,6 +106,7 @@ static void powerz_usb_cmd_complete(struct urb *urb)
 
 static int powerz_read_data(struct usb_device *udev, struct powerz_priv *priv)
 {
+	long rc;
 	int ret;
 
 	if (!priv->urb)
@@ -127,8 +128,14 @@ 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))) {
+	rc = wait_for_completion_interruptible_timeout(&priv->completion,
+						       msecs_to_jiffies(5));
+	if (rc < 0) {
+		usb_kill_urb(priv->urb);
+		return rc;
+	}
+
+	if (rc == 0) {
 		usb_kill_urb(priv->urb);
 		return -EIO;
 	}
-- 
2.34.1

Re: [PATCH v4 2/2] hwmon: (powerz) Fix missing usb_kill_urb() on signal interrupt

[sashiko-bot]

Sashiko AI review found 1 potential issue(s):
- [Critical] Unsafe DMA Buffer Allocation / Cacheline Sharing in `struct powerz_priv`
--

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

hwmon: (powerz) Fix missing usb_kill_urb() on signal interrupt

This commit fixes a bug where wait_for_completion_interruptible_timeout
returning a negative value due to a signal would bypass the usb_kill_urb
cleanup. It captures the return value into a long and handles both the
signal and timeout cases appropriately.

> diff --git a/drivers/hwmon/powerz.c b/drivers/hwmon/powerz.c
> index a75b941bd6e2f..96438f5f05d48 100644
> --- a/drivers/hwmon/powerz.c
> +++ b/drivers/hwmon/powerz.c

[ ... ]

> @@ -127,8 +128,14 @@ 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))) {
> +	rc = wait_for_completion_interruptible_timeout(&priv->completion,
> +						       msecs_to_jiffies(5));

This isn't a bug introduced by this patch, but could this completion logic
be vulnerable to memory corruption on architectures with larger cache
lines?

Looking at struct powerz_priv:

struct powerz_priv {
	char transfer_buffer[64];
	struct mutex mutex;
	struct completion completion;
	struct urb *urb;
	int status;
};

Since transfer_buffer[64] only occupies 64 bytes, could it share a
cacheline with the mutex and completion fields on architectures where
ARCH_DMA_MINALIGN is greater than 64?

When the USB subsystem unmaps the DMA buffer after a transfer, the DMA
layer might invalidate the entire cacheline. If the CPU concurrently
modifies the completion or mutex fields during wait queue operations
or locking, could those CPU writes be lost due to the invalidation?

Would it be safer to allocate the transfer buffer separately using a
dedicated allocation, or add adequate padding to ensure it occupies its
own cacheline?

[ ... ]

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

Re: [PATCH v4 2/2] hwmon: (powerz) Fix missing usb_kill_urb() on signal interrupt

[Guenter Roeck]

On Fri, Apr 10, 2026 at 12:25:41AM +0000, Pradhan, Sanman wrote:
> From: Sanman Pradhan <psanman@juniper.net>
> 
> wait_for_completion_interruptible_timeout() returns -ERESTARTSYS 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.
> 
> The original code tests !ret, which only catches the timeout case (0).
> On signal delivery (-ERESTARTSYS), !ret is false so the function skips
> usb_kill_urb() and falls through to read from the unfilled transfer
> buffer.
> 
> Fix by capturing the return value into a long (matching the function
> return type) and handling signal (negative) and timeout (zero) cases
> with separate checks that both call usb_kill_urb() before returning.
> 
> Fixes: 4381a36abdf1c ("hwmon: add POWER-Z driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Sanman Pradhan <psanman@juniper.net>

Applied.

Thanks,
Guenter

Re: [PATCH v4 0/2] hwmon: (powerz) Fix disconnect and signal handling bugs

[Thomas Weißschuh]

Hi Sanman,

On 2026-04-10 00:25:29+0000, Pradhan, Sanman wrote:
> Fix two independent bugs in the powerz USB hwmon driver:
> 
> 1. Use-after-free: After USB disconnect frees the URB, a subsequent
>    sysfs read can dereference the freed pointer.
> 2. Missing usb_kill_urb() on signal: When
>    wait_for_completion_interruptible_timeout() is interrupted by a
>    signal, the in-flight URB is not cancelled.
> 
> Changes since v3:
> - Patch 1/2: Split from combined patch, reword commit message,
>   drop unnecessary usb_set_intfdata(NULL) calls.
> - Patch 2/2: Split from combined patch, use long type for wait
>   return value, split into separate signal/timeout checks.
> 
> Sanman Pradhan (2):
>   hwmon: (powerz) Fix use-after-free on USB disconnect
>   hwmon: (powerz) Fix missing usb_kill_urb() on signal interrupt

Thanks! For the series:

Acked-by: Thomas Weißschuh <linux@weissschuh.net>

FYI, the cacheline issue reported by Sashiko should be fixed by this:
https://lore.kernel.org/lkml/20260408-powerz-cacheline-alias-v1-1-1254891be0dd@weissschuh.net/


Thomas