Re: [PATCH v2] HID: uhid: forbid UHID_CREATE under KERNEL_DS or elevated privileges

[Andy Lutomirski <luto@amacapital.net>]

> On Nov 14, 2018, at 2:46 PM, Dmitry Torokhov <dtor@google.com> wrote:
> 
>> On Wed, Nov 14, 2018 at 2:38 PM Jann Horn <jannh@google.com> wrote:
>> 
>>> On Wed, Nov 14, 2018 at 11:29 PM Dmitry Torokhov <dtor@google.com> wrote:
>>>> On Wed, Nov 14, 2018 at 2:05 PM Jann Horn <jannh@google.com> wrote:
>>>>> On Wed, Nov 14, 2018 at 10:55 PM Eric Biggers <ebiggers@kernel.org> wrote:
>>>>> When a UHID_CREATE command is written to the uhid char device, a
>>>>> copy_from_user() is done from a user pointer embedded in the command.
>>>>> When the address limit is KERNEL_DS, e.g. as is the case during
>>>>> sys_sendfile(), this can read from kernel memory.  Alternatively,
>>>>> information can be leaked from a setuid binary that is tricked to write
>>>>> to the file descriptor.  Therefore, forbid UHID_CREATE in these cases.
>>>>> 
>>>>> No other commands in uhid_char_write() are affected by this bug and
>>>>> UHID_CREATE is marked as "obsolete", so apply the restriction to
>>>>> UHID_CREATE only rather than to uhid_char_write() entirely.
>> [...]
>>>>> diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
>> [...]
>>>>> @@ -722,6 +723,17 @@ static ssize_t uhid_char_write(struct file *file, const char __user *buffer,
>>>>> 
>>>>>        switch (uhid->input_buf.type) {
>>>>>        case UHID_CREATE:
>>>>> +               /*
>>>>> +                * 'struct uhid_create_req' contains a __user pointer which is
>>>>> +                * copied from, so it's unsafe to allow this with elevated
>>>>> +                * privileges (e.g. from a setuid binary) or via kernel_write().
>>>>> +                */
>>> 
>>> uhid is a privileged interface so we would go from root to less
>>> privileged (if at all). If non-privileged process can open uhid it can
>>> construct virtual keyboard and inject whatever keystrokes it wants.
>>> 
>>> Also, instead of disallowing access, can we ensure that we switch back
>>> to USER_DS before trying to load data from the user pointer?
>> 
>> Does that even make sense? You are using some deprecated legacy
>> interface; you interact with it by splicing a request from something
>> like a file or a pipe into the uhid device; but the request you're
>> splicing through contains a pointer into userspace memory? Do you know
>> of anyone who is actually doing that? If not, anyone who does want to
>> do this for some reason in the future can just go use UHID_CREATE2
>> instead.
> 
> I do not know if anyone is still using UHID_CREATE with sendpage and
> neither do you really. It is all about not breaking userspace without
> good reason and here ensuring that we switch to USER_DS and then back
> to whatever it was does not seem too hard.

It’s about not breaking userspace *except as needed for security fixes*. User pointers in a write() payload is a big no-no.

Also, that f_cred hack is only barely enough. This isn’t just about attacking suid things — this bug allows poking at the address space of an unsuspecting process. So, if a privileged program opens a uhid fd and is then tricked into writing untrusted data to the same fd (which is supposed to be safe), then we have a problem. Fortunately, identically privileged programs usually still don’t share a cred pointer unless they came from the right place.

The real right fix is to remove UHID_CREATE outright. This is terminally broken.