[0/8] devm_led_classdev_register() usage problem

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

George Stark (8):
  leds: powernv: explicitly unregister LEDs at module's shutdown
  leds: nic78bx: explicitly unregister LEDs at module's shutdown
  leds: an30259a: explicitly unregister LEDs at module's shutdown
  leds: mlxreg: explicitly unregister LEDs at module's shutdown
  leds: aw200xx: explicitly unregister LEDs at module's shutdown
  leds: aw2013: explicitly unregister LEDs at module's shutdown
  leds: lp3952: explicitly unregister LEDs at module's shutdown
  leds: lm3532: explicitly unregister LEDs at module's shutdown

 drivers/leds/leds-an30259a.c |  4 ++++
 drivers/leds/leds-aw200xx.c  |  4 ++++
 drivers/leds/leds-aw2013.c   |  4 ++++
 drivers/leds/leds-lm3532.c   |  6 ++++++
 drivers/leds/leds-lp3952.c   |  5 +++++
 drivers/leds/leds-mlxreg.c   | 12 +++++++++++-
 drivers/leds/leds-nic78bx.c  |  4 ++++
 drivers/leds/leds-powernv.c  |  7 +++++++
 8 files changed, 45 insertions(+), 1 deletion(-)
  

Hello Andy

Could you please take a look at this patch series?

I've just found your post on habr about devres API misusing and I think 
this is just another case.

On 10/25/23 16:07, 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).
> 
> George Stark (8):
>    leds: powernv: explicitly unregister LEDs at module's shutdown
>    leds: nic78bx: explicitly unregister LEDs at module's shutdown
>    leds: an30259a: explicitly unregister LEDs at module's shutdown
>    leds: mlxreg: explicitly unregister LEDs at module's shutdown
>    leds: aw200xx: explicitly unregister LEDs at module's shutdown
>    leds: aw2013: explicitly unregister LEDs at module's shutdown
>    leds: lp3952: explicitly unregister LEDs at module's shutdown
>    leds: lm3532: explicitly unregister LEDs at module's shutdown
> 
>   drivers/leds/leds-an30259a.c |  4 ++++
>   drivers/leds/leds-aw200xx.c  |  4 ++++
>   drivers/leds/leds-aw2013.c   |  4 ++++
>   drivers/leds/leds-lm3532.c   |  6 ++++++
>   drivers/leds/leds-lp3952.c   |  5 +++++
>   drivers/leds/leds-mlxreg.c   | 12 +++++++++++-
>   drivers/leds/leds-nic78bx.c  |  4 ++++
>   drivers/leds/leds-powernv.c  |  7 +++++++
>   8 files changed, 45 insertions(+), 1 deletion(-)
>
  

Le 25/10/2023 à 15:07, George Stark a écrit :
> 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.

I see no point in using a device managed function if you have to call 
the unregister function. All the purpose of device managed functions is 
to avoid having to call an unregister function at the end.

> 
> 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).
> 
> George Stark (8):
>    leds: powernv: explicitly unregister LEDs at module's shutdown
>    leds: nic78bx: explicitly unregister LEDs at module's shutdown
>    leds: an30259a: explicitly unregister LEDs at module's shutdown
>    leds: mlxreg: explicitly unregister LEDs at module's shutdown
>    leds: aw200xx: explicitly unregister LEDs at module's shutdown
>    leds: aw2013: explicitly unregister LEDs at module's shutdown
>    leds: lp3952: explicitly unregister LEDs at module's shutdown
>    leds: lm3532: explicitly unregister LEDs at module's shutdown
> 
>   drivers/leds/leds-an30259a.c |  4 ++++
>   drivers/leds/leds-aw200xx.c  |  4 ++++
>   drivers/leds/leds-aw2013.c   |  4 ++++
>   drivers/leds/leds-lm3532.c   |  6 ++++++
>   drivers/leds/leds-lp3952.c   |  5 +++++
>   drivers/leds/leds-mlxreg.c   | 12 +++++++++++-
>   drivers/leds/leds-nic78bx.c  |  4 ++++
>   drivers/leds/leds-powernv.c  |  7 +++++++
>   8 files changed, 45 insertions(+), 1 deletion(-)
>
  

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*
  

On Sat, Nov 4, 2023 at 9:17 AM George Stark <gnstark@salutedevices.com> wrote:
>
> Hello Andy
>
> Could you please take a look at this patch series?
>
> I've just found your post on habr about devres API misusing and I think
> this is just another case.

Just had a look, sorry for the delay.
By quickly reading it seems to be a wrong approach (or wrong end to
start solving the issue from).
  

Hello Andy

Thanks for the review.

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?
  

On Sat, 25 Nov 2023 03:47:41 +0300
George Stark <gnstark@salutedevices.com> wrote:

> Hello Andy
> 
> Thanks for the review.
> 
> 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?

I've never considered it remotely critical. The way IIO works means that things
have gone pretty horribly wrong in the core if you managed to access a mutex after
the unwind of devm_iio_device_register() has completed but sure, add a
devm_mutex_init() and I'd happily see that adopted in IIO for consistency
and to avoid answering questions on whether it is necessary to call mutex_destroy()

My arguement has always eben that if line after(ish) a mutex_destroy() is going to
either free the memory it's in, or make it otherwise inaccessible (IIO is proxying
accesses via chardevs if there are open so should ensure they never hit the driver)
then it's pointless and messy to call mutex_destroy().  devm_mutex_init() gets rid
of that mess..

Jonathan


>
  

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