From: Eric Biggers ebiggers@google.com
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 sendfile(), this can read from kernel memory. Therefore, UHID_CREATE must not be allowed in this case.
For consistency and to make sure all current and future uhid commands are covered, apply the restriction to uhid_char_write() as a whole rather than to UHID_CREATE specifically.
Thanks to Dmitry Vyukov for adding uhid definitions to syzkaller and to Jann Horn for commit 9da3f2b740544 ("x86/fault: BUG() when uaccess helpers fault on kernel addresses"), allowing this bug to be found.
Reported-by: syzbot+72473edc9bf4eb1c6556@syzkaller.appspotmail.com Fixes: d365c6cfd337 ("HID: uhid: add UHID_CREATE and UHID_DESTROY events") Cc: stable@vger.kernel.org # v3.6+ Cc: Jann Horn jannh@google.com Signed-off-by: Eric Biggers ebiggers@google.com --- drivers/hid/uhid.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c index 3c55073136064..e94c5e248b56e 100644 --- a/drivers/hid/uhid.c +++ b/drivers/hid/uhid.c @@ -705,6 +705,12 @@ static ssize_t uhid_char_write(struct file *file, const char __user *buffer, int ret; size_t len;
+ if (uaccess_kernel()) { /* payload may contain a __user pointer */ + pr_err_once("%s: process %d (%s) called from kernel context, this is not allowed.\n", + __func__, task_tgid_vnr(current), current->comm); + return -EACCES; + } + /* we need at least the "type" member of uhid_event */ if (count < sizeof(__u32)) return -EINVAL;
On Wed, Nov 14, 2018 at 10:03 AM Eric Biggers ebiggers@kernel.org wrote:
From: Eric Biggers ebiggers@google.com
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 sendfile(), this can read from kernel memory. Therefore, UHID_CREATE must not be allowed in this case.
Hmm, instead of disallowing access, can we switch back to USER_DS before trying to use the user pointer?
For consistency and to make sure all current and future uhid commands are covered, apply the restriction to uhid_char_write() as a whole rather than to UHID_CREATE specifically.
Thanks to Dmitry Vyukov for adding uhid definitions to syzkaller and to Jann Horn for commit 9da3f2b740544 ("x86/fault: BUG() when uaccess helpers fault on kernel addresses"), allowing this bug to be found.
Reported-by: syzbot+72473edc9bf4eb1c6556@syzkaller.appspotmail.com Fixes: d365c6cfd337 ("HID: uhid: add UHID_CREATE and UHID_DESTROY events") Cc: stable@vger.kernel.org # v3.6+ Cc: Jann Horn jannh@google.com Signed-off-by: Eric Biggers ebiggers@google.com
drivers/hid/uhid.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c index 3c55073136064..e94c5e248b56e 100644 --- a/drivers/hid/uhid.c +++ b/drivers/hid/uhid.c @@ -705,6 +705,12 @@ static ssize_t uhid_char_write(struct file *file, const char __user *buffer, int ret; size_t len;
if (uaccess_kernel()) { /* payload may contain a __user pointer */pr_err_once("%s: process %d (%s) called from kernel context, this is not allowed.\n",__func__, task_tgid_vnr(current), current->comm);return -EACCES;}/* we need at least the "type" member of uhid_event */ if (count < sizeof(__u32)) return -EINVAL;-- 2.19.1.930.g4563a0d9d0-goog
+cc Andy
On Wed, Nov 14, 2018 at 7:03 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 sendfile(), this can read from kernel memory. Therefore, UHID_CREATE must not be allowed in this case.
For consistency and to make sure all current and future uhid commands are covered, apply the restriction to uhid_char_write() as a whole rather than to UHID_CREATE specifically.
Thanks to Dmitry Vyukov for adding uhid definitions to syzkaller and to Jann Horn for commit 9da3f2b740544 ("x86/fault: BUG() when uaccess helpers fault on kernel addresses"), allowing this bug to be found.
Reported-by: syzbot+72473edc9bf4eb1c6556@syzkaller.appspotmail.com
Wheeeee, it found something! :)
Fixes: d365c6cfd337 ("HID: uhid: add UHID_CREATE and UHID_DESTROY events") Cc: stable@vger.kernel.org # v3.6+ Cc: Jann Horn jannh@google.com Signed-off-by: Eric Biggers ebiggers@google.com
drivers/hid/uhid.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c index 3c55073136064..e94c5e248b56e 100644 --- a/drivers/hid/uhid.c +++ b/drivers/hid/uhid.c @@ -705,6 +705,12 @@ static ssize_t uhid_char_write(struct file *file, const char __user *buffer, int ret; size_t len;
if (uaccess_kernel()) { /* payload may contain a __user pointer */pr_err_once("%s: process %d (%s) called from kernel context, this is not allowed.\n",__func__, task_tgid_vnr(current), current->comm);return -EACCES;}
If this file can conceivably be opened by a process that doesn't have root privileges, this check should be something along the lines of ib_safe_file_access() or sg_check_file_access().
Checking for uaccess_kernel() prevents the symptom that syzkaller notices - a user being able to cause a kernel memory access -, but it doesn't deal with the case where a user opens a file descriptor to this device and tricks a more privileged process into writing into it (e.g. by passing it to a suid binary as stdout or stderr).
Looking closer, I wonder whether this kind of behavior is limited to the UHID_CREATE request, which has a comment on it saying "/* Obsolete! Use UHID_CREATE2. */". If we could keep this kind of ugly kludge away from the code paths you're supposed to be using, that would be nice...
On Wed, Nov 14, 2018 at 07:18:39PM +0100, 'Jann Horn' via syzkaller-bugs wrote:
+cc Andy
On Wed, Nov 14, 2018 at 7:03 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 sendfile(), this can read from kernel memory. Therefore, UHID_CREATE must not be allowed in this case.
For consistency and to make sure all current and future uhid commands are covered, apply the restriction to uhid_char_write() as a whole rather than to UHID_CREATE specifically.
Thanks to Dmitry Vyukov for adding uhid definitions to syzkaller and to Jann Horn for commit 9da3f2b740544 ("x86/fault: BUG() when uaccess helpers fault on kernel addresses"), allowing this bug to be found.
Reported-by: syzbot+72473edc9bf4eb1c6556@syzkaller.appspotmail.com
Wheeeee, it found something! :)
Fixes: d365c6cfd337 ("HID: uhid: add UHID_CREATE and UHID_DESTROY events") Cc: stable@vger.kernel.org # v3.6+ Cc: Jann Horn jannh@google.com Signed-off-by: Eric Biggers ebiggers@google.com
drivers/hid/uhid.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c index 3c55073136064..e94c5e248b56e 100644 --- a/drivers/hid/uhid.c +++ b/drivers/hid/uhid.c @@ -705,6 +705,12 @@ static ssize_t uhid_char_write(struct file *file, const char __user *buffer, int ret; size_t len;
if (uaccess_kernel()) { /* payload may contain a __user pointer */pr_err_once("%s: process %d (%s) called from kernel context, this is not allowed.\n",__func__, task_tgid_vnr(current), current->comm);return -EACCES;}If this file can conceivably be opened by a process that doesn't have root privileges, this check should be something along the lines of ib_safe_file_access() or sg_check_file_access().
Checking for uaccess_kernel() prevents the symptom that syzkaller notices - a user being able to cause a kernel memory access -, but it doesn't deal with the case where a user opens a file descriptor to this device and tricks a more privileged process into writing into it (e.g. by passing it to a suid binary as stdout or stderr).
Yep, I'll do that.
Looking closer, I wonder whether this kind of behavior is limited to the UHID_CREATE request, which has a comment on it saying "/* Obsolete! Use UHID_CREATE2. */". If we could keep this kind of ugly kludge away from the code paths you're supposed to be using, that would be nice...
I wanted to be careful, but yes AFAICS it can be limited to UHID_CREATE only, so I'll do that instead.
- Eric
From: Eric Biggers ebiggers@google.com
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.
Thanks to Dmitry Vyukov for adding uhid definitions to syzkaller and to Jann Horn for commit 9da3f2b740544 ("x86/fault: BUG() when uaccess helpers fault on kernel addresses"), allowing this bug to be found.
Reported-by: syzbot+72473edc9bf4eb1c6556@syzkaller.appspotmail.com Fixes: d365c6cfd337 ("HID: uhid: add UHID_CREATE and UHID_DESTROY events") Cc: stable@vger.kernel.org # v3.6+ Cc: Jann Horn jannh@google.com Cc: Andy Lutomirski luto@kernel.org Signed-off-by: Eric Biggers ebiggers@google.com --- drivers/hid/uhid.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c index 3c55073136064..051639c09f728 100644 --- a/drivers/hid/uhid.c +++ b/drivers/hid/uhid.c @@ -12,6 +12,7 @@
#include <linux/atomic.h> #include <linux/compat.h> +#include <linux/cred.h> #include <linux/device.h> #include <linux/fs.h> #include <linux/hid.h> @@ -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(). + */ + if (file->f_cred != current_cred() || uaccess_kernel()) { + pr_err_once("UHID_CREATE from different security context by process %d (%s), this is not allowed.\n", + task_tgid_vnr(current), current->comm); + ret = -EACCES; + goto unlock; + } ret = uhid_dev_create(uhid, &uhid->input_buf); break; case UHID_CREATE2:
On Wed, Nov 14, 2018 at 10:55 PM Eric Biggers ebiggers@kernel.org wrote:
From: Eric Biggers ebiggers@google.com
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.
Thanks to Dmitry Vyukov for adding uhid definitions to syzkaller and to Jann Horn for commit 9da3f2b740544 ("x86/fault: BUG() when uaccess helpers fault on kernel addresses"), allowing this bug to be found.
Reported-by: syzbot+72473edc9bf4eb1c6556@syzkaller.appspotmail.com Fixes: d365c6cfd337 ("HID: uhid: add UHID_CREATE and UHID_DESTROY events") Cc: stable@vger.kernel.org # v3.6+ Cc: Jann Horn jannh@google.com Cc: Andy Lutomirski luto@kernel.org Signed-off-by: Eric Biggers ebiggers@google.com
Reviewed-by: Jann Horn jannh@google.com
drivers/hid/uhid.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c index 3c55073136064..051639c09f728 100644 --- a/drivers/hid/uhid.c +++ b/drivers/hid/uhid.c @@ -12,6 +12,7 @@
#include <linux/atomic.h> #include <linux/compat.h> +#include <linux/cred.h> #include <linux/device.h> #include <linux/fs.h> #include <linux/hid.h> @@ -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().*/if (file->f_cred != current_cred() || uaccess_kernel()) {pr_err_once("UHID_CREATE from different security context by process %d (%s), this is not allowed.\n",task_tgid_vnr(current), current->comm);ret = -EACCES;goto unlock;} ret = uhid_dev_create(uhid, &uhid->input_buf); break; case UHID_CREATE2:-- 2.19.1.930.g4563a0d9d0-goog
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:
From: Eric Biggers ebiggers@google.com
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.
Thanks to Dmitry Vyukov for adding uhid definitions to syzkaller and to Jann Horn for commit 9da3f2b740544 ("x86/fault: BUG() when uaccess helpers fault on kernel addresses"), allowing this bug to be found.
Reported-by: syzbot+72473edc9bf4eb1c6556@syzkaller.appspotmail.com Fixes: d365c6cfd337 ("HID: uhid: add UHID_CREATE and UHID_DESTROY events") Cc: stable@vger.kernel.org # v3.6+ Cc: Jann Horn jannh@google.com Cc: Andy Lutomirski luto@kernel.org Signed-off-by: Eric Biggers ebiggers@google.com
Reviewed-by: Jann Horn jannh@google.com
drivers/hid/uhid.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c index 3c55073136064..051639c09f728 100644 --- a/drivers/hid/uhid.c +++ b/drivers/hid/uhid.c @@ -12,6 +12,7 @@
#include <linux/atomic.h> #include <linux/compat.h> +#include <linux/cred.h> #include <linux/device.h> #include <linux/fs.h> #include <linux/hid.h> @@ -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?
if (file->f_cred != current_cred() || uaccess_kernel()) {pr_err_once("UHID_CREATE from different security context by process %d (%s), this is not allowed.\n",task_tgid_vnr(current), current->comm);ret = -EACCES;goto unlock;} ret = uhid_dev_create(uhid, &uhid->input_buf); break; case UHID_CREATE2:-- 2.19.1.930.g4563a0d9d0-goog
Thanks.
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.
if (file->f_cred != current_cred() || uaccess_kernel()) {pr_err_once("UHID_CREATE from different security context by process %d (%s), this is not allowed.\n",task_tgid_vnr(current), current->comm);ret = -EACCES;goto unlock;} ret = uhid_dev_create(uhid, &uhid->input_buf); break; case UHID_CREATE2:
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.
if (file->f_cred != current_cred() || uaccess_kernel()) {pr_err_once("UHID_CREATE from different security context by process %d (%s), this is not allowed.\n",task_tgid_vnr(current), current->comm);ret = -EACCES;goto unlock;} ret = uhid_dev_create(uhid, &uhid->input_buf); break; case UHID_CREATE2:
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.
Hi Dmitry,
On Wed, Nov 14, 2018 at 02:28:56PM -0800, 'Dmitry Torokhov' via syzkaller-bugs 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:
From: Eric Biggers ebiggers@google.com
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.
Thanks to Dmitry Vyukov for adding uhid definitions to syzkaller and to Jann Horn for commit 9da3f2b740544 ("x86/fault: BUG() when uaccess helpers fault on kernel addresses"), allowing this bug to be found.
Reported-by: syzbot+72473edc9bf4eb1c6556@syzkaller.appspotmail.com Fixes: d365c6cfd337 ("HID: uhid: add UHID_CREATE and UHID_DESTROY events") Cc: stable@vger.kernel.org # v3.6+ Cc: Jann Horn jannh@google.com Cc: Andy Lutomirski luto@kernel.org Signed-off-by: Eric Biggers ebiggers@google.com
Reviewed-by: Jann Horn jannh@google.com
drivers/hid/uhid.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c index 3c55073136064..051639c09f728 100644 --- a/drivers/hid/uhid.c +++ b/drivers/hid/uhid.c @@ -12,6 +12,7 @@
#include <linux/atomic.h> #include <linux/compat.h> +#include <linux/cred.h> #include <linux/device.h> #include <linux/fs.h> #include <linux/hid.h> @@ -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?
Actually uhid doesn't have any capability checks, so it's up to userspace to assign permissions to the device node. I think it's best not to make assumptions about how the interface will be used and to be consistent with how other ->write() methods in the kernel handle the misfeature where a __user pointer in the write() or read() payload is dereferenced. Temporarily switching to USER_DS would only avoid one of the two problems.
Do you think the proposed restrictions would actually break anything?
- Eric
if (file->f_cred != current_cred() || uaccess_kernel()) {pr_err_once("UHID_CREATE from different security context by process %d (%s), this is not allowed.\n",task_tgid_vnr(current), current->comm);ret = -EACCES;goto unlock;} ret = uhid_dev_create(uhid, &uhid->input_buf); break; case UHID_CREATE2:--
On Wed, Nov 14, 2018 at 3:00 PM Eric Biggers ebiggers@kernel.org wrote:
Hi Dmitry,
On Wed, Nov 14, 2018 at 02:28:56PM -0800, 'Dmitry Torokhov' via syzkaller-bugs 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:
From: Eric Biggers ebiggers@google.com
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.
Thanks to Dmitry Vyukov for adding uhid definitions to syzkaller and to Jann Horn for commit 9da3f2b740544 ("x86/fault: BUG() when uaccess helpers fault on kernel addresses"), allowing this bug to be found.
Reported-by: syzbot+72473edc9bf4eb1c6556@syzkaller.appspotmail.com Fixes: d365c6cfd337 ("HID: uhid: add UHID_CREATE and UHID_DESTROY events") Cc: stable@vger.kernel.org # v3.6+ Cc: Jann Horn jannh@google.com Cc: Andy Lutomirski luto@kernel.org Signed-off-by: Eric Biggers ebiggers@google.com
Reviewed-by: Jann Horn jannh@google.com
drivers/hid/uhid.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c index 3c55073136064..051639c09f728 100644 --- a/drivers/hid/uhid.c +++ b/drivers/hid/uhid.c @@ -12,6 +12,7 @@
#include <linux/atomic.h> #include <linux/compat.h> +#include <linux/cred.h> #include <linux/device.h> #include <linux/fs.h> #include <linux/hid.h> @@ -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?
Actually uhid doesn't have any capability checks, so it's up to userspace to assign permissions to the device node.
Yes. There are quite a few such instances where kernel does not bother implementing superfluous checks and instead relies on userspace to provide sane environment. IIRC nobody in the kernel enforces root filesystem not be accessible to ordinary users, it is done with generic permission checks.
I think it's best not to make assumptions about how the interface will be used and to be consistent with how other ->write() methods in the kernel handle the misfeature where a __user pointer in the write() or read() payload is dereferenced.
I can see that you might want to check credentials, etc, if interface can be accessed by unprivileged process, however is it a big no no for uhid/userio/uinput devices.
Temporarily switching to USER_DS would only avoid one of the two problems.
So because of the above there is only one problem. If your system opened access to uhid to random processes you have much bigger problems than exposing some data from a suid binary. You can spam "rm -rf .; rm -rf /" though uhid if there is interactive session somewhere.
Do you think the proposed restrictions would actually break anything?
It would break if someone uses UHID_CREATE with sendpage. I do not know if anyone does. If we were certain there are no users we'd simply removed UHID_CREATE altogether.
- Eric
if (file->f_cred != current_cred() || uaccess_kernel()) {pr_err_once("UHID_CREATE from different security context by process %d (%s), this is not allowed.\n",task_tgid_vnr(current), current->comm);ret = -EACCES;goto unlock;} ret = uhid_dev_create(uhid, &uhid->input_buf); break; case UHID_CREATE2:--
Thanks.
On Thu, Nov 15, 2018 at 12:20 AM Dmitry Torokhov dtor@google.com wrote:
On Wed, Nov 14, 2018 at 3:00 PM Eric Biggers ebiggers@kernel.org wrote:
Hi Dmitry,
On Wed, Nov 14, 2018 at 02:28:56PM -0800, 'Dmitry Torokhov' via syzkaller-bugs 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:
From: Eric Biggers ebiggers@google.com
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.
Thanks to Dmitry Vyukov for adding uhid definitions to syzkaller and to Jann Horn for commit 9da3f2b740544 ("x86/fault: BUG() when uaccess helpers fault on kernel addresses"), allowing this bug to be found.
Reported-by: syzbot+72473edc9bf4eb1c6556@syzkaller.appspotmail.com Fixes: d365c6cfd337 ("HID: uhid: add UHID_CREATE and UHID_DESTROY events") Cc: stable@vger.kernel.org # v3.6+ Cc: Jann Horn jannh@google.com Cc: Andy Lutomirski luto@kernel.org Signed-off-by: Eric Biggers ebiggers@google.com
Reviewed-by: Jann Horn jannh@google.com
drivers/hid/uhid.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c index 3c55073136064..051639c09f728 100644 --- a/drivers/hid/uhid.c +++ b/drivers/hid/uhid.c @@ -12,6 +12,7 @@
#include <linux/atomic.h> #include <linux/compat.h> +#include <linux/cred.h> #include <linux/device.h> #include <linux/fs.h> #include <linux/hid.h> @@ -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?
Actually uhid doesn't have any capability checks, so it's up to userspace to assign permissions to the device node.
Yes. There are quite a few such instances where kernel does not bother implementing superfluous checks and instead relies on userspace to provide sane environment. IIRC nobody in the kernel enforces root filesystem not be accessible to ordinary users, it is done with generic permission checks.
I think it's best not to make assumptions about how the interface will be used and to be consistent with how other ->write() methods in the kernel handle the misfeature where a __user pointer in the write() or read() payload is dereferenced.
I can see that you might want to check credentials, etc, if interface can be accessed by unprivileged process, however is it a big no no for uhid/userio/uinput devices.
Yep, any sane distribution would restrict the permissions of uhid/userio/uinput to only be accessed by root. If that ever changes, there is already an issue with the system and it was compromised either by a terribly dizzy sysadmin.
Temporarily switching to USER_DS would only avoid one of the two problems.
So because of the above there is only one problem. If your system opened access to uhid to random processes you have much bigger problems than exposing some data from a suid binary. You can spam "rm -rf .; rm -rf /" though uhid if there is interactive session somewhere.
Do you think the proposed restrictions would actually break anything?
It would break if someone uses UHID_CREATE with sendpage. I do not know if anyone does. If we were certain there are no users we'd simply removed UHID_CREATE altogether.
AFAICT, there are 2 users of uhid: - bluez for BLE devices (using HID over GATT) - hid-replay for debugging.
There might be a few other users that are making some user space driver out of opencv, but I wouldn't expect those to be really widespread.
IIRC, bluez uses UHID_CREATE2 and hid-replay should also (or ought to be, but this can be easily fixed as I maintain the code and I am the almost sole user).
Regarding the sendpage fix, doesn't David's patch fixes the issue already (https://patchwork.kernel.org/patch/10682549/).
I am fine applying whatever patch that fixes the security issues, as long as it doesn't break bluez nor the hid-replay uses I have for debugging and regression testing.
Cheers, Benjamin
- Eric
if (file->f_cred != current_cred() || uaccess_kernel()) {pr_err_once("UHID_CREATE from different security context by process %d (%s), this is not allowed.\n",task_tgid_vnr(current), current->comm);ret = -EACCES;goto unlock;} ret = uhid_dev_create(uhid, &uhid->input_buf); break; case UHID_CREATE2:--
Thanks.
-- Dmitry
Hey
On Thu, Nov 15, 2018 at 9:14 AM Benjamin Tissoires benjamin.tissoires@redhat.com wrote:
On Thu, Nov 15, 2018 at 12:20 AM Dmitry Torokhov dtor@google.com wrote:
I think it's best not to make assumptions about how the interface will be used and to be consistent with how other ->write() methods in the kernel handle the misfeature where a __user pointer in the write() or read() payload is dereferenced.
I can see that you might want to check credentials, etc, if interface can be accessed by unprivileged process, however is it a big no no for uhid/userio/uinput devices.
Yep, any sane distribution would restrict the permissions of uhid/userio/uinput to only be accessed by root. If that ever changes, there is already an issue with the system and it was compromised either by a terribly dizzy sysadmin.
UHID is safe to be used by a non-root user. This does not imply that you should open up access to the world, but you are free to have a dedicated group or user with access to uhid. I agree that in most common desktop-scenarios you should not grant world-access to it, though.
Temporarily switching to USER_DS would only avoid one of the two problems.
So because of the above there is only one problem. If your system opened access to uhid to random processes you have much bigger problems than exposing some data from a suid binary. You can spam "rm -rf .; rm -rf /" though uhid if there is interactive session somewhere.
Do you think the proposed restrictions would actually break anything?
It would break if someone uses UHID_CREATE with sendpage. I do not know if anyone does. If we were certain there are no users we'd simply removed UHID_CREATE altogether.
AFAICT, there are 2 users of uhid:
- bluez for BLE devices (using HID over GATT)
- hid-replay for debugging.
There are several more (eg., android bt-broadcom), and UHID_CREATE is actively used. Dropping support for it will break these use-cases.
Thanks David
On Nov 15, 2018, at 4:06 AM, David Herrmann dh.herrmann@gmail.com wrote:
Hey
On Thu, Nov 15, 2018 at 9:14 AM Benjamin Tissoires benjamin.tissoires@redhat.com wrote:
On Thu, Nov 15, 2018 at 12:20 AM Dmitry Torokhov dtor@google.com wrote:
I think it's best not to make assumptions about how the interface will be used and to be consistent with how other ->write() methods in the kernel handle the misfeature where a __user pointer in the write() or read() payload is dereferenced.
I can see that you might want to check credentials, etc, if interface can be accessed by unprivileged process, however is it a big no no for uhid/userio/uinput devices.
Yep, any sane distribution would restrict the permissions of uhid/userio/uinput to only be accessed by root. If that ever changes, there is already an issue with the system and it was compromised either by a terribly dizzy sysadmin.
UHID is safe to be used by a non-root user. This does not imply that you should open up access to the world, but you are free to have a dedicated group or user with access to uhid. I agree that in most common desktop-scenarios you should not grant world-access to it, though.
Temporarily switching to USER_DS would only avoid one of the two problems.
So because of the above there is only one problem. If your system opened access to uhid to random processes you have much bigger problems than exposing some data from a suid binary. You can spam "rm -rf .; rm -rf /" though uhid if there is interactive session somewhere.
Do you think the proposed restrictions would actually break anything?
It would break if someone uses UHID_CREATE with sendpage. I do not know if anyone does. If we were certain there are no users we'd simply removed UHID_CREATE altogether.
AFAICT, there are 2 users of uhid:
- bluez for BLE devices (using HID over GATT)
- hid-replay for debugging.
There are several more (eg., android bt-broadcom), and UHID_CREATE is actively used. Dropping support for it will break these use-cases.
Is the support story for these programs such that we could add a big scary warning and remove UHID_CREATE in a couple months?
Hi
On Wed, Nov 14, 2018 at 10:55 PM Eric Biggers ebiggers@kernel.org wrote:
From: Eric Biggers ebiggers@google.com
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.
Thanks to Dmitry Vyukov for adding uhid definitions to syzkaller and to Jann Horn for commit 9da3f2b740544 ("x86/fault: BUG() when uaccess helpers fault on kernel addresses"), allowing this bug to be found.
Reported-by: syzbot+72473edc9bf4eb1c6556@syzkaller.appspotmail.com Fixes: d365c6cfd337 ("HID: uhid: add UHID_CREATE and UHID_DESTROY events") Cc: stable@vger.kernel.org # v3.6+ Cc: Jann Horn jannh@google.com Cc: Andy Lutomirski luto@kernel.org Signed-off-by: Eric Biggers ebiggers@google.com
drivers/hid/uhid.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c index 3c55073136064..051639c09f728 100644 --- a/drivers/hid/uhid.c +++ b/drivers/hid/uhid.c @@ -12,6 +12,7 @@
#include <linux/atomic.h> #include <linux/compat.h> +#include <linux/cred.h> #include <linux/device.h> #include <linux/fs.h> #include <linux/hid.h> @@ -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().*/if (file->f_cred != current_cred() || uaccess_kernel()) {
I think `uaccess_kernel()` would be enough. UHID does not check any credentials. If you believe this should be there nevertheless, feel free to keep it. Either way:
Acked-by: David Herrmann dh.herrmann@gmail.com
Thanks David
pr_err_once("UHID_CREATE from different security context by process %d (%s), this is not allowed.\n",task_tgid_vnr(current), current->comm);ret = -EACCES;goto unlock;} ret = uhid_dev_create(uhid, &uhid->input_buf); break; case UHID_CREATE2:-- 2.19.1.930.g4563a0d9d0-goog
On Nov 15, 2018, at 4:09 AM, David Herrmann dh.herrmann@gmail.com wrote:
Hi
On Wed, Nov 14, 2018 at 10:55 PM Eric Biggers ebiggers@kernel.org wrote: From: Eric Biggers ebiggers@google.com
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.
Thanks to Dmitry Vyukov for adding uhid definitions to syzkaller and to Jann Horn for commit 9da3f2b740544 ("x86/fault: BUG() when uaccess helpers fault on kernel addresses"), allowing this bug to be found.
Reported-by: syzbot+72473edc9bf4eb1c6556@syzkaller.appspotmail.com Fixes: d365c6cfd337 ("HID: uhid: add UHID_CREATE and UHID_DESTROY events") Cc: stable@vger.kernel.org # v3.6+ Cc: Jann Horn jannh@google.com Cc: Andy Lutomirski luto@kernel.org Signed-off-by: Eric Biggers ebiggers@google.com
drivers/hid/uhid.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c index 3c55073136064..051639c09f728 100644 --- a/drivers/hid/uhid.c +++ b/drivers/hid/uhid.c @@ -12,6 +12,7 @@
#include <linux/atomic.h> #include <linux/compat.h> +#include <linux/cred.h> #include <linux/device.h> #include <linux/fs.h> #include <linux/hid.h> @@ -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().*/if (file->f_cred != current_cred() || uaccess_kernel()) {I think `uaccess_kernel()` would be enough. UHID does not check any credentials. If you believe this should be there nevertheless, feel free to keep it.
The free check is needed. Without it, consider what sudo >uhid_fd does. It doesn’t use sudo’s credentials, but it does read its address space.
Can this patch get a comment added?
Either way:
Acked-by: David Herrmann dh.herrmann@gmail.com
Thanks David
[ David added to CC ]
On Wed, 14 Nov 2018, Eric Biggers wrote:
From: Eric Biggers ebiggers@google.com
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.
Thanks to Dmitry Vyukov for adding uhid definitions to syzkaller and to Jann Horn for commit 9da3f2b740544 ("x86/fault: BUG() when uaccess helpers fault on kernel addresses"), allowing this bug to be found.
Reported-by: syzbot+72473edc9bf4eb1c6556@syzkaller.appspotmail.com Fixes: d365c6cfd337 ("HID: uhid: add UHID_CREATE and UHID_DESTROY events") Cc: stable@vger.kernel.org # v3.6+ Cc: Jann Horn jannh@google.com Cc: Andy Lutomirski luto@kernel.org Signed-off-by: Eric Biggers ebiggers@google.com
Thanks for the patch. I however believe the fix below is more generic, and would prefer taking that one in case noone sees any major flaw in that I've overlooked. Thanks.
From: David Herrmann dh.herrmann@gmail.com Subject: [PATCH] HID: uhid: prevent splice(2)
The kernel has a default implementation of splice(2) for writing from a pipe into an arbitrary file. This behavior can be overriden by providing an f_op.splice_write() callback.
Unfortunately, the default implementation of splice_write() takes page by page from the source pipe, calls kmap() and passes the mapped page as kernel-address to f_op.write(). Thus, it uses standard write(2) to implement splice(2). However, since the page is kernel-mapped, they have to `set_fs(get_ds())`. This is mostly fine, but UHID takes command-streams through write(2), and thus it might interpret the data taken as pointers. If called with KERNEL_DS, you can trick UHID to allow kernel-space pointers as well.
As a simple fix, prevent splice(2) on UHID. It is unsecure, but it is also non-functional. We need a linear mapping of the input in UHID, so chunked input from splice(2) makes no sense, anyway.
Reported-by: syzbot+72473edc9bf4eb1c6556@syzkaller.appspotmail.com Signed-off-by: David Herrmann dh.herrmann@gmail.com --- drivers/hid/uhid.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c index 3c5507313606..fefedc0b4dc6 100644 --- a/drivers/hid/uhid.c +++ b/drivers/hid/uhid.c @@ -753,6 +753,15 @@ static ssize_t uhid_char_write(struct file *file, const char __user *buffer, return ret ? ret : count; }
+static ssize_t uhid_char_splice_write(struct pipe_inode_info *pipe, + struct file *out, + loff_t *ppos, + size_t len, + unsigned int flags) +{ + return -EOPNOTSUPP; +} + static __poll_t uhid_char_poll(struct file *file, poll_table *wait) { struct uhid_device *uhid = file->private_data; @@ -771,6 +780,7 @@ static const struct file_operations uhid_fops = { .release = uhid_char_release, .read = uhid_char_read, .write = uhid_char_write, + .splice_write = uhid_char_splice_write, .poll = uhid_char_poll, .llseek = no_llseek, };
Hey
On Mon, Nov 19, 2018 at 1:52 PM Jiri Kosina jikos@kernel.org wrote:
[ David added to CC ]
On Wed, 14 Nov 2018, Eric Biggers wrote:
From: Eric Biggers ebiggers@google.com
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.
Thanks to Dmitry Vyukov for adding uhid definitions to syzkaller and to Jann Horn for commit 9da3f2b740544 ("x86/fault: BUG() when uaccess helpers fault on kernel addresses"), allowing this bug to be found.
Reported-by: syzbot+72473edc9bf4eb1c6556@syzkaller.appspotmail.com Fixes: d365c6cfd337 ("HID: uhid: add UHID_CREATE and UHID_DESTROY events") Cc: stable@vger.kernel.org # v3.6+ Cc: Jann Horn jannh@google.com Cc: Andy Lutomirski luto@kernel.org Signed-off-by: Eric Biggers ebiggers@google.com
Thanks for the patch. I however believe the fix below is more generic, and would prefer taking that one in case noone sees any major flaw in that I've overlooked. Thanks.
As Andy rightly pointed out, the credentials check is actually needed. The scenario here is using a uhid-fd as stdout when executing a setuid-program. This will possibly end up reading arbitrary memory from the setuid program and use it as input for the hid-descriptor.
To my knowledge, this is a rather small attack surface. UHID is usually a privileged interface, which in itself already gives you ridiculous privileges. Furthermore, it only allows read-access if you happen to be able to craft the message the setuid program writes (which must be a valid user-space pointer, pointing to a valid hid descriptor). But people have been creative in the past, and they will find a way to use this. So I do think Eric's patch here is the way to go.
Lastly, this only guards UHID_CREATE, which is already a deprecated interface for several years. I don't think we can drop it any time soon, but at least the other uhid interfaces should be safe.
Thanks David
On Mon, 19 Nov 2018, David Herrmann wrote:
Thanks for the patch. I however believe the fix below is more generic, and would prefer taking that one in case noone sees any major flaw in that I've overlooked. Thanks.
As Andy rightly pointed out, the credentials check is actually needed. The scenario here is using a uhid-fd as stdout when executing a setuid-program. This will possibly end up reading arbitrary memory from the setuid program and use it as input for the hid-descriptor.
Ah, right, that's a very good point indeed; I've overlooked that (valid) concern in the thread. Thanks for spotting that, Andy.
I've now applied Eric's patch. Thanks everybody,
linux-stable-mirror@lists.linaro.org
-
Andy Lutomirski -
Benjamin Tissoires -
David Herrmann -
Dmitry Torokhov -
Eric Biggers -
Jann Horn -
Jiri Kosina