linux-kernel - Re: [PATCH V4] leds: trigger: Introduce an USB port trigger

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]

Date:   Thu, 25 Aug 2016 08:49:48 -0400
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Rafał Miłecki <zajec5@...il.com>
Cc:     Richard Purdie <rpurdie@...ys.net>,
        Jacek Anaszewski <j.anaszewski@...sung.com>,
        Felipe Balbi <balbi@...nel.org>,
        Peter Chen <hzpeterchen@...il.com>, linux-usb@...r.kernel.org,
        Rafał Miłecki <rafal@...ecki.pl>,
        Jonathan Corbet <corbet@....net>,
        Ezequiel Garcia <ezequiel@...guardiasur.com.ar>,
        Boris Brezillon <boris.brezillon@...e-electrons.com>,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        Stephan Linz <linz@...pro.net>,
        open list <linux-kernel@...r.kernel.org>,
        "open list:DOCUMENTATION" <linux-doc@...r.kernel.org>,
        "open list:LED SUBSYSTEM" <linux-leds@...r.kernel.org>
Subject: Re: [PATCH V4] leds: trigger: Introduce an USB port trigger

On Thu, Aug 25, 2016 at 10:03:52AM +0200, Rafał Miłecki wrote:
> +static void usbport_trig_activate(struct led_classdev *led_cdev)
> +{
> +	struct usbport_trig_data *usbport_data;
> +	int err;
> +
> +	usbport_data = kzalloc(sizeof(*usbport_data), GFP_KERNEL);
> +	if (!usbport_data)
> +		return;
> +	usbport_data->led_cdev = led_cdev;
> +
> +	/* Storing ports */
> +	INIT_LIST_HEAD(&usbport_data->ports);
> +	usbport_data->ports_dir = kobject_create_and_add("ports",
> +							 &led_cdev->dev->kobj);

If you _ever_ find yourself in a driver having to use kobject calls,
it's a HUGE hint that you are doing something wrong.

Hint, you are doing this wrong :)

Use an attribute group if you need a subdirectory in sysfs, using a
"raw" kobject like this hides it from all userspace tools and so no
userspace program can see it (hint, try using libudev to access these
files attached to the device...)

> +	if (!usbport_data->ports_dir)
> +		goto err_free;
> +
> +	/* API for ports management */
> +	err = device_create_file(led_cdev->dev, &dev_attr_new_port);
> +	if (err)
> +		goto err_put_ports;
> +	err = device_create_file(led_cdev->dev, &dev_attr_remove_port);
> +	if (err)
> +		goto err_remove_new_port;

Doesn't this race with userspace and fail?  Shouldn't the led core be
creating your leds for you?

> +
> +	/* Notifications */
> +	usbport_data->nb.notifier_call = usbport_trig_notify,
> +	led_cdev->trigger_data = usbport_data;
> +	usb_register_notify(&usbport_data->nb);

Don't abuse the USB notifier chain with stuff like this please, is that
really necessary?  Why can't your hardware just tell you what USB ports
are in use "out of band"?


> +
> +	led_cdev->activated = true;
> +	return;
> +
> +err_remove_new_port:
> +	device_remove_file(led_cdev->dev, &dev_attr_new_port);
> +err_put_ports:
> +	kobject_put(usbport_data->ports_dir);
> +err_free:
> +	kfree(usbport_data);
> +}

And again, why is this USB specific?  Why can't you use this same
userspace api and codebase for PCI ports?  For a sdcard port?  For a
thunderbolt port?

thanks,

greg k-h