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

[Thomas Weißschuh]

On 2026-04-09 15:50:54+0000, Pradhan, Sanman wrote:
> On 2026-04-08 19:30+0000, Thomas Weißschuh wrote:

(...)

> > > 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.

Agreed, that makes sense.

(...)

> > > 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.

I misunderstood the "stale URB data". This is *not* a use-after-free
but read from the buffer that has not yet been filled by the device.

> 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.

Ack.

Maybe the description can be reworded to be a bit clearer.

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

(...)


Thomas