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

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

Hi Christian,

On Fri, Oct 09, 2026 at 09:37:36PM +0200, Christian Lamparter wrote:
> On 10/8/26 11:39 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 explicit unregistration from carl9170_unregister().
> > 
> > In carl9170_usb_disconnect(), the driver calls carl9170_unregister()
> > followed immediately by carl9170_free(), which frees struct ar9170
> > inside the interface .disconnect() callback before devres_release_all()
> > runs. Furthermore, devres on &ar->udev->dev is not released on
> > interface unbind or registration failure in carl9170_register().
> > As a result, both the WPS input device (whose input->name and
> > input->phys point into freed memory) and the embedded struct hwrng
> > remain registered after struct ar9170 has been freed, leading to
> > use-after-free bugs.
>
> Ok, so you just reworded your patch? Sight...

Yes, as I promised I would.

> looking at the WPS input
>
> |        input->name = ar->wps.name;
> |        input->phys = ar->wps.phys;
> |        input->id.bustype = BUS_USB;
> |        input->dev.parent = &ar->hw->wiphy->dev;
>           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> the input's dev.parent is set to ar->hw->wiphy->dev and not ar->udev->dev, right?
> Does this do anything at all? If not, why? The wiphy gets shutdown by
> ieee80211_unregister() and having the "freeing" stick around after the USB device
> is gone should not hurt, right?

devm_input_allocate_device() explicitly states:

	NOTE: the owner device is set up as parent of input device and
	users should not override it.

That is because managed input devices split teardown across two separate
devres entries: devm_input_allocate_device(dev) registers
devm_input_device_release() on dev, whereas input_register_device()
registers devm_input_device_unregister() on input->dev.parent.

Overriding input->dev.parent splits those two actions across different
devices: if carl9170 is unbound from the USB interface (via sysfs unbind
or rmmod) without physically unplugging the USB device, or if
input_register_device() fails, &ar->udev->dev is never unbound and
devm_input_device_release never runs, leaking struct input_dev and an
input-core module reference on every unbind/rebind cycle.

I'll look into how to make input core reject such overrides. So far
there are 4 drivers that do that and I'll fix them up.

Also, using ar->hw->wiphy->dev as the input device's parent has
its own issues: wiphy devices can be renamed ("iw phy phy0 set name
...") or moved between network namespaces ("iw phy phy0 set netns ...").
Because phyX sits under a netns-tagged ieee80211 glue directory whereas
input_class is not namespace-tagged:

- renaming phyX changes the input device's sysfs path without emitting
  KOBJ_MOVE uevents for child input/event devices (leaving udev's cached
  DEVPATH out of date); and

- moving phyX into another netns re-tags the phyX directory to that
  namespace, turning /sys/class/input/inputN into a dangling symlink
  in init_net.

> As for the hwrng, wouldn't it make sense to use the wiphy dev there as well?
> So the whole reverting can be sidestepped by simply going with wiphy dev.

If we do that then unregistering of input device and hwrng will happen
inside ieee80211_unregister_hw() -> wiphy_unregister() -> device_del(),
which will hold both global rtnl_lock() and wiphy_lock(). Unregistering
the HWRNG there stalls global RTNL while hwrng_unregister() waits on
cleanup_done for any in-flight read to finish, whereas explicit
carl9170_unregister() tears down both WPS input and HWRNG upfront before
wiphy_unregister() acquires rtnl_lock() and wiphy_lock().

Until carl9170's own lifecycle (carl9170_alloc()/carl9170_free()) is
managed via devres on &ar->intf->dev, explicit unregistration in
carl9170_unregister() is the right way to manage these sub-devices.

Thanks.

-- 
Dmitry