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

["Pradhan, Sanman" <sanman.pradhan@hpe.com>]

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