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

[sashiko-bot@kernel.org]

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