On Nov 14, 2018, at 2:46 PM, Dmitry Torokhov [off-list ref] wrote:

quoted

On Wed, Nov 14, 2018 at 2:38 PM Jann Horn [off-list ref] wrote:

quoted

On Wed, Nov 14, 2018 at 11:29 PM Dmitry Torokhov [off-list ref] wrote:

quoted

On Wed, Nov 14, 2018 at 2:05 PM Jann Horn [off-list ref] wrote:

quoted

On Wed, Nov 14, 2018 at 10:55 PM Eric Biggers [off-list ref] 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.

[...]

quoted

quoted

quoted

diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c

[...]

quoted

quoted

quoted

@@ -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.