linux-kernel - Re: [PATCH] leds: Add mutex protection in brightness

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 9 Nov 2016 21:26:26 +0100
From:   Pavel Machek <pavel@....cz>
To:     Jacek Anaszewski <j.anaszewski@...sung.com>
Cc:     Hans de Goede <hdegoede@...hat.com>, linux-leds@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Sakari Ailus <sakari.ailus@...ux.intel.com>,
        Andrew Lunn <andrew@...n.ch>
Subject: Re: [PATCH] leds: Add mutex protection in brightness_show()

Hi!

> > Thanks for the analysis. Either way, this patch, with the modification
> > I mentioned in my previous message is required to assure proper
> > LED sysfs locking.
> > 
> > Regarding the races between user and atomic context, I think that
> > it should be system root's responsibility to define LED access
> > policy. If a LED is registered for any trigger events then setting
> > brightness from user space should be made impossible. Such a hint
> > could be even added to the Documentation/leds/leds-class.txt.
> 
> No, kernel locking may not depend on "root did not do anything
> stupid". Sorry.
> 
> Is there problem with led_access becoming a spinlock, and
> brightness_set_blocking taking it internally while it reads the
> brightness (but not while sleeping)? 

led_timer_function() does not seem to have any locking. IMO it needs
some, as it does not use atomic accesses.

Do we need a spinlock protecting led_classdev.flags and
delayed_set_value?

Would it be good idea to create new "blink_cancel" so brightness_set
is used just for .. brightness and not for timer manipulations?

Should we do something like this for consistency?

BTW how is locking expected to work with userland LED drivers? What if
userland LED driver accesses /sys files for its own LED? I'd really
prefer that patch not to be merged until we get locking right.

Thanks,
								Pavel

diff --git a/include/linux/leds.h b/include/linux/leds.h
index ddfcb2d..60e436d 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -88,11 +88,11 @@ struct led_classdev {
 
 	unsigned long		 blink_delay_on, blink_delay_off;
 	struct timer_list	 blink_timer;
-	int			 blink_brightness;
+	enum led_brightness      blink_brightness;
 	void			(*flash_resume)(struct led_classdev *led_cdev);
 
 	struct work_struct	set_brightness_work;
-	int			delayed_set_value;
+	enum led_brightness     delayed_set_value;
 
 #ifdef CONFIG_LEDS_TRIGGERS
 	/* Protects the trigger data below */

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Download attachment "signature.asc" of type "application/pgp-signature" (182 bytes)