Re: [PATCH 0/8] devm_led_classdev_register() usage problem

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Sat, Nov 25, 2023 at 03:47:41AM +0300, George Stark wrote:
> On 11/24/23 18:28, Andy Shevchenko wrote:
> > On Wed, Oct 25, 2023 at 04:07:29PM +0300, George Stark wrote:
> > > Lots of drivers use devm_led_classdev_register() to register their led objects
> > > and let the kernel free those leds at the driver's remove stage.
> > > It can lead to a problem due to led_classdev_unregister()
> > > implementation calls led_set_brightness() to turn off the led.
> > > led_set_brightness() may call one of the module's brightness_set callbacks.
> > > If that callback uses module's resources allocated without using devm funcs()
> > > then those resources will be already freed at module's remove() callback and
> > > we may have use-after-free situation.
> > > 
> > > Here is an example:
> > > 
> > > module_probe()
> > > {
> > >      devm_led_classdev_register(module_brightness_set_cb);
> > >      mutex_init(&mutex);
> > > }
> > > 
> > > module_brightness_set_cb()
> > > {
> > >      mutex_lock(&mutex);
> > >      do_set_brightness();
> > >      mutex_unlock(&mutex);
> > > }
> > > 
> > > module_remove()
> > > {
> > >      mutex_destroy(&mutex);
> > > }
> > > 
> > > at rmmod:
> > > module_remove()
> > >      ->mutex_destroy(&mutex);
> > > devres_release_all()
> > >      ->led_classdev_unregister();
> > >          ->led_set_brightness();
> > >              ->module_brightness_set_cb();
> > >                   ->mutex_lock(&mutex);  /* use-after-free */
> > > 
> > > I think it's an architectural issue and should be discussed thoroughly.
> > > Some thoughts about fixing it as a start:
> > > 1) drivers can use devm_led_classdev_unregister() to explicitly free leds before
> > > dependend resources are freed. devm_led_classdev_register() remains being useful
> > > to simplify probe implementation.
> > > As a proof of concept I examined all drivers from drivers/leds and prepared
> > > patches where it's needed. Sometimes it was not as clean as just calling
> > > devm_led_classdev_unregister() because several drivers do not track
> > > their leds object at all - they can call devm_led_classdev_register() and drop the
> > > returned pointer. In that case I used devres group API.
> > > 
> > > Drivers outside drivers/leds should be checked too after discussion.
> > > 
> > > 2) remove led_set_brightness from led_classdev_unregister() and force the drivers
> > > to turn leds off at shutdown. May be add check that led's brightness is 0
> > > at led_classdev_unregister() and put a warning to dmesg if it's not.
> > > Actually in many cases it doesn't really need to turn off the leds manually one-by-one
> > > if driver shutdowns whole led controller. For the last case to disable the warning
> > > new flag can be brought in e.g LED_AUTO_OFF_AT_SHUTDOWN (similar to LED_RETAIN_AT_SHUTDOWN).
> > 
> > NAK.
> > 
> > Just fix the drivers by wrapping mutex_destroy() into devm, There are many
> > doing so. You may be brave enough to introduce devm_mutex_init() somewhere
> > in include/linux/device*
> 
> Just one thing about mutex_destroy(). It seems like there's no single
> opinion on should it be called in 100% cases e.g. in remove() paths.
> For example in iio subsystem Jonathan suggests it can be dropped in simple
> cases: https://www.spinics.net/lists/linux-iio/msg73423.html
> 
> So the question is can we just drop mutex_destroy() in module's remove()
> callback here if that mutex is needed for devm subsequent callbacks?

mutex_destroy() makes sense when debugging mutexes. It's harmless to drop,
but will make life harder to one who is trying to debug something there...

-- 
With Best Regards,
Andy Shevchenko