linux-kernel - Re: [PATCH 0/8] devm_led_classdev

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <a2ebca38-7fa8-6aa8-6195-70d8df1b9f45@csgroup.eu>
Date:   Mon, 6 Nov 2023 08:11:36 +0000
From:   Christophe Leroy <christophe.leroy@...roup.eu>
To:     George Stark <gnstark@...utedevices.com>,
        "pavel@....cz" <pavel@....cz>, "lee@...nel.org" <lee@...nel.org>,
        "vadimp@...dia.com" <vadimp@...dia.com>,
        "mpe@...erman.id.au" <mpe@...erman.id.au>,
        "npiggin@...il.com" <npiggin@...il.com>
CC:     "linux-leds@...r.kernel.org" <linux-leds@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linuxppc-dev@...ts.ozlabs.org" <linuxppc-dev@...ts.ozlabs.org>,
        "kernel@...rdevices.ru" <kernel@...rdevices.ru>
Subject: Re: [PATCH 0/8] devm_led_classdev_register() usage problem



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