Re: [PATCH 2/9] PM: suspend_block: Add driver to access suspend blockers from user-space

Arve Hjønnevåg
2009-05-07T01:42:59Z

On Tue, May 5, 2009 at 1:12 PM, Pavel Machek <pavel@ucw.cz> wrote:
> Hi!
>
>> Add a misc device, "suspend_blocker", that allows user-space processes
>> to block auto suspend. The device has ioctls to create a suspend_blocker,
>> and to block and unblock suspend. To delete the suspend_blocker, close
>> the device.
>
> Rafael proposed write() interface. I don't think you answered that.
>

I think an ioctl interface makes more sense. With a write interface
you either have to do string parsing, or invent some other protocol
between the driver and user-space.

>
>> +static int create_user_suspend_blocker(struct file *file, void __user *name,
>> +                              size_t name_len)
>> +{
>> +     struct user_suspend_blocker *bl;
>> +     if (file->private_data)
>> +             return -EBUSY;
>> +     bl = kzalloc(sizeof(*bl) + name_len + 1, GFP_KERNEL);
>> +     if (!bl)
>> +             return -ENOMEM;
>> +     if (copy_from_user(bl->name, name, name_len))
>> +             goto err_fault;
>> +     suspend_blocker_init(&bl->blocker, bl->name);
>> +     file->private_data = bl;
>> +     return 0;
>
> kzalloc on user-supplied argument. Sounds like bad idea.
>
> Aha. And probably integer overflow in the same line. Ouch.
>

The size is limited to _IOC_SIZEMASK with is 13 or 14 bits depending
on the architecture. Do you want a lower limit on the name length?

> Plus you actually 'trust' the string from userspace. Someone passes
> "evdev" there and can masquerade as a part of kernel.

Yes. I could add a prefix to userspace names, but it did not seem
worth the effort since the name is not used as a key for anything.

>
>> +static int user_suspend_blocker_release(struct inode *inode, struct file *file)
>> +{
>> +     struct user_suspend_blocker *bl = file->private_data;
>> +     if (!bl)
>> +             return 0;
>> +     suspend_blocker_destroy(&bl->blocker);
>> +     kfree(bl);
>> +     return 0;
>> +}
>
> What happens if someone does dup() on such file descriptor?
>

Then you have two file descriptors pointing to the same suspend
blocker. Release is called when the last reference goes away.

--
Arve Hjønnevåg