[PATCH] hwmon: (powerz) Avoid cacheline sharing for DMA buffer

[PATCH] hwmon: (powerz) Avoid cacheline sharing for DMA buffer

[Thomas Weißschuh]

Depending on the architecture the transfer buffer may share a cacheline
with the following mutex. As the buffer may be used for DMA, that is
problematic.

Use the high-level DMA helpers to make sure that cacheline sharing can
not happen.

Also drop the comment, as the helpers are documentation enough.

https://sashiko.dev/#/message/20260408175814.934BFC19421%40smtp.kernel.org

Fixes: 4381a36abdf1c ("hwmon: add POWER-Z driver")
Cc: stable@vger.kernel.org # ca085faabb42: dma-mapping: add __dma_from_device_group_begin()/end()
Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/hwmon/powerz.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/powerz.c b/drivers/hwmon/powerz.c
index 4e663d5b4e33..5e8397895613 100644
--- a/drivers/hwmon/powerz.c
+++ b/drivers/hwmon/powerz.c
@@ -6,6 +6,7 @@
 
 #include <linux/completion.h>
 #include <linux/device.h>
+#include <linux/dma-mapping.h>
 #include <linux/hwmon.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
@@ -33,7 +34,9 @@ struct powerz_sensor_data {
 } __packed;
 
 struct powerz_priv {
-	char transfer_buffer[64];	/* first member to satisfy DMA alignment */
+	__dma_from_device_group_begin();
+	char transfer_buffer[64];
+	__dma_from_device_group_end();
 	struct mutex mutex;
 	struct completion completion;
 	struct urb *urb;

---
base-commit: 3036cd0d3328220a1858b1ab390be8b562774e8a
change-id: 20260408-powerz-cacheline-alias-dfca016234c2

Best regards,
--  
Thomas Weißschuh <linux@weissschuh.net>

Re: [PATCH] hwmon: (powerz) Avoid cacheline sharing for DMA buffer

[sashiko-bot]

Sashiko AI review found 2 potential issue(s):
- [High] A Use-After-Free (UAF) vulnerability occurs on `priv->urb` due to a race condition between sysfs reads and device disconnection.
- [High] Incorrect error handling of `wait_for_completion_interruptible_timeout()` allows an active URB to leak in-flight, leading to concurrent DMA modification and URB state corruption.
--

commit e653439f03252b9df547ec7df8f35d202c25c060
Author: Thomas Weißschuh <linux@weissschuh.net>

hwmon: (powerz) Avoid cacheline sharing for DMA buffer

This commit uses the high-level DMA helpers to ensure cacheline sharing
cannot occur for the DMA transfer buffer.

https://sashiko.dev/#/message/20260408175814.934BFC19421%40smtp.kernel.org

> diff --git a/drivers/hwmon/powerz.c b/drivers/hwmon/powerz.c
> index 4e663d5b4e330..5e83978956137 100644
> --- a/drivers/hwmon/powerz.c
> +++ b/drivers/hwmon/powerz.c
[ ... ]
> @@ -33,7 +34,9 @@ struct powerz_sensor_data {
>  } __packed;
>  
>  struct powerz_priv {
> -	char transfer_buffer[64];	/* first member to satisfy DMA alignment */
> +	__dma_from_device_group_begin();
> +	char transfer_buffer[64];
> +	__dma_from_device_group_end();
>  	struct mutex mutex;
>  	struct completion completion;
>  	struct urb *urb;

While reviewing this patch, I noticed a couple of pre-existing issues in
this driver.

Can a use-after-free occur on priv->urb during device disconnection?

The hwmon device is registered using devm_hwmon_device_register_with_info(),
meaning its sysfs attributes are removed during devres cleanup, which
happens after powerz_disconnect() returns.

If a userspace process performs a sysfs read concurrently, it can block
on mutex_lock(&priv->mutex) in powerz_read(). Once powerz_disconnect()
frees the URB and drops the mutex:

drivers/hwmon/powerz.c:powerz_disconnect() {
    ...
	mutex_lock(&priv->mutex);
	usb_kill_urb(priv->urb);
	usb_free_urb(priv->urb);
	mutex_unlock(&priv->mutex);
}

The blocked sysfs read could wake up, proceed into powerz_read_data(),
and use the freed priv->urb:

drivers/hwmon/powerz.c:powerz_read_data() {
    ...
	usb_fill_bulk_urb(priv->urb, udev, ...);
	ret = usb_submit_urb(priv->urb, GFP_KERNEL);
    ...
}


Additionally, is the error handling for wait_for_completion_interruptible_timeout()
correct in powerz_read_data()?

drivers/hwmon/powerz.c:powerz_read_data() {
    ...
	ret = usb_submit_urb(priv->urb, GFP_KERNEL);
	if (ret)
		return ret;

	if (!wait_for_completion_interruptible_timeout
	    (&priv->completion, msecs_to_jiffies(5))) {
		usb_kill_urb(priv->urb);
		return -EIO;
	}
    ...
}

If wait_for_completion_interruptible_timeout() is interrupted by a signal,
it returns -ERESTARTSYS (a negative value). The condition
if (!wait_for_completion_interruptible_timeout(...)) only catches timeouts
(which return 0), bypassing the usb_kill_urb() cleanup.

This could allow the function to return -EIO (since actual_length is 0)
and release priv->mutex while the USB transfer is still active.

Could a subsequent sysfs read then acquire the mutex, modify
priv->transfer_buffer, and call usb_submit_urb() on an active URB?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260408-powerz-cacheline-alias-v1-1-1254891be0dd@weissschuh.net?part=1