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.