BUG: GPF in non-whitelisted uaccess (non-canonical address?)

syzbot has found a reproducer for the following crash on:

HEAD commit:    ccda4af0f4b9 Linux 4.20-rc2
git tree:       upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=13b4e77b400000
kernel config:  https://syzkaller.appspot.com/x/.config?x=4a0a89f12ca9b0f5
dashboard link: https://syzkaller.appspot.com/bug?extid=72473edc9bf4eb1c6556
compiler:       gcc (GCC) 8.0.1 20180413 (experimental)
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=1646a225400000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=108a6533400000

IMPORTANT: if you fix the bug, please add the following tag to the commit:

Reported-by [email protected]

BUG: GPF in non-whitelisted uaccess (non-canonical address?)

Hey

Quoted 14 lines · syzbot

syzbot has found a reproducer for the following crash on:

HEAD commit:    ccda4af0f4b9 Linux 4.20-rc2
git tree:       upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=13b4e77b400000
kernel config:  https://syzkaller.appspot.com/x/.config?x=4a0a89f12ca9b0f5
dashboard link: https://syzkaller.appspot.com/bug?extid=72473edc9bf4eb1c6556
compiler:       gcc (GCC) 8.0.1 20180413 (experimental)
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=1646a225400000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=108a6533400000

IMPORTANT: if you fix the bug, please add the following tag to the commit:

Reported-by [email protected]

[...]

Quoted 1 line

BUG: GPF in non-whitelisted uaccess (non-canonical address?)

This uses sendpage(2) to feed data from a file into a uhid chardev. The default behavior of the kernel is to create a temporary pipe, then splice from the file into the pipe, and then splice again from the pipe into uhid.

The kernel provides default implementations for splicing between files and any other file. The default implementation of .splice_write() uses kmap() to map the page from the pipe and then uses the __kernel_write() (which uses .f_op->write()) to push the data into the target file. The problem is, __kernel_write() sets the address-space to KERNEL_DS set_fs(get_ds()), thus granting the UHID request access to kernel memory.

I see several ways to fix that, the most simple solution is to simply prevent splice/sendpage on uhid (by setting f_op.splice_write to a dummy). Alternatively, we can implement a proper splice helper that takes the page directly, rather than through the __kernel_write() default implementation.

Quoted 34 lines · David Herrmann

Hey

Quoted 14 lines · syzbot

syzbot has found a reproducer for the following crash on:

HEAD commit:    ccda4af0f4b9 Linux 4.20-rc2
git tree:       upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=13b4e77b400000
kernel config:  https://syzkaller.appspot.com/x/.config?x=4a0a89f12ca9b0f5
dashboard link: https://syzkaller.appspot.com/bug?extid=72473edc9bf4eb1c6556
compiler:       gcc (GCC) 8.0.1 20180413 (experimental)
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=1646a225400000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=108a6533400000

IMPORTANT: if you fix the bug, please add the following tag to the commit:

Reported-by [email protected]

[...]

Quoted 1 line

BUG: GPF in non-whitelisted uaccess (non-canonical address?)

This uses sendpage(2) to feed data from a file into a uhid chardev. The default behavior of the kernel is to create a temporary pipe, then splice from the file into the pipe, and then splice again from the pipe into uhid.

The kernel provides default implementations for splicing between files and any other file. The default implementation of .splice_write() uses kmap() to map the page from the pipe and then uses the __kernel_write() (which uses .f_op->write()) to push the data into the target file. The problem is, __kernel_write() sets the address-space to KERNEL_DS set_fs(get_ds()), thus granting the UHID request access to kernel memory.

Quoted 5 lines

I see several ways to fix that, the most simple solution is to simply prevent splice/sendpage on uhid (by setting f_op.splice_write to a dummy). Alternatively, we can implement a proper splice helper that takes the page directly, rather than through the __kernel_write() default implementation.

also +dtor for uhid

From: Eric Biggers <[email protected]>

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 [email protected]

Fixes d365c6cfd337 ("HID: uhid: add UHID_CREATE and UHID_DESTROY events")

Cc <[email protected]> # v3.6+

Cc Jann Horn <[email protected]>

Signed-off-by Eric Biggers <[email protected]>

---
 drivers/hid/uhid.c | 6 ++++++
 1 file changed, 6 insertions(+)

drivers/hid/uhid.c +6 -1

index 3c55073136064..e94c5e248b56e 100644
@@ -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

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 [email protected]

Fixes d365c6cfd337 ("HID: uhid: add UHID_CREATE and UHID_DESTROY events")

Cc <[email protected]> # v3.6+

Cc Jann Horn <[email protected]>

Signed-off-by Eric Biggers <[email protected]>

---
 drivers/hid/uhid.c | 6 ++++++
 1 file changed, 6 insertions(+)

drivers/hid/uhid.c +0 -0

index 3c55073136064..e94c5e248b56e 100644

+cc Andy

Quoted 10 lines · Eric Biggers

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.

Quoted 5 lines

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 [email protected]

Wheeeee, it found something! :)

Quoted 13 lines

Fixes d365c6cfd337 ("HID: uhid: add UHID_CREATE and UHID_DESTROY events")

Cc <[email protected]> # v3.6+

Cc Jann Horn <[email protected]>

Signed-off-by Eric Biggers <[email protected]>

---
 drivers/hid/uhid.c | 6 ++++++
 1 file changed, 6 insertions(+)

drivers/hid/uhid.c +0 -0

index 3c55073136064..e94c5e248b56e 100644

drivers/hid/uhid.c static ssize_t uhid_char_write(struct file *file, const char __user *buffer, · line 712

Quoted 8 lines

drivers/hid/uhid.c +5 -0

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

From: Eric Biggers <[email protected]>

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 [email protected]

Fixes d365c6cfd337 ("HID: uhid: add UHID_CREATE and UHID_DESTROY events")

Cc <[email protected]> # v3.6+

Cc Jann Horn <[email protected]>

Cc Andy Lutomirski <[email protected]>

Signed-off-by Eric Biggers <[email protected]>

---
 drivers/hid/uhid.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

drivers/hid/uhid.c +12 -1

index 3c55073136064..051639c09f728 100644
@@ -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

Quoted 18 lines · Eric Biggers

From: Eric Biggers <[email protected]>

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.

Quoted 6 lines

Reported-by [email protected]

Fixes d365c6cfd337 ("HID: uhid: add UHID_CREATE and UHID_DESTROY events")

Cc <[email protected]> # v3.6+

Cc Jann Horn <[email protected]>

Cc Andy Lutomirski <[email protected]>

Signed-off-by Eric Biggers <[email protected]>

Reviewed-by Jann Horn <[email protected]>

Quoted 18 lines

---
 drivers/hid/uhid.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

drivers/hid/uhid.c +1 -0

index 3c55073136064..051639c09f728 100644
@@ -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,
 

drivers/hid/uhid.c static ssize_t uhid_char_write(struct file *file, const char __user *buffer, · line 730

Quoted 7 lines

drivers/hid/uhid.c +5 -0

        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().
+                */

Quoted 4 lines

drivers/hid/uhid.c +4 -0

+               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;

Quoted 8 lines

drivers/hid/uhid.c +2 -1

+                       goto unlock;
+               }
                ret = uhid_dev_create(uhid, &uhid->input_buf);
                break;
        case UHID_CREATE2:
--

2.19.1.930.g4563a0d9d0-goog

Quoted 11 lines · Jann Horn

Quoted 10 lines · Eric Biggers

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 1 line

Quoted 1 line

drivers/hid/uhid.c +0 -0

Quoted 9 lines

drivers/hid/uhid.c static ssize_t uhid_char_write(struct file *file, const char __user *buffer, · line 730

Quoted 9 lines

drivers/hid/uhid.c +5 -0

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

Quoted 9 lines

Quoted 9 lines

drivers/hid/uhid.c +6 -0

+               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:

Quoted 12 lines · Dmitry Torokhov

Quoted 11 lines · Jann Horn

Quoted 10 lines · Eric Biggers

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 1 line

Quoted 1 line

Quoted 1 line

drivers/hid/uhid.c +0 -0

[...]

Quoted 14 lines

Quoted 9 lines

drivers/hid/uhid.c static ssize_t uhid_char_write(struct file *file, const char __user *buffer, · line 730

Quoted 9 lines

drivers/hid/uhid.c +5 -0

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

Quoted 2 lines

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.

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

drivers/hid/uhid.c +6 -0

+               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:

Quoted 36 lines · Jann Horn

Quoted 14 lines · Dmitry Torokhov

Quoted 12 lines · Jann Horn

Quoted 11 lines · Eric Biggers

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 1 line

Quoted 1 line

Quoted 1 line

drivers/hid/uhid.c +0 -0

[...]

Quoted 14 lines

Quoted 9 lines

drivers/hid/uhid.c static ssize_t uhid_char_write(struct file *file, const char __user *buffer, · line 730

Quoted 9 lines

drivers/hid/uhid.c +5 -0

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

Quoted 2 lines

Also, instead of disallowing access, can we ensure that we switch back to USER_DS before trying to load data from the user pointer?

Quoted 7 lines

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.

Quoted 47 lines · Jann Horn

Quoted 18 lines · Eric Biggers

From: Eric Biggers <[email protected]>

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.

Quoted 6 lines

Reported-by [email protected]

Fixes d365c6cfd337 ("HID: uhid: add UHID_CREATE and UHID_DESTROY events")

Cc <[email protected]> # v3.6+

Cc Jann Horn <[email protected]>

Cc Andy Lutomirski <[email protected]>

Signed-off-by Eric Biggers <[email protected]>

Reviewed-by Jann Horn <[email protected]>

Quoted 18 lines

---
 drivers/hid/uhid.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

drivers/hid/uhid.c +1 -0

index 3c55073136064..051639c09f728 100644
@@ -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,
 

Quoted 7 lines

drivers/hid/uhid.c static ssize_t uhid_char_write(struct file *file, const char __user *buffer, · line 730

Quoted 7 lines

drivers/hid/uhid.c +5 -0

        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?

Quoted 10 lines

Quoted 10 lines

drivers/hid/uhid.c +6 -1

+               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:
--

Hi Dmitry,

Quoted 60 lines · Dmitry Torokhov' via syzkaller-bugs

Quoted 47 lines · Jann Horn

Quoted 18 lines · Eric Biggers

From: Eric Biggers <[email protected]>

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.

Quoted 6 lines

Reported-by [email protected]

Fixes d365c6cfd337 ("HID: uhid: add UHID_CREATE and UHID_DESTROY events")

Cc <[email protected]> # v3.6+

Cc Jann Horn <[email protected]>

Cc Andy Lutomirski <[email protected]>

Signed-off-by Eric Biggers <[email protected]>

Reviewed-by Jann Horn <[email protected]>

Quoted 18 lines

---
 drivers/hid/uhid.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

drivers/hid/uhid.c +1 -0

index 3c55073136064..051639c09f728 100644
@@ -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,
 

Quoted 7 lines

drivers/hid/uhid.c static ssize_t uhid_char_write(struct file *file, const char __user *buffer, · line 730

Quoted 7 lines

drivers/hid/uhid.c +5 -0

        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.

Quoted 3 lines

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

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

drivers/hid/uhid.c +6 -1

+               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:
--

Quoted 64 lines · Eric Biggers

Hi Dmitry,

Quoted 60 lines · Dmitry Torokhov' via syzkaller-bugs

Quoted 47 lines · Jann Horn

Quoted 18 lines · Eric Biggers

From: Eric Biggers <[email protected]>

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.

Quoted 6 lines

Reported-by [email protected]

Fixes d365c6cfd337 ("HID: uhid: add UHID_CREATE and UHID_DESTROY events")

Cc <[email protected]> # v3.6+

Cc Jann Horn <[email protected]>

Cc Andy Lutomirski <[email protected]>

Signed-off-by Eric Biggers <[email protected]>

Reviewed-by Jann Horn <[email protected]>

Quoted 18 lines

---
 drivers/hid/uhid.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

drivers/hid/uhid.c +1 -0

index 3c55073136064..051639c09f728 100644
@@ -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,
 

Quoted 7 lines

drivers/hid/uhid.c static ssize_t uhid_char_write(struct file *file, const char __user *buffer, · line 730

Quoted 7 lines

drivers/hid/uhid.c +5 -0

        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.

Quoted 6 lines

Quoted 3 lines

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.

Quoted 4 lines

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.

Quoted 3 lines

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.

Quoted 1 line

Quoted 1 line

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.

Quoted 14 lines

- Eric

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

drivers/hid/uhid.c +6 -1

+               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

Quoted 5 lines · Dmitry Torokhov

Quoted 4 lines

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.

Quoted 3 lines

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.

Quoted 13 lines

Quoted 3 lines

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.

Quoted 2 lines

Do you think the proposed restrictions would actually break anything?

Quoted 3 lines

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.

Hey

Quoted 7 lines · Benjamin Tissoires

Quoted 5 lines · Dmitry Torokhov

Quoted 4 lines

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.

Quoted 8 lines

Quoted 3 lines

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.

Quoted 17 lines

Quoted 16 lines

Quoted 3 lines

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.

Quoted 2 lines

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.

Quoted 3 lines

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.

From: Eric Biggers <[email protected]>

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 [email protected]

Fixes d365c6cfd337 ("HID: uhid: add UHID_CREATE and UHID_DESTROY events")

Cc <[email protected]> # v3.6+

Cc Jann Horn <[email protected]>

Cc Andy Lutomirski <[email protected]>

Signed-off-by Eric Biggers <[email protected]>

---
 drivers/hid/uhid.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

drivers/hid/uhid.c +1 -0

index 3c55073136064..051639c09f728 100644
@@ -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>

drivers/hid/uhid.c +5 -1

+                       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

Hi

Quoted 42 lines · Eric Biggers

From: Eric Biggers <[email protected]>

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 [email protected]

Fixes d365c6cfd337 ("HID: uhid: add UHID_CREATE and UHID_DESTROY events")

Cc <[email protected]> # v3.6+

Cc Jann Horn <[email protected]>

Cc Andy Lutomirski <[email protected]>

Signed-off-by Eric Biggers <[email protected]>

---
drivers/hid/uhid.c | 12 ++++++++++++

1 file changed, 12 insertions(+)

drivers/hid/uhid.c +1 -0

index 3c55073136064..051639c09f728 100644
@@ -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,
 

drivers/hid/uhid.c static ssize_t uhid_char_write(struct file *file, const char __user *buffer, · line 731

Quoted 8 lines

drivers/hid/uhid.c +6 -0

       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 <[email protected]>

Thanks
David

From: Eric Biggers <[email protected]>

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 [email protected]

Fixes d365c6cfd337 ("HID: uhid: add UHID_CREATE and UHID_DESTROY events")

Cc <[email protected]> # v3.6+

Cc Jann Horn <[email protected]>

Cc Andy Lutomirski <[email protected]>

Signed-off-by Eric Biggers <[email protected]>

[ David added to CC ]

Quoted 17 lines · Eric Biggers

From: Eric Biggers <[email protected]>

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.

Quoted 6 lines

Reported-by [email protected]

Fixes d365c6cfd337 ("HID: uhid: add UHID_CREATE and UHID_DESTROY events")

Cc <[email protected]> # v3.6+

Cc Jann Horn <[email protected]>

Cc Andy Lutomirski <[email protected]>

Signed-off-by Eric Biggers <[email protected]>

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.

Quoted 3 lines

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.