1 |
From 68faa679b8be1a74e6663c21c3a9d25d32f1c079 Mon Sep 17 00:00:00 2001 |
2 |
From: Will Deacon <will@kernel.org> |
3 |
Date: Thu, 19 Dec 2019 12:02:03 +0000 |
4 |
Subject: chardev: Avoid potential use-after-free in 'chrdev_open()' |
5 |
|
6 |
From: Will Deacon <will@kernel.org> |
7 |
|
8 |
commit 68faa679b8be1a74e6663c21c3a9d25d32f1c079 upstream. |
9 |
|
10 |
'chrdev_open()' calls 'cdev_get()' to obtain a reference to the |
11 |
'struct cdev *' stashed in the 'i_cdev' field of the target inode |
12 |
structure. If the pointer is NULL, then it is initialised lazily by |
13 |
looking up the kobject in the 'cdev_map' and so the whole procedure is |
14 |
protected by the 'cdev_lock' spinlock to serialise initialisation of |
15 |
the shared pointer. |
16 |
|
17 |
Unfortunately, it is possible for the initialising thread to fail *after* |
18 |
installing the new pointer, for example if the subsequent '->open()' call |
19 |
on the file fails. In this case, 'cdev_put()' is called, the reference |
20 |
count on the kobject is dropped and, if nobody else has taken a reference, |
21 |
the release function is called which finally clears 'inode->i_cdev' from |
22 |
'cdev_purge()' before potentially freeing the object. The problem here |
23 |
is that a racing thread can happily take the 'cdev_lock' and see the |
24 |
non-NULL pointer in the inode, which can result in a refcount increment |
25 |
from zero and a warning: |
26 |
|
27 |
| ------------[ cut here ]------------ |
28 |
| refcount_t: addition on 0; use-after-free. |
29 |
| WARNING: CPU: 2 PID: 6385 at lib/refcount.c:25 refcount_warn_saturate+0x6d/0xf0 |
30 |
| Modules linked in: |
31 |
| CPU: 2 PID: 6385 Comm: repro Not tainted 5.5.0-rc2+ #22 |
32 |
| Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-1 04/01/2014 |
33 |
| RIP: 0010:refcount_warn_saturate+0x6d/0xf0 |
34 |
| Code: 05 55 9a 15 01 01 e8 9d aa c8 ff 0f 0b c3 80 3d 45 9a 15 01 00 75 ce 48 c7 c7 00 9c 62 b3 c6 08 |
35 |
| RSP: 0018:ffffb524c1b9bc70 EFLAGS: 00010282 |
36 |
| RAX: 0000000000000000 RBX: ffff9e9da1f71390 RCX: 0000000000000000 |
37 |
| RDX: ffff9e9dbbd27618 RSI: ffff9e9dbbd18798 RDI: ffff9e9dbbd18798 |
38 |
| RBP: 0000000000000000 R08: 000000000000095f R09: 0000000000000039 |
39 |
| R10: 0000000000000000 R11: ffffb524c1b9bb20 R12: ffff9e9da1e8c700 |
40 |
| R13: ffffffffb25ee8b0 R14: 0000000000000000 R15: ffff9e9da1e8c700 |
41 |
| FS: 00007f3b87d26700(0000) GS:ffff9e9dbbd00000(0000) knlGS:0000000000000000 |
42 |
| CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 |
43 |
| CR2: 00007fc16909c000 CR3: 000000012df9c000 CR4: 00000000000006e0 |
44 |
| DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 |
45 |
| DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 |
46 |
| Call Trace: |
47 |
| kobject_get+0x5c/0x60 |
48 |
| cdev_get+0x2b/0x60 |
49 |
| chrdev_open+0x55/0x220 |
50 |
| ? cdev_put.part.3+0x20/0x20 |
51 |
| do_dentry_open+0x13a/0x390 |
52 |
| path_openat+0x2c8/0x1470 |
53 |
| do_filp_open+0x93/0x100 |
54 |
| ? selinux_file_ioctl+0x17f/0x220 |
55 |
| do_sys_open+0x186/0x220 |
56 |
| do_syscall_64+0x48/0x150 |
57 |
| entry_SYSCALL_64_after_hwframe+0x44/0xa9 |
58 |
| RIP: 0033:0x7f3b87efcd0e |
59 |
| Code: 89 54 24 08 e8 a3 f4 ff ff 8b 74 24 0c 48 8b 3c 24 41 89 c0 44 8b 54 24 08 b8 01 01 00 00 89 f4 |
60 |
| RSP: 002b:00007f3b87d259f0 EFLAGS: 00000293 ORIG_RAX: 0000000000000101 |
61 |
| RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f3b87efcd0e |
62 |
| RDX: 0000000000000000 RSI: 00007f3b87d25a80 RDI: 00000000ffffff9c |
63 |
| RBP: 00007f3b87d25e90 R08: 0000000000000000 R09: 0000000000000000 |
64 |
| R10: 0000000000000000 R11: 0000000000000293 R12: 00007ffe188f504e |
65 |
| R13: 00007ffe188f504f R14: 00007f3b87d26700 R15: 0000000000000000 |
66 |
| ---[ end trace 24f53ca58db8180a ]--- |
67 |
|
68 |
Since 'cdev_get()' can already fail to obtain a reference, simply move |
69 |
it over to use 'kobject_get_unless_zero()' instead of 'kobject_get()', |
70 |
which will cause the racing thread to return -ENXIO if the initialising |
71 |
thread fails unexpectedly. |
72 |
|
73 |
Cc: Hillf Danton <hdanton@sina.com> |
74 |
Cc: Andrew Morton <akpm@linux-foundation.org> |
75 |
Cc: Al Viro <viro@zeniv.linux.org.uk> |
76 |
Reported-by: syzbot+82defefbbd8527e1c2cb@syzkaller.appspotmail.com |
77 |
Signed-off-by: Will Deacon <will@kernel.org> |
78 |
Cc: stable <stable@vger.kernel.org> |
79 |
Link: https://lore.kernel.org/r/20191219120203.32691-1-will@kernel.org |
80 |
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> |
81 |
|
82 |
--- |
83 |
fs/char_dev.c | 2 +- |
84 |
1 file changed, 1 insertion(+), 1 deletion(-) |
85 |
|
86 |
--- a/fs/char_dev.c |
87 |
+++ b/fs/char_dev.c |
88 |
@@ -352,7 +352,7 @@ static struct kobject *cdev_get(struct c |
89 |
|
90 |
if (owner && !try_module_get(owner)) |
91 |
return NULL; |
92 |
- kobj = kobject_get(&p->kobj); |
93 |
+ kobj = kobject_get_unless_zero(&p->kobj); |
94 |
if (!kobj) |
95 |
module_put(owner); |
96 |
return kobj; |