Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1480207 > unrolled thread

[PATCH v3] leds: Introduce userspace leds driver

Back to article view | Back to linux.kernel

Contents

  [PATCH v3] leds: Introduce userspace leds driver David Lechner <david@lechnology.com> - 2016-09-09 19:00 +0200
    Re: [PATCH v3] leds: Introduce userspace leds driver David Lechner <david@lechnology.com> - 2016-09-09 22:50 +0200
    Re: [PATCH v3] leds: Introduce userspace leds driver Jacek Anaszewski <j.anaszewski@samsung.com> - 2016-09-12 10:20 +0200
      Re: [PATCH v3] leds: Introduce userspace leds driver David Lechner <david@lechnology.com> - 2016-09-12 17:00 +0200
      Re: [PATCH v3] leds: Introduce userspace leds driver Pavel Machek <pavel@ucw.cz> - 2016-09-15 15:10 +0200
        Re: [PATCH v3] leds: Introduce userspace leds driver Pavel Machek <pavel@ucw.cz> - 2016-09-15 15:40 +0200
          Re: [PATCH v3] leds: Introduce userspace leds driver Jacek Anaszewski <j.anaszewski@samsung.com> - 2016-09-15 17:00 +0200
            Re: [PATCH v3] leds: Introduce userspace leds driver Pavel Machek <pavel@ucw.cz> - 2016-09-16 08:20 +0200
        Re: [PATCH v3] leds: Introduce userspace leds driver Jacek Anaszewski <j.anaszewski@samsung.com> - 2016-09-15 17:00 +0200
          Re: [PATCH v3] leds: Introduce userspace leds driver David Lechner <david@lechnology.com> - 2016-09-15 17:40 +0200
            Re: [PATCH v3] leds: Introduce userspace leds driver David Lechner <david@lechnology.com> - 2016-09-15 17:40 +0200
            Re: [PATCH v3] leds: Introduce userspace leds driver Pavel Machek <pavel@ucw.cz> - 2016-09-16 08:00 +0200
              Re: [PATCH v3] leds: Introduce userspace leds driver David Lechner <david@lechnology.com> - 2016-09-16 17:40 +0200
            Re: [PATCH v3] leds: Introduce userspace leds driver Pavel Machek <pavel@ucw.cz> - 2016-09-16 08:00 +0200
              Re: [PATCH v3] leds: Introduce userspace leds driver David Lechner <david@lechnology.com> - 2016-09-16 17:20 +0200
            Re: [PATCH v3] leds: Introduce userspace leds driver Pavel Machek <pavel@ucw.cz> - 2016-09-16 08:10 +0200
              Re: [PATCH v3] leds: Introduce userspace leds driver David Lechner <david@lechnology.com> - 2016-09-16 17:50 +0200
          Re: [PATCH v3] leds: Introduce userspace leds driver David Lechner <david@lechnology.com> - 2016-09-15 18:40 +0200
            Re: [PATCH v3] leds: Introduce userspace leds driver Pavel Machek <pavel@ucw.cz> - 2016-09-16 08:00 +0200
              Re: [PATCH v3] leds: Introduce userspace leds driver Jacek Anaszewski <j.anaszewski@samsung.com> - 2016-09-16 09:10 +0200
                Re: [PATCH v3] leds: Introduce userspace leds driver David Lechner <david@lechnology.com> - 2016-09-16 17:20 +0200
                  Re: [PATCH v3] leds: Introduce userspace leds driver Pavel Machek <pavel@ucw.cz> - 2016-09-16 21:40 +0200

Page 2 of 2 — ← Prev page 1 [2]

#1485108

On 09/16/2016 02:07 AM, Jacek Anaszewski wrote:
> On 09/16/2016 07:50 AM, Pavel Machek wrote:
>> Hi!
>>
>>>>>>> +    if (copy_from_user(&udev->user_dev, buffer,
>>>>>>> +               sizeof(struct uleds_user_dev))) {
>>>>>>> +        ret = -EFAULT;
>>>>>>> +        goto out;
>>>>>>> +    }
>>>>>>> +
>>>>>>> +    if (!udev->user_dev.name[0]) {
>>>>>>> +        ret = -EINVAL;
>>>>>>> +        goto out;
>>>>>>> +    }
>>>>>>> +
>>>>>>> +    ret = led_classdev_register(NULL, &udev->led_cdev);
>>>>>>> +    if (ret < 0)
>>>>>>> +        goto out;
>>>>>
>>>>> No sanity checking on the name -> probably a security hole. Do not
>>>>> push this upstream before this is fixed.
>>>>
>>>
>>> If this is a serious security issue, then you should also raise an issue
>>> with input maintainers because this is the extent of sanity checking for
>>> uinput device names as well.
>>
>> I guess that should be fixed. But lets not add new ones.
>>
>>> I must confess that I am no security expert, so unless you can give
>>> specific
>>> examples of what potential threats are, I will not be able to guess
>>> what I
>>> need to do to fix it.
>>>
>>> After some digging around the kernel, I don't see many instances of
>>> validating device node names. The best I have found so far comes from
>>> create_entry() in binfmt_misc.c
>>>
>>>     if (!e->name[0] ||
>>>         !strcmp(e->name, ".") ||
>>>         !strcmp(e->name, "..") ||
>>>         strchr(e->name, '/'))
>>>         goto einval;
>>>
>>> Would something like this be a sufficient sanity check? I suppose we
>>> could
>>> also check for non-printing characters, but I don't think ignoring them
>>> would be a security issue.
>>
>> That would be minimum, yes. I guess it would be better/easier to just
>> limit the names to [a-zA-Z:-_0-9]*?
>
> Right, and we also could check if there are no more then two ":"
> characters in the name.
>

Again, I am going to disagree here. docs/sysfs-rules.txt says nothing 
about restricting characters for device names, so I don't think we 
should do so here. In fact, the only thing it says about names is 
"applications need to handle spaces and characters like '!' in the 
name". My opinion is that if people want to give devices dumb names with 
special characters and spaces, we should let them.

If someone can point out a real security issue here, then I will gladly 
fix it, otherwise I am inclined to leave it as it is (with the checks 
for '.', '..' and '/').

If this was a regular userspace library, I would feel differently, but 
since the kernel has limited means to pass errors to userspace, all of 
these checks will pass the same -EINVAL to userspace if they fail. We 
could print error messages to the kernel log, but it is really annoying 
to have to check the kernel log to find out why your userspace 
application is not working. Any if you are not a kernel hacker, you 
would probably not even know to check the kernel logs.

[toc] | [prev] | [next] | [standalone]

#1485273

Hi!

> >>>After some digging around the kernel, I don't see many instances of
> >>>validating device node names. The best I have found so far comes from
> >>>create_entry() in binfmt_misc.c
> >>>
> >>>    if (!e->name[0] ||
> >>>        !strcmp(e->name, ".") ||
> >>>        !strcmp(e->name, "..") ||
> >>>        strchr(e->name, '/'))
> >>>        goto einval;
> >>>
> >>>Would something like this be a sufficient sanity check? I suppose we
> >>>could
> >>>also check for non-printing characters, but I don't think ignoring them
> >>>would be a security issue.
> >>
> >>That would be minimum, yes. I guess it would be better/easier to just
> >>limit the names to [a-zA-Z:-_0-9]*?
> >
> >Right, and we also could check if there are no more then two ":"
> >characters in the name.
> >
> 
> Again, I am going to disagree here. docs/sysfs-rules.txt says nothing about
> restricting characters for device names, so I don't think we should do so
> here. In fact, the only thing it says about names is "applications need to
> handle spaces and characters like '!' in the name". My opinion is that if
> people want to give devices dumb names with special characters and spaces,
> we should let them.

You should be able to emulate your leds on the rapsperry pi. So
checking number of :'s does not make sense.

OTOH having a LED called ^[c that clears your screen, or having
invalid utf-8 in name .. is just going to cause problems for someone,
somewhere. Perhaps you can even use mouse reporting escape sequences
to prepare some nice surprise for admin doing "dmesg". Don't go
there.. please.

> If someone can point out a real security issue here, then I will gladly fix
> it, otherwise I am inclined to leave it as it is (with the checks for '.',
> '..' and '/').

Thanks for those checks.

But I'd really disallow control characters (<0x20), space and
non-ascii stuff (>0x7f). Yes, userspace _should_ handle that ok, but
device names usually don't contain crazy characters, I'm pretty sure
there is printk() with something like that (which would have sideeffects)..

> If this was a regular userspace library, I would feel differently, but since
> the kernel has limited means to pass errors to userspace, all of these
> checks will pass the same -EINVAL to userspace if they fail. We could print
> error messages to the kernel log, but it is really annoying to have to check
> the kernel log to find out why your userspace application is not working.
> Any if you are not a kernel hacker, you would probably not even know to
> check the kernel logs.

People doing device drivers normally know about printk()... (and I
don't expect people to hit the name limits too often.)

But people are normally careless, and do dangerous stuff such as
"dmesg" and "ls /sys/class/leds". If those can contain crazy
characters, bad things can happen.

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

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web