Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:3 as permitted sender) client-ip=2620:137:e000::3:3;
From:   George Stark <gnstark@salutedevices.com>
To:     <pavel@ucw.cz>, <lee@kernel.org>, <vadimp@nvidia.com>,
        <mpe@ellerman.id.au>, <npiggin@gmail.com>,
        <christophe.leroy@csgroup.eu>, <gnstark@salutedevices.com>
CC:     <linux-leds@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <linuxppc-dev@lists.ozlabs.org>, <kernel@sberdevices.ru>
Subject: [PATCH 0/8] devm_led_classdev_register() usage problem
Date:   Wed, 25 Oct 2023 16:07:29 +0300
Message-ID: <20231025130737.2015468-1-gnstark@salutedevices.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Content-Type: text/plain
Precedence: bulk

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

-- 
2.38.4