Re: [PATCH 09/10] leds: Support custom sysfs groups on multicolor and flash LEDs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] leds: dangling led_cdev->dev pointer on registration failure causes UAF
--

commit fc841d87d7344c0d4e24b361394ed5d20afd7ffb
Author: Armin Wolf <W_Armin@gmx.de>

leds: Support custom sysfs groups on multicolor and flash LEDs

This commit moves the multicolor and flash LED sysfs attribute groups onto
dedicated device types ("multicolor", "flash", and "standard") so that drivers
can populate led_cdev->groups without having them overwritten during
registration.

> diff --git a/drivers/leds/led-class-flash.c b/drivers/leds/led-class-flash.c
> index 953265fddb7d9..e023511cea1c1 100644
> --- a/drivers/leds/led-class-flash.c
> +++ b/drivers/leds/led-class-flash.c

[ ... ]

> @@ -289,26 +282,29 @@ int led_classdev_flash_register_ext(struct device *parent,
>  
>  	led_cdev = &fled_cdev->led_cdev;
>  
> -	if (led_cdev->flags & LED_DEV_CAP_FLASH) {
> -		if (!led_cdev->brightness_set_blocking)
> -			return -EINVAL;
> +	if (!(led_cdev->flags & LED_DEV_CAP_FLASH))
> +		return led_classdev_register_ext(parent, led_cdev, init_data);
>  
> -		ops = fled_cdev->ops;
> -		if (!ops || !ops->strobe_set)
> -			return -EINVAL;
> +	if (!led_cdev->brightness_set_blocking)
> +		return -EINVAL;
>  
> -		led_cdev->flash_resume = led_flash_resume;
> +	ops = fled_cdev->ops;
> +	if (!ops || !ops->strobe_set)
> +		return -EINVAL;
>  
> -		/* Select the sysfs attributes to be created for the device */
> -		led_flash_init_sysfs_groups(fled_cdev);
> -	}
> +	led_cdev->flash_resume = led_flash_resume;
>  
>  	/* Register led class device */
> -	ret = led_classdev_register_ext(parent, led_cdev, init_data);
> +	ret = led_classdev_init(parent, led_cdev, init_data);
>  	if (ret < 0)
>  		return ret;
>  
> -	return 0;
> +	led_cdev->dev->type = &led_flash_device_type;
> +	ret = led_classdev_add(led_cdev);
> +	if (ret < 0)
> +		put_device(led_cdev->dev);
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(led_classdev_flash_register_ext);
>  
> diff --git a/drivers/leds/led-class-multicolor.c b/drivers/leds/led-class-multicolor.c
> index 57df0b2e785b5..9b8c37f7bdf45 100644
> --- a/drivers/leds/led-class-multicolor.c
> +++ b/drivers/leds/led-class-multicolor.c

[ ... ]

> @@ -176,9 +183,16 @@ int led_classdev_multicolor_register_ext(struct device *parent,
>  
>  	led_cdev = &mcled_cdev->led_cdev;
>  	led_cdev->flags |= LED_MULTI_COLOR;
> -	mcled_cdev->led_cdev.groups = led_multicolor_groups;
> +	ret = led_classdev_init(parent, led_cdev, init_data);
> +	if (ret < 0)
> +		return ret;
> +
> +	led_cdev->dev->type = &led_multicolor_device_type;
> +	ret = led_classdev_add(led_cdev);
> +	if (ret < 0)
> +		put_device(led_cdev->dev);
>  
> -	return led_classdev_register_ext(parent, led_cdev, init_data);
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(led_classdev_multicolor_register_ext);

[Severity: High]
Could this leave led_cdev->dev as a dangling pointer when registration fails,
leading to a use-after-free in led_classdev_unregister()?

In led_classdev_init(), led_cdev->dev is allocated and initialized via
device_initialize():

drivers/leds/led-class.c:led_classdev_init() {
	...
	led_cdev->dev = kzalloc_obj(*led_cdev->dev);
	if (!led_cdev->dev)
		return -ENOMEM;

	device_initialize(led_cdev->dev);
	...
	ret = kobject_set_name(&led_cdev->dev->kobj, "%s", final_name);
	if (ret < 0)
	{
		put_device(led_cdev->dev);
		return ret;
	}
	...
}

If kobject_set_name() fails in led_classdev_init(), or if led_classdev_add()
subsequently fails (for example when device_add() fails due to a sysfs group
conflict or -ENOMEM) in led_classdev_flash_register_ext(),
led_classdev_multicolor_register_ext(), or led_classdev_register_ext(),
put_device(led_cdev->dev) releases the final reference and frees the struct
device via led_classdev_release(), but led_cdev->dev is not reset to NULL and
led_cdev->node is never added to leds_list.

Since commit 1dbb9fb4082ce2 ("leds: Allow to call led_classdev_unregister()
unconditionally"), led_classdev_unregister() permits unconditional teardown by
checking:

drivers/leds/led-class.c:led_classdev_unregister() {
	if (IS_ERR_OR_NULL(led_cdev->dev))
		return;
	...
	device_unregister(led_cdev->dev);
	...
	down_write(&leds_list_lock);
	list_del(&led_cdev->node);
	up_write(&leds_list_lock);
	...
}

Several drivers rely on this contract when cleaning up after failed
registration or on driver removal, such as drivers/leds/leds-blinkm.c:

drivers/leds/leds-blinkm.c:register_multicolor() {
	...
	err = led_classdev_multicolor_register(&client->dev, &mc_led->cdev.mcled_cdev);
	if (err < 0) {
		dev_err(&client->dev, "couldn't register LED %s\n",
				mc_led->cdev.led_cdev.name);
		sysfs_remove_group(&client->dev.kobj, &blinkm_group);
	}
	return 0;
}

as well as drivers/platform/x86/asus-wmi.c (where asus_wmi_led_init() jumps to
asus_wmi_led_exit() on error), drivers/platform/x86/asus-laptop.c, and
drivers/platform/x86/toshiba_acpi.c.

Because led_cdev->dev still holds a non-NULL, non-ERR_PTR address after
put_device(led_cdev->dev), wouldn't a subsequent call to
led_classdev_unregister() (or its flash and multicolor wrappers) bypass the
IS_ERR_OR_NULL(led_cdev->dev) check, pass the freed pointer to
device_unregister(led_cdev->dev), and dereference NULL in
list_del(&led_cdev->node)?

Should led_cdev->dev be set to NULL after put_device(led_cdev->dev) in the
error paths of led_classdev_init(), led_classdev_register_ext(),
led_classdev_flash_register_ext(), and led_classdev_multicolor_register_ext()?

> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index ace417a2388e0..49b321b3d863b 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -613,6 +613,10 @@ int led_classdev_add(struct led_classdev *led_cdev)
>  }
>  EXPORT_SYMBOL_GPL(led_classdev_add);
>  
> +static const struct device_type led_device_type = {
> +	.name = "standard",
> +};
> +
>  /**
>   * led_classdev_register_ext - register a new object of led_classdev class
>   *                             with init data.
> @@ -631,6 +635,7 @@ int led_classdev_register_ext(struct device *parent,
>  	if (ret < 0)
>  		return ret;
>  
> +	led_cdev->dev->type = &led_device_type;
>  	ret = led_classdev_add(led_cdev);
>  	if (ret < 0)
>  		put_device(led_cdev->dev);

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261018135339.1109271-1-W_Armin@gmx.de?part=9