]>
Commit | Line | Data |
---|---|---|
5417ad6e GKH |
1 | From 46ca3f735f345c9d87383dd3a09fa5d43870770e Mon Sep 17 00:00:00 2001 |
2 | From: Sergei Trofimovich <slyfox@gentoo.org> | |
3 | Date: Sun, 10 Mar 2019 21:24:15 +0000 | |
4 | Subject: tty/vt: fix write/write race in ioctl(KDSKBSENT) handler | |
5 | ||
6 | From: Sergei Trofimovich <slyfox@gentoo.org> | |
7 | ||
8 | commit 46ca3f735f345c9d87383dd3a09fa5d43870770e upstream. | |
9 | ||
10 | The bug manifests as an attempt to access deallocated memory: | |
11 | ||
12 | BUG: unable to handle kernel paging request at ffff9c8735448000 | |
13 | #PF error: [PROT] [WRITE] | |
14 | PGD 288a05067 P4D 288a05067 PUD 288a07067 PMD 7f60c2063 PTE 80000007f5448161 | |
15 | Oops: 0003 [#1] PREEMPT SMP | |
16 | CPU: 6 PID: 388 Comm: loadkeys Tainted: G C 5.0.0-rc6-00153-g5ded5871030e #91 | |
17 | Hardware name: Gigabyte Technology Co., Ltd. To be filled by O.E.M./H77M-D3H, BIOS F12 11/14/2013 | |
18 | RIP: 0010:__memmove+0x81/0x1a0 | |
19 | Code: 4c 89 4f 10 4c 89 47 18 48 8d 7f 20 73 d4 48 83 c2 20 e9 a2 00 00 00 66 90 48 89 d1 4c 8b 5c 16 f8 4c 8d 54 17 f8 48 c1 e9 03 <f3> 48 a5 4d 89 1a e9 0c 01 00 00 0f 1f 40 00 48 89 d1 4c 8b 1e 49 | |
20 | RSP: 0018:ffffa1b9002d7d08 EFLAGS: 00010203 | |
21 | RAX: ffff9c873541af43 RBX: ffff9c873541af43 RCX: 00000c6f105cd6bf | |
22 | RDX: 0000637882e986b6 RSI: ffff9c8735447ffb RDI: ffff9c8735447ffb | |
23 | RBP: ffff9c8739cd3800 R08: ffff9c873b802f00 R09: 00000000fffff73b | |
24 | R10: ffffffffb82b35f1 R11: 00505b1b004d5b1b R12: 0000000000000000 | |
25 | R13: ffff9c873541af3d R14: 000000000000000b R15: 000000000000000c | |
26 | FS: 00007f450c390580(0000) GS:ffff9c873f180000(0000) knlGS:0000000000000000 | |
27 | CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 | |
28 | CR2: ffff9c8735448000 CR3: 00000007e213c002 CR4: 00000000000606e0 | |
29 | Call Trace: | |
30 | vt_do_kdgkb_ioctl+0x34d/0x440 | |
31 | vt_ioctl+0xba3/0x1190 | |
32 | ? __bpf_prog_run32+0x39/0x60 | |
33 | ? mem_cgroup_commit_charge+0x7b/0x4e0 | |
34 | tty_ioctl+0x23f/0x920 | |
35 | ? preempt_count_sub+0x98/0xe0 | |
36 | ? __seccomp_filter+0x67/0x600 | |
37 | do_vfs_ioctl+0xa2/0x6a0 | |
38 | ? syscall_trace_enter+0x192/0x2d0 | |
39 | ksys_ioctl+0x3a/0x70 | |
40 | __x64_sys_ioctl+0x16/0x20 | |
41 | do_syscall_64+0x54/0xe0 | |
42 | entry_SYSCALL_64_after_hwframe+0x49/0xbe | |
43 | ||
44 | The bug manifests on systemd systems with multiple vtcon devices: | |
45 | # cat /sys/devices/virtual/vtconsole/vtcon0/name | |
46 | (S) dummy device | |
47 | # cat /sys/devices/virtual/vtconsole/vtcon1/name | |
48 | (M) frame buffer device | |
49 | ||
50 | There systemd runs 'loadkeys' tool in tapallel for each vtcon | |
51 | instance. This causes two parallel ioctl(KDSKBSENT) calls to | |
52 | race into adding the same entry into 'func_table' array at: | |
53 | ||
54 | drivers/tty/vt/keyboard.c:vt_do_kdgkb_ioctl() | |
55 | ||
56 | The function has no locking around writes to 'func_table'. | |
57 | ||
58 | The simplest reproducer is to have initrams with the following | |
59 | init on a 8-CPU machine x86_64: | |
60 | ||
61 | #!/bin/sh | |
62 | ||
63 | loadkeys -q windowkeys ru4 & | |
64 | loadkeys -q windowkeys ru4 & | |
65 | loadkeys -q windowkeys ru4 & | |
66 | loadkeys -q windowkeys ru4 & | |
67 | ||
68 | loadkeys -q windowkeys ru4 & | |
69 | loadkeys -q windowkeys ru4 & | |
70 | loadkeys -q windowkeys ru4 & | |
71 | loadkeys -q windowkeys ru4 & | |
72 | wait | |
73 | ||
74 | The change adds lock on write path only. Reads are still racy. | |
75 | ||
76 | CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org> | |
77 | CC: Jiri Slaby <jslaby@suse.com> | |
78 | Link: https://lkml.org/lkml/2019/2/17/256 | |
79 | Signed-off-by: Sergei Trofimovich <slyfox@gentoo.org> | |
80 | Cc: stable <stable@vger.kernel.org> | |
81 | Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> | |
82 | Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> | |
83 | ||
84 | --- | |
85 | drivers/tty/vt/keyboard.c | 33 +++++++++++++++++++++++++++------ | |
86 | 1 file changed, 27 insertions(+), 6 deletions(-) | |
87 | ||
88 | --- a/drivers/tty/vt/keyboard.c | |
89 | +++ b/drivers/tty/vt/keyboard.c | |
90 | @@ -121,6 +121,7 @@ static const int NR_TYPES = ARRAY_SIZE(m | |
91 | static struct input_handler kbd_handler; | |
92 | static DEFINE_SPINLOCK(kbd_event_lock); | |
93 | static DEFINE_SPINLOCK(led_lock); | |
94 | +static DEFINE_SPINLOCK(func_buf_lock); /* guard 'func_buf' and friends */ | |
95 | static unsigned long key_down[BITS_TO_LONGS(KEY_CNT)]; /* keyboard key bitmap */ | |
96 | static unsigned char shift_down[NR_SHIFT]; /* shift state counters.. */ | |
97 | static bool dead_key_next; | |
98 | @@ -1969,11 +1970,12 @@ int vt_do_kdgkb_ioctl(int cmd, struct kb | |
99 | char *p; | |
100 | u_char *q; | |
101 | u_char __user *up; | |
102 | - int sz; | |
103 | + int sz, fnw_sz; | |
104 | int delta; | |
105 | char *first_free, *fj, *fnw; | |
106 | int i, j, k; | |
107 | int ret; | |
108 | + unsigned long flags; | |
109 | ||
110 | if (!capable(CAP_SYS_TTY_CONFIG)) | |
111 | perm = 0; | |
112 | @@ -2016,7 +2018,14 @@ int vt_do_kdgkb_ioctl(int cmd, struct kb | |
113 | goto reterr; | |
114 | } | |
115 | ||
116 | + fnw = NULL; | |
117 | + fnw_sz = 0; | |
118 | + /* race aginst other writers */ | |
119 | + again: | |
120 | + spin_lock_irqsave(&func_buf_lock, flags); | |
121 | q = func_table[i]; | |
122 | + | |
123 | + /* fj pointer to next entry after 'q' */ | |
124 | first_free = funcbufptr + (funcbufsize - funcbufleft); | |
125 | for (j = i+1; j < MAX_NR_FUNC && !func_table[j]; j++) | |
126 | ; | |
127 | @@ -2024,10 +2033,12 @@ int vt_do_kdgkb_ioctl(int cmd, struct kb | |
128 | fj = func_table[j]; | |
129 | else | |
130 | fj = first_free; | |
131 | - | |
132 | + /* buffer usage increase by new entry */ | |
133 | delta = (q ? -strlen(q) : 1) + strlen(kbs->kb_string); | |
134 | + | |
135 | if (delta <= funcbufleft) { /* it fits in current buf */ | |
136 | if (j < MAX_NR_FUNC) { | |
137 | + /* make enough space for new entry at 'fj' */ | |
138 | memmove(fj + delta, fj, first_free - fj); | |
139 | for (k = j; k < MAX_NR_FUNC; k++) | |
140 | if (func_table[k]) | |
141 | @@ -2040,20 +2051,28 @@ int vt_do_kdgkb_ioctl(int cmd, struct kb | |
142 | sz = 256; | |
143 | while (sz < funcbufsize - funcbufleft + delta) | |
144 | sz <<= 1; | |
145 | - fnw = kmalloc(sz, GFP_KERNEL); | |
146 | - if(!fnw) { | |
147 | - ret = -ENOMEM; | |
148 | - goto reterr; | |
149 | + if (fnw_sz != sz) { | |
150 | + spin_unlock_irqrestore(&func_buf_lock, flags); | |
151 | + kfree(fnw); | |
152 | + fnw = kmalloc(sz, GFP_KERNEL); | |
153 | + fnw_sz = sz; | |
154 | + if (!fnw) { | |
155 | + ret = -ENOMEM; | |
156 | + goto reterr; | |
157 | + } | |
158 | + goto again; | |
159 | } | |
160 | ||
161 | if (!q) | |
162 | func_table[i] = fj; | |
163 | + /* copy data before insertion point to new location */ | |
164 | if (fj > funcbufptr) | |
165 | memmove(fnw, funcbufptr, fj - funcbufptr); | |
166 | for (k = 0; k < j; k++) | |
167 | if (func_table[k]) | |
168 | func_table[k] = fnw + (func_table[k] - funcbufptr); | |
169 | ||
170 | + /* copy data after insertion point to new location */ | |
171 | if (first_free > fj) { | |
172 | memmove(fnw + (fj - funcbufptr) + delta, fj, first_free - fj); | |
173 | for (k = j; k < MAX_NR_FUNC; k++) | |
174 | @@ -2066,7 +2085,9 @@ int vt_do_kdgkb_ioctl(int cmd, struct kb | |
175 | funcbufleft = funcbufleft - delta + sz - funcbufsize; | |
176 | funcbufsize = sz; | |
177 | } | |
178 | + /* finally insert item itself */ | |
179 | strcpy(func_table[i], kbs->kb_string); | |
180 | + spin_unlock_irqrestore(&func_buf_lock, flags); | |
181 | break; | |
182 | } | |
183 | ret = 0; |