generic: add leds_st1202 patch to fix NULL pointer access

[innovara]

Merged on #17451, the module has to be loaded before launching preinit to avoid a kernel panic triggered by userspace accessing the controller before it is ready.

It was initially put under modules to minimise the time the LEDs are off, as the driver resets them all, but that surfaced the before-mentioned issue.

So lets use AutoLoad with the boot flag and a relatively low priority as most other LED driver modules.

[KanjiMonster]

Merged on #17451, the module has to be loaded before launching preinit to avoid a kernel panic triggered by userspace accessing the controller before it is ready.

If it causes a kernel panic it means the driver is broken and needs to be fixed, not force loaded on boot to avoid it.

Please report the issue upstream so it can be fixed.

Also if there is an error message, always include the error message verbatim.

[KanjiMonster]

And at a quick glance looking at the driver, I'm 99% sure the issue is that st1202_dt_init() calls devm_led_classdev_register_ext() before the internal data structures are properly setup, so the leds become visible to user space while being partially initialized, leading to a window where trying to access them will cause (probably) a NULL pointer access.

[innovara]

If it causes a kernel panic it means the driver is broken and needs to be fixed, not force loaded on boot to avoid it.

Please report the issue upstream so it can be fixed.

It was reported to the developer, but it is not something that he can reproduce, nor immediately obvious that the crash was caused by the driver itself. Your comment about the device becoming visible before it should could well explain why the crash seems to happen elsewhere but the root cause is still the driver.

I was working on the basis that I had made a bad decision not putting the module in boot which is what almost every other kmod LED package does. Therefore, I didn't consider adding any crash information which I am doing now
as suggested.

dmesg-pstore_blk-1.txt is the crash with the driver unmodified.
dmesg-pstore_blk-6.txt is the crash with some pr_info I sprinkled to know what was happening when. It might corroborate what you said but I am out of my depth here.

I still think that moving it to boot is not forcing anything and it seems to be the norm but it doesn't change the fact that if there is a bug, it needs fixing.

On a related note, I was able to collect the panic logs with #17422 and #16689. I'd be grateful if you could take a look at them at some point.

[KanjiMonster]

[   13.081187] Unable to handle kernel access to user memory outside uaccess routines at virtual address 0000000000000008

That's a NULL pointer access all right.

[   13.289312] Call trace:
[   13.296418]  mutex_lock+0x14/0x58
[   13.298678]  led_update_brightness+0x28/0x68
[   13.302153]  brightness_show+0x38/0x78

so this happens here:

static enum led_brightness st1202_brightness_get(struct led_classdev *led_cdev)
{
	struct st1202_led *led = cdev_to_st1202_led(led_cdev);
	struct st1202_chip *chip = led->chip;
	u8 value = 0;

	guard(mutex)(&chip->lock);
        ^^^^^^^^^^^^^^^^^^^^^^^^^

where led->chip is (still) NULL, as devm_led_classdev_register_ext() was called in st1202_dt_init(), but led->chip is only set after that:

static int st1202_probe(struct i2c_client *client)
{
	...

	ret = st1202_dt_init(chip); <- registers the leds
	if (ret < 0)
		return ret;

	...

	for (int i = 0; i < ST1202_MAX_LEDS; i++) {
		led = &chip->leds[i];
		led->chip = chip; <- and only here it is actually set
		led->led_num = i;

		if (!led->is_active)
			continue;

		ret = st1202_channel_set(led->chip, led->led_num, true);
		if (ret < 0)
			return dev_err_probe(&client->dev, ret,
					"Failed to activate LED channel\n");

		ret = st1202_led_pattern_clear(&led->led_cdev);
		if (ret < 0)
			return dev_err_probe(&client->dev, ret,
					"Failed to clear LED pattern\n");
	}

Classic race condition ;-)

There is a lot of stuff happening after devm_led_classdev_register_ext() that looks racy as well, so devm_led_classdev_register_ext() should actually be the very last thing to happen, only after all things have been setup.

[innovara]

@KanjiMonster thanks, I'll pass the feedback to the developer. Meanwhile, I made an amateur attempt at moving devm_led_classdev_register_ext() to the end of st1202_probe(), since there is already a for loop to set the channels and clear the patterns for each active LED which by the way I reckon should be first clear pattern then activate LED. Earlier in st1202_probe(), I also swapped st1202_dt_init() and st1202_setup() so the later is done first. All seems to work but I need to test more carefully.

leds-st1202.txt

@robimarko please hold fire because it might not be necessary to move the module to boot which would help shorten the time between the module loading and /etc/init.d/led coming in which was the preferred option.

[innovara]

Since my last message, the driver as it is in OpenWrt (v11) has been added to the LEDs subsystem, although I can't find any trace of that on public repositories: https://lore.kernel.org/lkml/[email protected]

I have changed this PR to a patch with the minimum changes required to prevent the NULL pointer access for your consideration. There is no need to move the module to boot.

I also passed the feedback to the developer so he might take some actions himself. Otherwise, I will send the patch upstream once the driver actually shows up in their repo.

[robimarko]

I will merge this, but please send it upstream as well since it looks like this driver will end up in the next release since LED maintainer has merged it into his LED tree in the for-next branch:
https://git.kernel.org/pub/scm/linux/kernel/git/lee/leds.git/log/?h=for-leds-next

[robimarko]

Thanks! Rebased on top of main and merged!

[innovara]

I will merge this, but please send it upstream as well

Thanks. Yes, the patch you've merged is what I sent after make target/linux/refresh