Re: [PATCH 0/2] wifi: carl9170: revert broken devres conversions for input and hwrng

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

Hi Christian,

On Thu, Oct 01, 2026 at 08:30:29PM +0200, Christian Lamparter wrote:
> On 10/1/26 6:45 AM, Dmitry Torokhov wrote:
> > Commits 23de0fa0d2a0 ("carl9170: devres-ing hwrng_register usage") and
> > 87ddb2fc29f1 ("carl9170: devres-ing input_allocate_device") converted
> > the HWRNG and WPS button input device registrations in carl9170 to
> > devres attached to the parent struct usb_device (&ar->udev->dev) and
> > removed the explicit unregistration calls from carl9170_unregister().
> > 
> > Because carl9170_register() runs asynchronously from the
> > request_firmware_nowait() callback after probe has returned, and
> > carl9170_usb_disconnect() frees struct ar9170 immediately in the
> > interface disconnect callback before devres_release_all() runs, both the
> > WPS input device (along with its ar->wps.name and ar->wps.phys strings)
> > and the embedded struct hwrng remain registered after struct ar9170 has
> > been freed, leading to use-after-free bugs.
>
> ? Do I have a different source there ?
>
> carl9170_usb_disconnect() does a wait_for_completion(&ar->fw_load_wait)
> before doing anything.
>
> For this completion to be "completed" either the firmware loader callback
> went as far as running through all the initialization (includes
> carl9170_register(), which registers the WPS button + rng) successfully
> and the device is up.
>
> or if there was a grave error (usb protocol error, firmware not responding
> the way we want) and the driver basically has to give up... (but then
> carl9170_register would have never been able to even get as far as
> registering the wps + rng)

Sorry for the confusion, mentioning request_firmware_nowait() in the
commit description was a distraction. The issue is not a race with
fw_load_wait, but what happens *after* wait_for_completion() returns in
carl9170_usb_disconnect():

1. Normal disconnect / unbind teardown order:
   In carl9170_usb_disconnect(), right after wait_for_completion(), the
   driver calls carl9170_unregister(ar) and then carl9170_free(ar) ->
   ieee80211_free_hw(ar->hw), which frees struct ar9170 immediately
   inside the .disconnect() callback.

   Because the devres conversions removed input_unregister_device() and
   hwrng_unregister() from carl9170_unregister(), both the WPS input
   device (whose input->name and input->phys point to ar->wps.name and
   ar->wps.phys inside struct ar9170) and ar->rng.rng (which is embedded
   directly inside struct ar9170) are still registered when struct
   ar9170 is freed:

   - The devm_* calls were attached to &ar->udev->dev (the parent
     struct usb_device) rather than &ar->intf->dev (struct
     usb_interface). If the driver is unbound from the interface (via
     sysfs unbind or rmmod) while the USB device stays plugged in,
     devres_release_all(&ar->udev->dev) does not run at all, leaving the
     input device and embedded hwrng registered in global lists after ar
     is freed.
   - Even on a physical USB unplug (and even if &ar->intf->dev had been
     used), the driver core runs .disconnect() before
     devres_release_all(). Thus struct ar9170 is already freed before
     devres runs devm_hwrng_unregister() (which dereferences the freed
     ar->rng.rng to unlink it from rng_list) and
     devm_input_device_unregister() (which reads the freed ar->wps.name
     and ar->wps.phys when generating the KOBJ_REMOVE uevent).

2. Error path in carl9170_register():
   Even during initial bringup, carl9170_register() calls
   carl9170_register_wps_button() and then carl9170_register_hwrng(),
   which calls devm_hwrng_register() *before* carl9170_rng_get(ar)
   issues CARL9170_CMD_RREG over USB.

   If carl9170_rng_get(ar) fails due to a USB or firmware error, both
   the WPS button and the HWRNG have already been registered.
   carl9170_register() then jumps to err_unreg -> carl9170_unregister(),
   and carl9170_usb_firmware_failed() calls
   usb_driver_release_interface(), which runs carl9170_usb_disconnect()
   and frees ar while &ar->udev->dev remains bound.

I can send a v2 with updated commit messages that drop the mention of
request_firmware_nowait() and focus directly on the disconnect teardown
order if you prefer.

Thanks.

-- 
Dmitry