linux-kernel - Re: [PATCH v2 10/15] leds: trigger: blkdev: Add LED trigger activate function

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: <YTr/iQBYclqjFri2@kroah.com>
Date:   Fri, 10 Sep 2021 08:47:37 +0200
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Ian Pilcher <arequipeno@...il.com>
Cc:     axboe@...nel.dk, pavel@....cz, linux-leds@...r.kernel.org,
        linux-block@...r.kernel.org, linux-kernel@...r.kernel.org,
        kabel@...nel.org
Subject: Re: [PATCH v2 10/15] leds: trigger: blkdev: Add LED trigger activate
 function

On Thu, Sep 09, 2021 at 05:25:08PM -0500, Ian Pilcher wrote:
> Allocate per-LED data structure and initialize with default values
> 
> Create /sys/class/leds/<led>/block_devices directory
> 
> Increment module reference count.  Module can only be removed when no LEDs
> are associated with the trigger.
> 
> Signed-off-by: Ian Pilcher <arequipeno@...il.com>
> ---
>  drivers/leds/trigger/ledtrig-blkdev.c | 57 +++++++++++++++++++++++++++
>  1 file changed, 57 insertions(+)
> 
> diff --git a/drivers/leds/trigger/ledtrig-blkdev.c b/drivers/leds/trigger/ledtrig-blkdev.c
> index 40dc55e5d4f3..6f78a9515976 100644
> --- a/drivers/leds/trigger/ledtrig-blkdev.c
> +++ b/drivers/leds/trigger/ledtrig-blkdev.c
> @@ -164,6 +164,62 @@ static void blkdev_process(struct work_struct *const work)
>  }
>  
>  
> +/*
> + *
> + *	Associate an LED with the blkdev trigger
> + *
> + */
> +
> +static int blkdev_activate(struct led_classdev *const led_dev)
> +{
> +	struct ledtrig_blkdev_led *led;
> +	int ret;
> +
> +	/* Don't allow module to be removed while any LEDs are linked */
> +	if (WARN_ON(!try_module_get(THIS_MODULE))) {

That pattern is racy and broken and never ever ever add it to the kernel
again please.  All existing in-kernel users of it are also wrong, we
have been removing them for decades now.

> +		ret = -ENODEV;		/* Shouldn't ever happen */
> +		goto exit_return;
> +	}
> +
> +	led = kmalloc(sizeof(*led), GFP_KERNEL);
> +	if (led == NULL) {
> +		ret = -ENOMEM;
> +		goto exit_put_module;
> +	}
> +
> +	led->led_dev = led_dev;
> +	led->blink_msec = LEDTRIG_BLKDEV_BLINK_MSEC;
> +	led->mode = LEDTRIG_BLKDEV_MODE_RW;
> +	INIT_HLIST_HEAD(&led->disks);
> +
> +	ret = mutex_lock_interruptible(&ledtrig_blkdev_mutex);
> +	if (ret != 0)
> +		goto exit_free;
> +
> +	led->dir = kobject_create_and_add("linked_devices",
> +					  &led_dev->dev->kobj);

You have created a "raw" kobject in the device tree now, which means
that userspace will not be notified of it and will have a "hole" in it's
knowledge.  Why not just create a named attribute group to this device
instead?

> +	if (led->dir == NULL) {
> +		ret = -ENOMEM;
> +		goto exit_unlock;
> +	}
> +
> +	hlist_add_head(&led->leds_node, &ledtrig_blkdev_leds);
> +	led_set_trigger_data(led_dev, led);
> +	ret = 0;
> +
> +exit_unlock:
> +	mutex_unlock(&ledtrig_blkdev_mutex);
> +exit_free:
> +	if (ret != 0)
> +		kfree(led);
> +exit_put_module:
> +	if (ret != 0)
> +		module_put(THIS_MODULE);

Again, racy and broken, please do not do this.

thanks,

greg k-h