linux-kernel - Re: [PATCH] Add LED pattern 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 for Android: free password hash cracker in your pocket

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

Date:	Tue, 31 Dec 2013 00:18:40 +0000
From:	One Thousand Gnomes <gnomes@...rguk.ukuu.org.uk>
To:	Joe Xue <lgxue@...mail.com>
Cc:	"cooloney@...il.com" <cooloney@...il.com>,
	"rpurdie@...ys.net" <rpurdie@...ys.net>,
	"rob@...dley.net" <rob@...dley.net>,
	"milo.kim@...com" <milo.kim@...com>, "pavel@....cz" <pavel@....cz>,
	"linux-leds@...r.kernel.org" <linux-leds@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>
Subject: Re: [PATCH] Add LED pattern trigger

On Mon, 30 Dec 2013 18:24:51 -0500
Joe Xue <lgxue@...mail.com> wrote:

> >> +#define MAX_PATTEN_LEN 255
> >
> > Arbitary limits that are not needed if it was in userspace, and not it
> > seems a sensible one - why not use 256 ?
> 
> The maximum memory is 256, we keep one for '\0'

Why - you have pattern_len already ?

> >> + del_timer_sync(&pattern_data->timer);
> >> +
> >> + memcpy(pattern_data->pattern, buf, len);
> >> + pattern_data->pattern[len] = '\0';
> >> + pattern_data->pattern_len = len;
> >> + pattern_data->count = 0;
> >> +
> >> + mod_timer(&pattern_data->timer, jiffies + 1);
> >
> > What if the pattern isn't currently active ?
> 
> Doesn't matter as per my test.

What test - you've not explained why it doesn't blow up ?

> > Why +1, you don't need a zero terminator you know the length
> >
> > Why allocate a fixed 256 byte blob when you can make the data the end of
> > the struct (ie pattern[0] in the declaration) and not waste memory.
> 
> This just easy for patten_show.

Even so the fixed 256 is broken - you can just allocate the proper size
for the pattern when you set it up.
 
> >> +static void pattern_trig_deactivate(struct led_classdev *led_cdev)
> >> +{
> >> + struct pattern_trig_data *pattern_data = led_cdev->trigger_data;
> >> +
> >> + if (led_cdev->activated) {
> >> + del_timer_sync(&pattern_data->timer);
> >> + device_remove_file(led_cdev->dev, &dev_attr_pattern);
> >> + device_remove_file(led_cdev->dev, &dev_attr_delay_unit);
> >
> > This doesn't as far as I can see do what you think. If I have the file
> > currently open then device_remove_file will not remove my existing access
> > to it, but you just released the pattern data so I now write to free
> > memory.
> 
> I believe kernel will handle this

I looked through the code paths and I see nothing to protect this at all ?

> >> + led_cdev->trigger_data = NULL;
> >> + led_cdev->activated = false;
> >> + kfree(pattern_data);
> >> + }
> >> + __led_set_brightness(led_cdev, LED_OFF);
> >> +}
> >> +
> >> +static struct led_trigger pattern_trigger = {
> >
> > const ?
> 

You don't want function pointers in objects that are not const, at least
whenever possible. On processors that have suitable protection modes that
helps prevent attacks based upon patching kernel function pointers.


Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/