linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* BUG: GPF in non-whitelisted uaccess (non-canonical address?)
@ 2018-11-11 18:26 syzbot
  2018-11-14  0:25 ` syzbot
  0 siblings, 1 reply; 25+ messages in thread
From: syzbot @ 2018-11-11 18:26 UTC (permalink / raw)
  To: benjamin.tissoires, dh.herrmann, jikos, linux-input,
	linux-kernel, syzkaller-bugs

Hello,

syzbot found the following crash on:

HEAD commit:    ab6e1f378f54 Merge tag 'for-linus-4.20a-rc2-tag' of git://..
git tree:       upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=11f65b83400000
kernel config:  https://syzkaller.appspot.com/x/.config?x=8f215f21f041a0d7
dashboard link: https://syzkaller.appspot.com/bug?extid=72473edc9bf4eb1c6556
compiler:       gcc (GCC) 8.0.1 20180413 (experimental)

Unfortunately, I don't have any reproducer for this crash yet.

IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+72473edc9bf4eb1c6556@syzkaller.appspotmail.com

kernel msg: ebtables bug: please report to author: Nentries wrong
kernel msg: ebtables bug: please report to author: Nentries wrong
BUG: GPF in non-whitelisted uaccess (non-canonical address?)
kasan: CONFIG_KASAN_INLINE enabled
IPVS: ftp: loaded support on port[0] = 21
kasan: GPF could be caused by NULL-ptr deref or user memory access
general protection fault: 0000 [#1] PREEMPT SMP KASAN
CPU: 1 PID: 31848 Comm: syz-executor1 Not tainted 4.20.0-rc1+ #109
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS  
Google 01/01/2011
RIP: 0010:copy_user_enhanced_fast_string+0xe/0x20  
arch/x86/lib/copy_user_64.S:180
Code: 89 d1 c1 e9 03 83 e2 07 f3 48 a5 89 d1 f3 a4 31 c0 0f 1f 00 c3 0f 1f  
80 00 00 00 00 0f 1f 00 83 fa 40 0f 82 70 ff ff ff 89 d1 <f3> a4 31 c0 0f  
1f 00 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 00 83
kobject: 'lo' (00000000a6443dd1): kobject_add_internal: parent: 'net',  
set: 'devices'
RSP: 0018:ffff8801d70af398 EFLAGS: 00010206
RAX: 0000000000000000 RBX: 00000000000000be RCX: 00000000000000be
RDX: 00000000000000be RSI: 4caaadbf9db04a99 RDI: ffff8801bf63b2b8
RBP: ffff8801d70af3d0 R08: ffffed0037ec766f R09: ffffed0037ec766f
R10: ffffed0037ec766e R11: ffff8801bf63b375 R12: 4caaadbf9db04b57
R13: 4caaadbf9db04a99 R14: ffff8801bf63b2b8 R15: ffffffffffffffff
FS:  00007f0b59d0f700(0000) GS:ffff8801daf00000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000625208 CR3: 00000001b7e87000 CR4: 00000000001406e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
kobject: 'loop5' (00000000bad8892f): kobject_uevent_env
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
  copy_from_user include/linux/uaccess.h:147 [inline]
  uhid_dev_create+0x20c/0xb40 drivers/hid/uhid.c:542
kobject: 'loop5' (00000000bad8892f): fill_kobj_path: path  
= '/devices/virtual/block/loop5'
kernel msg: ebtables bug: please report to author: Nentries wrong
  uhid_char_write+0xc74/0xef0 drivers/hid/uhid.c:725
kernel msg: ebtables bug: please report to author: Nentries wrong
  __vfs_write+0x119/0x9f0 fs/read_write.c:485
kobject: 'loop4' (000000000f3fc2e6): kobject_uevent_env
kobject: 'loop4' (000000000f3fc2e6): fill_kobj_path: path  
= '/devices/virtual/block/loop4'
kobject: 'lo' (00000000a6443dd1): kobject_uevent_env
  __kernel_write+0x10c/0x370 fs/read_write.c:506
  write_pipe_buf+0x180/0x240 fs/splice.c:797
kobject: 'lo' (00000000a6443dd1): fill_kobj_path: path  
= '/devices/virtual/net/lo'
  splice_from_pipe_feed fs/splice.c:503 [inline]
  __splice_from_pipe+0x38b/0x7c0 fs/splice.c:627
kobject: 'queues' (00000000cdfb81e7): kobject_add_internal: parent: 'lo',  
set: '<NULL>'
  splice_from_pipe+0x1ec/0x340 fs/splice.c:662
kernel msg: ebtables bug: please report to author: Nentries wrong
  default_file_splice_write+0x3c/0x90 fs/splice.c:809
kernel msg: ebtables bug: please report to author: Nentries wrong
  do_splice_from fs/splice.c:851 [inline]
  direct_splice_actor+0x128/0x190 fs/splice.c:1018
  splice_direct_to_actor+0x318/0x8f0 fs/splice.c:973
kobject: 'queues' (00000000cdfb81e7): kobject_uevent_env
kobject: 'loop2' (00000000314877e3): kobject_uevent_env
kobject: 'loop2' (00000000314877e3): fill_kobj_path: path  
= '/devices/virtual/block/loop2'
  do_splice_direct+0x2d4/0x420 fs/splice.c:1061
kobject: 'queues' (00000000cdfb81e7): kobject_uevent_env: filter function  
caused the event to drop!
  do_sendfile+0x62a/0xe20 fs/read_write.c:1439
kobject: 'rx-0' (0000000095d78570): kobject_add_internal: parent: 'queues',  
set: 'queues'
  __do_sys_sendfile64 fs/read_write.c:1494 [inline]
  __se_sys_sendfile64 fs/read_write.c:1486 [inline]
  __x64_sys_sendfile64+0x15d/0x250 fs/read_write.c:1486
  do_syscall_64+0x1b9/0x820 arch/x86/entry/common.c:290
kobject: 'rx-0' (0000000095d78570): kobject_uevent_env
kobject: 'rx-0' (0000000095d78570): fill_kobj_path: path  
= '/devices/virtual/net/lo/queues/rx-0'
  entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x457569
Code: fd b3 fb ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 48 89 f8 48 89 f7  
48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff  
ff 0f 83 cb b3 fb ff c3 66 2e 0f 1f 84 00 00 00 00
kobject: 'tx-0' (0000000002ec5b19): kobject_add_internal: parent: 'queues',  
set: 'queues'
RSP: 002b:00007f0b59d0ec78 EFLAGS: 00000246 ORIG_RAX: 0000000000000028
RAX: ffffffffffffffda RBX: 0000000000000004 RCX: 0000000000457569
RDX: 0000000020d83ff8 RSI: 0000000000000004 RDI: 0000000000000003
RBP: 000000000072bf00 R08: 0000000000000000 R09: 0000000000000000
R10: 00008000fffffffe R11: 0000000000000246 R12: 00007f0b59d0f6d4
R13: 00000000004c37cc R14: 00000000004d5958 R15: 00000000ffffffff
kobject: 'tx-0' (0000000002ec5b19): kobject_uevent_env
Modules linked in:
---[ end trace 05f5a052f649341e ]---
kobject: 'tx-0' (0000000002ec5b19): fill_kobj_path: path  
= '/devices/virtual/net/lo/queues/tx-0'
RIP: 0010:copy_user_enhanced_fast_string+0xe/0x20  
arch/x86/lib/copy_user_64.S:180
Code: 89 d1 c1 e9 03 83 e2 07 f3 48 a5 89 d1 f3 a4 31 c0 0f 1f 00 c3 0f 1f  
80 00 00 00 00 0f 1f 00 83 fa 40 0f 82 70 ff ff ff 89 d1 <f3> a4 31 c0 0f  
1f 00 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 00 83
IPVS: ftp: loaded support on port[0] = 21
kobject: 'loop0' (00000000340bf24d): kobject_uevent_env
kobject: 'rx-0' (00000000a3b2c01d): kobject_cleanup, parent 00000000717c1169
kobject: 'loop0' (00000000340bf24d): fill_kobj_path: path  
= '/devices/virtual/block/loop0'
kobject: 'rx-0' (00000000a3b2c01d): auto cleanup 'remove' event
RSP: 0018:ffff8801d70af398 EFLAGS: 00010206
RAX: 0000000000000000 RBX: 00000000000000be RCX: 00000000000000be
kobject: 'rx-0' (00000000a3b2c01d): kobject_uevent_env
kobject: 'loop5' (00000000bad8892f): kobject_uevent_env
kobject: 'rx-0' (00000000a3b2c01d): fill_kobj_path: path  
= '/devices/virtual/net/syz_tun/queues/rx-0'
RDX: 00000000000000be RSI: 4caaadbf9db04a99 RDI: ffff8801bf63b2b8
kobject: 'loop5' (00000000bad8892f): fill_kobj_path: path  
= '/devices/virtual/block/loop5'
RBP: ffff8801d70af3d0 R08: ffffed0037ec766f R09: ffffed0037ec766f
kobject: 'rx-0' (00000000a3b2c01d): auto cleanup kobject_del
R10: ffffed0037ec766e R11: ffff8801bf63b375 R12: 4caaadbf9db04b57
R13: 4caaadbf9db04a99 R14: ffff8801bf63b2b8 R15: ffffffffffffffff
kobject: 'rx-0' (00000000a3b2c01d): calling ktype release
FS:  00007f0b59d0f700(0000) GS:ffff8801daf00000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000001cc6308 CR3: 00000001b7e87000 CR4: 00000000001406e0
kobject: 'rx-0': free name
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
kobject: 'tx-0' (00000000bb9c9857): kobject_cleanup, parent 00000000717c1169
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400


---
This bug is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.

syzbot will keep track of this bug report. See:
https://goo.gl/tpsmEJ#bug-status-tracking for how to communicate with  
syzbot.

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: BUG: GPF in non-whitelisted uaccess (non-canonical address?)
  2018-11-11 18:26 BUG: GPF in non-whitelisted uaccess (non-canonical address?) syzbot
@ 2018-11-14  0:25 ` syzbot
  2018-11-14 12:20   ` David Herrmann
  0 siblings, 1 reply; 25+ messages in thread
From: syzbot @ 2018-11-14  0:25 UTC (permalink / raw)
  To: benjamin.tissoires, dh.herrmann, jikos, linux-input,
	linux-kernel, syzkaller-bugs

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: syzbot+72473edc9bf4eb1c6556@syzkaller.appspotmail.com

audit: type=1400 audit(1542154838.102:35): avc:  denied  { map } for   
pid=6149 comm="bash" path="/bin/bash" dev="sda1" ino=1457  
scontext=unconfined_u:system_r:insmod_t:s0-s0:c0.c1023  
tcontext=system_u:object_r:file_t:s0 tclass=file permissive=1
audit: type=1400 audit(1542154844.872:36): avc:  denied  { map } for   
pid=6163 comm="syz-executor697" path="/root/syz-executor697315096"  
dev="sda1" ino=16483 scontext=unconfined_u:system_r:insmod_t:s0-s0:c0.c1023  
tcontext=unconfined_u:object_r:user_home_t:s0 tclass=file permissive=1
BUG: GPF in non-whitelisted uaccess (non-canonical address?)
kasan: CONFIG_KASAN_INLINE enabled
kasan: GPF could be caused by NULL-ptr deref or user memory access
general protection fault: 0000 [#1] PREEMPT SMP KASAN
CPU: 0 PID: 6163 Comm: syz-executor697 Not tainted 4.20.0-rc2+ #112
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS  
Google 01/01/2011
RIP: 0010:copy_user_enhanced_fast_string+0xe/0x20  
arch/x86/lib/copy_user_64.S:180
Code: 89 d1 c1 e9 03 83 e2 07 f3 48 a5 89 d1 f3 a4 31 c0 0f 1f 00 c3 0f 1f  
80 00 00 00 00 0f 1f 00 83 fa 40 0f 82 70 ff ff ff 89 d1 <f3> a4 31 c0 0f  
1f 00 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 00 83
RSP: 0018:ffff8881d188f398 EFLAGS: 00010206
RAX: 0000000000000000 RBX: 0000000000000109 RCX: 0000000000000109
RDX: 0000000000000109 RSI: 241037f828e5769d RDI: ffff8881ce130738
RBP: ffff8881d188f3d0 R08: ffffed1039c26109 R09: ffffed1039c26109
R10: ffffed1039c26108 R11: ffff8881ce130840 R12: 241037f828e577a6
R13: 241037f828e5769d R14: ffff8881ce130738 R15: ffffffffffffffff
FS:  0000000001dab880(0000) GS:ffff8881dae00000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000020d83ff8 CR3: 00000001b6a5c000 CR4: 00000000001406f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
  copy_from_user include/linux/uaccess.h:147 [inline]
  uhid_dev_create+0x20c/0xb40 drivers/hid/uhid.c:542
  uhid_char_write+0xc74/0xef0 drivers/hid/uhid.c:725
  __vfs_write+0x119/0x9f0 fs/read_write.c:485
  __kernel_write+0x10c/0x370 fs/read_write.c:506
  write_pipe_buf+0x180/0x240 fs/splice.c:797
  splice_from_pipe_feed fs/splice.c:503 [inline]
  __splice_from_pipe+0x38b/0x7c0 fs/splice.c:627
  splice_from_pipe+0x1ec/0x340 fs/splice.c:662
  default_file_splice_write+0x3c/0x90 fs/splice.c:809
  do_splice_from fs/splice.c:851 [inline]
  direct_splice_actor+0x128/0x190 fs/splice.c:1018
  splice_direct_to_actor+0x318/0x8f0 fs/splice.c:973
  do_splice_direct+0x2d4/0x420 fs/splice.c:1061
  do_sendfile+0x62a/0xe20 fs/read_write.c:1439
  __do_sys_sendfile64 fs/read_write.c:1494 [inline]
  __se_sys_sendfile64 fs/read_write.c:1486 [inline]
  __x64_sys_sendfile64+0x15d/0x250 fs/read_write.c:1486
  do_syscall_64+0x1b9/0x820 arch/x86/entry/common.c:290
  entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x440309
Code: 18 89 d0 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 00 48 89 f8 48 89 f7  
48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff  
ff 0f 83 fb 13 fc ff c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:00007ffcd9cfe2e8 EFLAGS: 00000203 ORIG_RAX: 0000000000000028
RAX: ffffffffffffffda RBX: 00000000004002c8 RCX: 0000000000440309
RDX: 0000000020d83ff8 RSI: 0000000000000004 RDI: 0000000000000003
RBP: 00000000006cb018 R08: 00000000004002c8 R09: 00000000004002c8
R10: 00008000fffffffe R11: 0000000000000203 R12: 0000000000401b90
R13: 0000000000401c20 R14: 0000000000000000 R15: 0000000000000000
Modules linked in:
---[ end trace d77e684529ebafbc ]---
RIP: 0010:copy_user_enhanced_fast_string+0xe/0x20  
arch/x86/lib/copy_user_64.S:180
Code: 89 d1 c1 e9 03 83 e2 07 f3 48 a5 89 d1 f3 a4 31 c0 0f 1f 00 c3 0f 1f  
80 00 00 00 00 0f 1f 00 83 fa 40 0f 82 70 ff ff ff 89 d1 <f3> a4 31 c0 0f  
1f 00 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 00 83
RSP: 0018:ffff8881d188f398 EFLAGS: 00010206
RAX: 0000000000000000 RBX: 0000000000000109 RCX: 0000000000000109
RDX: 0000000000000109 RSI: 241037f828e5769d RDI: ffff8881ce130738
RBP: ffff8881d188f3d0 R08: ffffed1039c26109 R09: ffffed1039c26109
R10: ffffed1039c26108 R11: ffff8881ce130840 R12: 241037f828e577a6
R13: 241037f828e5769d R14: ffff8881ce130738 R15: ffffffffffffffff
FS:  0000000001dab880(0000) GS:ffff8881dae00000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000020d83ff8 CR3: 00000001b6a5c000 CR4: 00000000001406f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: BUG: GPF in non-whitelisted uaccess (non-canonical address?)
  2018-11-14  0:25 ` syzbot
@ 2018-11-14 12:20   ` David Herrmann
  2018-11-14 16:52     ` Dmitry Vyukov
  0 siblings, 1 reply; 25+ messages in thread
From: David Herrmann @ 2018-11-14 12:20 UTC (permalink / raw)
  To: syzbot+72473edc9bf4eb1c6556
  Cc: Benjamin Tissoires, Jiri Kosina, open list:HID CORE LAYER,
	linux-kernel, syzkaller-bugs

Hey

On Wed, Nov 14, 2018 at 1:25 AM syzbot
<syzbot+72473edc9bf4eb1c6556@syzkaller.appspotmail.com> wrote:
> 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: syzbot+72473edc9bf4eb1c6556@syzkaller.appspotmail.com
>
[...]
> 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.

Thanks
David

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: BUG: GPF in non-whitelisted uaccess (non-canonical address?)
  2018-11-14 12:20   ` David Herrmann
@ 2018-11-14 16:52     ` Dmitry Vyukov
  2018-11-14 17:14       ` Eric Biggers
  0 siblings, 1 reply; 25+ messages in thread
From: Dmitry Vyukov @ 2018-11-14 16:52 UTC (permalink / raw)
  To: David Herrmann, Dmitry Torokhov
  Cc: syzbot+72473edc9bf4eb1c6556, Benjamin Tissoires, Jiri Kosina,
	open list:HID CORE LAYER, linux-kernel, syzkaller-bugs

On Wed, Nov 14, 2018 at 4:20 AM, David Herrmann <dh.herrmann@gmail.com> wrote:
> Hey
>
> On Wed, Nov 14, 2018 at 1:25 AM syzbot
> <syzbot+72473edc9bf4eb1c6556@syzkaller.appspotmail.com> wrote:
>> 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: syzbot+72473edc9bf4eb1c6556@syzkaller.appspotmail.com
>>
> [...]
>> 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.

also +dtor for uhid

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: BUG: GPF in non-whitelisted uaccess (non-canonical address?)
  2018-11-14 16:52     ` Dmitry Vyukov
@ 2018-11-14 17:14       ` Eric Biggers
  2018-11-14 18:02         ` [PATCH] HID: uhid: prevent uhid_char_write() under KERNEL_DS Eric Biggers
  0 siblings, 1 reply; 25+ messages in thread
From: Eric Biggers @ 2018-11-14 17:14 UTC (permalink / raw)
  To: David Herrmann
  Cc: Dmitry Vyukov, Dmitry Torokhov, syzbot+72473edc9bf4eb1c6556,
	Benjamin Tissoires, Jiri Kosina, open list:HID CORE LAYER,
	linux-kernel, syzkaller-bugs

On Wed, Nov 14, 2018 at 08:52:46AM -0800, 'Dmitry Vyukov' via syzkaller-bugs wrote:
> On Wed, Nov 14, 2018 at 4:20 AM, David Herrmann <dh.herrmann@gmail.com> wrote:
> > Hey
> >
> > On Wed, Nov 14, 2018 at 1:25 AM syzbot
> > <syzbot+72473edc9bf4eb1c6556@syzkaller.appspotmail.com> wrote:
> >> 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: syzbot+72473edc9bf4eb1c6556@syzkaller.appspotmail.com
> >>
> > [...]
> >> 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.
> 
> also +dtor for uhid
> 

Well, the problem is that uhid_char_write() reads from a user pointer embedded
in the write() payload.  (Which really is abusing write(), but I assume it
cannot be changed at this point...)  Thus it's unsafe to be called under
KERNEL_DS.  So it needs:

	if (uaccess_kernel())
		return -EACCES;

See sg_check_file_access(), called from sg_read() and sg_write(), for another
example of this in the kernel.

- Eric

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH] HID: uhid: prevent uhid_char_write() under KERNEL_DS
  2018-11-14 17:14       ` Eric Biggers
@ 2018-11-14 18:02         ` Eric Biggers
  2018-11-14 18:14           ` Dmitry Torokhov
  2018-11-14 18:18           ` Jann Horn
  0 siblings, 2 replies; 25+ messages in thread
From: Eric Biggers @ 2018-11-14 18:02 UTC (permalink / raw)
  To: David Herrmann, Jiri Kosina, Benjamin Tissoires, linux-input
  Cc: linux-kernel, syzkaller-bugs, Dmitry Vyukov, Dmitry Torokhov,
	syzbot+72473edc9bf4eb1c6556, stable, Jann Horn

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;
-- 
2.19.1.930.g4563a0d9d0-goog


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH] HID: uhid: prevent uhid_char_write() under KERNEL_DS
  2018-11-14 18:02         ` [PATCH] HID: uhid: prevent uhid_char_write() under KERNEL_DS Eric Biggers
@ 2018-11-14 18:14           ` Dmitry Torokhov
  2018-11-14 18:18           ` Jann Horn
  1 sibling, 0 replies; 25+ messages in thread
From: Dmitry Torokhov @ 2018-11-14 18:14 UTC (permalink / raw)
  To: ebiggers
  Cc: dh.herrmann, Jiri Kosina, Benjamin Tissoires,
	open list:HID CORE LAYER, lkml, syzkaller-bugs, Dmitry Vyukov,
	syzbot+72473edc9bf4eb1c6556, stable, jannh

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
>

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH] HID: uhid: prevent uhid_char_write() under KERNEL_DS
  2018-11-14 18:02         ` [PATCH] HID: uhid: prevent uhid_char_write() under KERNEL_DS Eric Biggers
  2018-11-14 18:14           ` Dmitry Torokhov
@ 2018-11-14 18:18           ` Jann Horn
  2018-11-14 21:54             ` Eric Biggers
  2018-11-14 21:55             ` [PATCH v2] HID: uhid: forbid UHID_CREATE under KERNEL_DS or elevated privileges Eric Biggers
  1 sibling, 2 replies; 25+ messages in thread
From: Jann Horn @ 2018-11-14 18:18 UTC (permalink / raw)
  To: ebiggers
  Cc: dh.herrmann, Jiri Kosina, benjamin.tissoires, linux-input,
	kernel list, syzkaller-bugs, Dmitry Vyukov, dtor,
	syzbot+72473edc9bf4eb1c6556, stable, Andy Lutomirski

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH] HID: uhid: prevent uhid_char_write() under KERNEL_DS
  2018-11-14 18:18           ` Jann Horn
@ 2018-11-14 21:54             ` Eric Biggers
  2018-11-14 21:55             ` [PATCH v2] HID: uhid: forbid UHID_CREATE under KERNEL_DS or elevated privileges Eric Biggers
  1 sibling, 0 replies; 25+ messages in thread
From: Eric Biggers @ 2018-11-14 21:54 UTC (permalink / raw)
  To: Jann Horn
  Cc: dh.herrmann, Jiri Kosina, benjamin.tissoires, linux-input,
	kernel list, syzkaller-bugs, Dmitry Vyukov, dtor,
	syzbot+72473edc9bf4eb1c6556, stable, Andy Lutomirski

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH v2] HID: uhid: forbid UHID_CREATE under KERNEL_DS or elevated privileges
  2018-11-14 18:18           ` Jann Horn
  2018-11-14 21:54             ` Eric Biggers
@ 2018-11-14 21:55             ` Eric Biggers
  2018-11-14 22:04               ` Jann Horn
                                 ` (2 more replies)
  1 sibling, 3 replies; 25+ messages in thread
From: Eric Biggers @ 2018-11-14 21:55 UTC (permalink / raw)
  To: David Herrmann, Jiri Kosina, Benjamin Tissoires, linux-input
  Cc: linux-kernel, syzkaller-bugs, Dmitry Vyukov, Dmitry Torokhov,
	syzbot+72473edc9bf4eb1c6556, stable, Jann Horn, Andy Lutomirski

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:
-- 
2.19.1.930.g4563a0d9d0-goog


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2] HID: uhid: forbid UHID_CREATE under KERNEL_DS or elevated privileges
  2018-11-14 21:55             ` [PATCH v2] HID: uhid: forbid UHID_CREATE under KERNEL_DS or elevated privileges Eric Biggers
@ 2018-11-14 22:04               ` Jann Horn
  2018-11-14 22:28                 ` Dmitry Torokhov
  2018-11-15 12:09               ` David Herrmann
  2018-11-19 12:52               ` Jiri Kosina
  2 siblings, 1 reply; 25+ messages in thread
From: Jann Horn @ 2018-11-14 22:04 UTC (permalink / raw)
  To: ebiggers
  Cc: dh.herrmann, Jiri Kosina, benjamin.tissoires, linux-input,
	kernel list, syzkaller-bugs, Dmitry Vyukov, dtor,
	syzbot+72473edc9bf4eb1c6556, stable, Andy Lutomirski

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
>

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2] HID: uhid: forbid UHID_CREATE under KERNEL_DS or elevated privileges
  2018-11-14 22:04               ` Jann Horn
@ 2018-11-14 22:28                 ` Dmitry Torokhov
  2018-11-14 22:37                   ` Jann Horn
  2018-11-14 23:00                   ` Eric Biggers
  0 siblings, 2 replies; 25+ messages in thread
From: Dmitry Torokhov @ 2018-11-14 22:28 UTC (permalink / raw)
  To: jannh
  Cc: ebiggers, dh.herrmann, Jiri Kosina, Benjamin Tissoires,
	open list:HID CORE LAYER, lkml, syzkaller-bugs, Dmitry Vyukov,
	syzbot+72473edc9bf4eb1c6556, stable, luto

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.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2] HID: uhid: forbid UHID_CREATE under KERNEL_DS or elevated privileges
  2018-11-14 22:28                 ` Dmitry Torokhov
@ 2018-11-14 22:37                   ` Jann Horn
  2018-11-14 22:46                     ` Dmitry Torokhov
  2018-11-14 23:00                   ` Eric Biggers
  1 sibling, 1 reply; 25+ messages in thread
From: Jann Horn @ 2018-11-14 22:37 UTC (permalink / raw)
  To: dtor
  Cc: ebiggers, dh.herrmann, Jiri Kosina, benjamin.tissoires,
	linux-input, kernel list, syzkaller-bugs, Dmitry Vyukov,
	syzbot+72473edc9bf4eb1c6556, stable, Andy Lutomirski

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:

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2] HID: uhid: forbid UHID_CREATE under KERNEL_DS or elevated privileges
  2018-11-14 22:37                   ` Jann Horn
@ 2018-11-14 22:46                     ` Dmitry Torokhov
  2018-11-15  0:39                       ` Andy Lutomirski
  0 siblings, 1 reply; 25+ messages in thread
From: Dmitry Torokhov @ 2018-11-14 22:46 UTC (permalink / raw)
  To: jannh
  Cc: ebiggers, dh.herrmann, Jiri Kosina, Benjamin Tissoires,
	open list:HID CORE LAYER, lkml, syzkaller-bugs, Dmitry Vyukov,
	syzbot+72473edc9bf4eb1c6556, stable, luto

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:

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2] HID: uhid: forbid UHID_CREATE under KERNEL_DS or elevated privileges
  2018-11-14 22:28                 ` Dmitry Torokhov
  2018-11-14 22:37                   ` Jann Horn
@ 2018-11-14 23:00                   ` Eric Biggers
  2018-11-14 23:20                     ` Dmitry Torokhov
  1 sibling, 1 reply; 25+ messages in thread
From: Eric Biggers @ 2018-11-14 23:00 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: jannh, dh.herrmann, Jiri Kosina, Benjamin Tissoires,
	open list:HID CORE LAYER, lkml, syzkaller-bugs, Dmitry Vyukov,
	syzbot+72473edc9bf4eb1c6556, stable, luto

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2] HID: uhid: forbid UHID_CREATE under KERNEL_DS or elevated privileges
  2018-11-14 23:00                   ` Eric Biggers
@ 2018-11-14 23:20                     ` Dmitry Torokhov
  2018-11-15  8:14                       ` Benjamin Tissoires
  0 siblings, 1 reply; 25+ messages in thread
From: Dmitry Torokhov @ 2018-11-14 23:20 UTC (permalink / raw)
  To: ebiggers
  Cc: jannh, dh.herrmann, Jiri Kosina, Benjamin Tissoires,
	open list:HID CORE LAYER, lkml, syzkaller-bugs, Dmitry Vyukov,
	syzbot+72473edc9bf4eb1c6556, stable, luto

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.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2] HID: uhid: forbid UHID_CREATE under KERNEL_DS or elevated privileges
  2018-11-14 22:46                     ` Dmitry Torokhov
@ 2018-11-15  0:39                       ` Andy Lutomirski
  0 siblings, 0 replies; 25+ messages in thread
From: Andy Lutomirski @ 2018-11-15  0:39 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: jannh, ebiggers, dh.herrmann, Jiri Kosina, Benjamin Tissoires,
	open list:HID CORE LAYER, lkml, syzkaller-bugs, Dmitry Vyukov,
	syzbot+72473edc9bf4eb1c6556, stable, luto



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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2] HID: uhid: forbid UHID_CREATE under KERNEL_DS or elevated privileges
  2018-11-14 23:20                     ` Dmitry Torokhov
@ 2018-11-15  8:14                       ` Benjamin Tissoires
  2018-11-15 12:06                         ` David Herrmann
  0 siblings, 1 reply; 25+ messages in thread
From: Benjamin Tissoires @ 2018-11-15  8:14 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: ebiggers, jannh, dh.herrmann, Jiri Kosina,
	open list:HID CORE LAYER, lkml, syzkaller-bugs, dvyukov,
	syzbot+72473edc9bf4eb1c6556, 3.8+,
	luto

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2] HID: uhid: forbid UHID_CREATE under KERNEL_DS or elevated privileges
  2018-11-15  8:14                       ` Benjamin Tissoires
@ 2018-11-15 12:06                         ` David Herrmann
  2018-11-15 14:50                           ` Andy Lutomirski
  0 siblings, 1 reply; 25+ messages in thread
From: David Herrmann @ 2018-11-15 12:06 UTC (permalink / raw)
  To: Benjamin Tissoires
  Cc: Dmitry Torokhov, ebiggers, jannh, Jiri Kosina,
	open list:HID CORE LAYER, linux-kernel, syzkaller-bugs,
	Dmitry Vyukov, syzbot+72473edc9bf4eb1c6556, stable,
	Andy Lutomirski

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2] HID: uhid: forbid UHID_CREATE under KERNEL_DS or elevated privileges
  2018-11-14 21:55             ` [PATCH v2] HID: uhid: forbid UHID_CREATE under KERNEL_DS or elevated privileges Eric Biggers
  2018-11-14 22:04               ` Jann Horn
@ 2018-11-15 12:09               ` David Herrmann
  2018-11-15 14:49                 ` Andy Lutomirski
  2018-11-19 12:52               ` Jiri Kosina
  2 siblings, 1 reply; 25+ messages in thread
From: David Herrmann @ 2018-11-15 12:09 UTC (permalink / raw)
  To: ebiggers
  Cc: Jiri Kosina, Benjamin Tissoires, open list:HID CORE LAYER,
	linux-kernel, syzkaller-bugs, Dmitry Vyukov, Dmitry Torokhov,
	syzbot+72473edc9bf4eb1c6556, stable, jannh, Andy Lutomirski

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
>

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2] HID: uhid: forbid UHID_CREATE under KERNEL_DS or elevated privileges
  2018-11-15 12:09               ` David Herrmann
@ 2018-11-15 14:49                 ` Andy Lutomirski
  0 siblings, 0 replies; 25+ messages in thread
From: Andy Lutomirski @ 2018-11-15 14:49 UTC (permalink / raw)
  To: David Herrmann
  Cc: ebiggers, Jiri Kosina, Benjamin Tissoires,
	open list:HID CORE LAYER, linux-kernel, syzkaller-bugs,
	Dmitry Vyukov, Dmitry Torokhov, syzbot+72473edc9bf4eb1c6556,
	stable, jannh, Andy Lutomirski


> 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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2] HID: uhid: forbid UHID_CREATE under KERNEL_DS or elevated privileges
  2018-11-15 12:06                         ` David Herrmann
@ 2018-11-15 14:50                           ` Andy Lutomirski
  0 siblings, 0 replies; 25+ messages in thread
From: Andy Lutomirski @ 2018-11-15 14:50 UTC (permalink / raw)
  To: David Herrmann
  Cc: Benjamin Tissoires, Dmitry Torokhov, ebiggers, jannh,
	Jiri Kosina, open list:HID CORE LAYER, linux-kernel,
	syzkaller-bugs, Dmitry Vyukov, syzbot+72473edc9bf4eb1c6556,
	stable, Andy Lutomirski



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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2] HID: uhid: forbid UHID_CREATE under KERNEL_DS or elevated privileges
  2018-11-14 21:55             ` [PATCH v2] HID: uhid: forbid UHID_CREATE under KERNEL_DS or elevated privileges Eric Biggers
  2018-11-14 22:04               ` Jann Horn
  2018-11-15 12:09               ` David Herrmann
@ 2018-11-19 12:52               ` Jiri Kosina
  2018-11-19 13:21                 ` David Herrmann
  2 siblings, 1 reply; 25+ messages in thread
From: Jiri Kosina @ 2018-11-19 12:52 UTC (permalink / raw)
  To: Eric Biggers
  Cc: David Herrmann, Benjamin Tissoires, linux-input, linux-kernel,
	syzkaller-bugs, Dmitry Vyukov, Dmitry Torokhov,
	syzbot+72473edc9bf4eb1c6556, stable, Jann Horn, Andy Lutomirski,
	David Herrmann


[ 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,
 };

-- 
Jiri Kosina
SUSE Labs


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2] HID: uhid: forbid UHID_CREATE under KERNEL_DS or elevated privileges
  2018-11-19 12:52               ` Jiri Kosina
@ 2018-11-19 13:21                 ` David Herrmann
  2018-11-19 13:26                   ` Jiri Kosina
  0 siblings, 1 reply; 25+ messages in thread
From: David Herrmann @ 2018-11-19 13:21 UTC (permalink / raw)
  To: Jiri Kosina
  Cc: ebiggers, Benjamin Tissoires, open list:HID CORE LAYER,
	linux-kernel, syzkaller-bugs, Dmitry Vyukov, Dmitry Torokhov,
	syzbot+72473edc9bf4eb1c6556, stable, Jann Horn, Andy Lutomirski

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2] HID: uhid: forbid UHID_CREATE under KERNEL_DS or elevated privileges
  2018-11-19 13:21                 ` David Herrmann
@ 2018-11-19 13:26                   ` Jiri Kosina
  0 siblings, 0 replies; 25+ messages in thread
From: Jiri Kosina @ 2018-11-19 13:26 UTC (permalink / raw)
  To: David Herrmann
  Cc: ebiggers, Benjamin Tissoires, open list:HID CORE LAYER,
	linux-kernel, syzkaller-bugs, Dmitry Vyukov, Dmitry Torokhov,
	syzbot+72473edc9bf4eb1c6556, stable, Jann Horn, Andy Lutomirski

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,

-- 
Jiri Kosina
SUSE Labs


^ permalink raw reply	[flat|nested] 25+ messages in thread

end of thread, other threads:[~2018-11-19 13:26 UTC | newest]

Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-11 18:26 BUG: GPF in non-whitelisted uaccess (non-canonical address?) syzbot
2018-11-14  0:25 ` syzbot
2018-11-14 12:20   ` David Herrmann
2018-11-14 16:52     ` Dmitry Vyukov
2018-11-14 17:14       ` Eric Biggers
2018-11-14 18:02         ` [PATCH] HID: uhid: prevent uhid_char_write() under KERNEL_DS Eric Biggers
2018-11-14 18:14           ` Dmitry Torokhov
2018-11-14 18:18           ` Jann Horn
2018-11-14 21:54             ` Eric Biggers
2018-11-14 21:55             ` [PATCH v2] HID: uhid: forbid UHID_CREATE under KERNEL_DS or elevated privileges Eric Biggers
2018-11-14 22:04               ` Jann Horn
2018-11-14 22:28                 ` Dmitry Torokhov
2018-11-14 22:37                   ` Jann Horn
2018-11-14 22:46                     ` Dmitry Torokhov
2018-11-15  0:39                       ` Andy Lutomirski
2018-11-14 23:00                   ` Eric Biggers
2018-11-14 23:20                     ` Dmitry Torokhov
2018-11-15  8:14                       ` Benjamin Tissoires
2018-11-15 12:06                         ` David Herrmann
2018-11-15 14:50                           ` Andy Lutomirski
2018-11-15 12:09               ` David Herrmann
2018-11-15 14:49                 ` Andy Lutomirski
2018-11-19 12:52               ` Jiri Kosina
2018-11-19 13:21                 ` David Herrmann
2018-11-19 13:26                   ` Jiri Kosina

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).