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

[sashiko-bot@kernel.org]

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