* [PATCH] epoll: fix use-after-free in eventpoll_release_file
@ 2014-06-17 2:58 Konstantin Khlebnikov
2014-06-17 3:02 ` Konstantin Khlebnikov
` (2 more replies)
0 siblings, 3 replies; 4+ messages in thread
From: Konstantin Khlebnikov @ 2014-06-17 2:58 UTC (permalink / raw)
To: Andrew Morton, linux-kernel
Cc: Cyrill Gorcunov, Sasha Levin, Jason Baron, Linus Torvalds, Stable
This fixes use-after-free of epi->fllink.next inside list loop macro.
This loop actually releases elements in the body. List is rcu-protected
but here we cannot hold rcu_read_lock because we need to lock mutex inside.
Obvious solution is to use list_for_each_entry_safe(). RCU-ness isn't essential
because nobody can change this list under us, it's final fput for this file.
Bug is here since ae10b2b4eb01bedc91d29d5c5bb9e416fd806c40
("epoll: optimize EPOLL_CTL_DEL using rcu")
Signed-off-by: Konstantin Khlebnikov <koct9i@gmail.com>
Reported-by: Cyrill Gorcunov <gorcunov@openvz.org>
Cc: Stable <stable@vger.kernel.org> # 3.13+
Cc: Sasha Levin <sasha.levin@oracle.com>
Cc: Jason Baron <jbaron@akamai.com>
---
fs/eventpoll.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/fs/eventpoll.c b/fs/eventpoll.c
index b73e062..b10b48c 100644
--- a/fs/eventpoll.c
+++ b/fs/eventpoll.c
@@ -910,7 +910,7 @@ static const struct file_operations eventpoll_fops = {
void eventpoll_release_file(struct file *file)
{
struct eventpoll *ep;
- struct epitem *epi;
+ struct epitem *epi, *next;
/*
* We don't want to get "file->f_lock" because it is not
@@ -926,7 +926,7 @@ void eventpoll_release_file(struct file *file)
* Besides, ep_remove() acquires the lock, so we can't hold it here.
*/
mutex_lock(&epmutex);
- list_for_each_entry_rcu(epi, &file->f_ep_links, fllink) {
+ list_for_each_entry_safe(epi, next, &file->f_ep_links, fllink) {
ep = epi->ep;
mutex_lock_nested(&ep->mtx, 0);
ep_remove(ep, epi);
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] epoll: fix use-after-free in eventpoll_release_file
2014-06-17 2:58 [PATCH] epoll: fix use-after-free in eventpoll_release_file Konstantin Khlebnikov
@ 2014-06-17 3:02 ` Konstantin Khlebnikov
2014-06-17 5:50 ` Cyrill Gorcunov
2014-06-17 16:40 ` Jason Baron
2 siblings, 0 replies; 4+ messages in thread
From: Konstantin Khlebnikov @ 2014-06-17 3:02 UTC (permalink / raw)
To: Andrew Morton, Linux Kernel Mailing List
Cc: Cyrill Gorcunov, Sasha Levin, Jason Baron, Linus Torvalds, Stable
On Tue, Jun 17, 2014 at 6:58 AM, Konstantin Khlebnikov <koct9i@gmail.com> wrote:
> This fixes use-after-free of epi->fllink.next inside list loop macro.
> This loop actually releases elements in the body. List is rcu-protected
> but here we cannot hold rcu_read_lock because we need to lock mutex inside.
>
> Obvious solution is to use list_for_each_entry_safe(). RCU-ness isn't essential
> because nobody can change this list under us, it's final fput for this file.
>
> Bug is here since ae10b2b4eb01bedc91d29d5c5bb9e416fd806c40
> ("epoll: optimize EPOLL_CTL_DEL using rcu")
>
> Signed-off-by: Konstantin Khlebnikov <koct9i@gmail.com>
> Reported-by: Cyrill Gorcunov <gorcunov@openvz.org>
> Cc: Stable <stable@vger.kernel.org> # 3.13+
> Cc: Sasha Levin <sasha.levin@oracle.com>
> Cc: Jason Baron <jbaron@akamai.com>
> ---
oops from Cyrill's report
https://plus.google.com/+CyrillGorcunov/posts/RPGi1ECc3Tk
[ 14.448483] general protection fault: 0000 [#1] [ 14.448483]
general protection fault: 0000 [#1] PREEMPT PREEMPT SMP SMP
[ 14.448533] Modules linked in:[ 14.448533] Modules linked in:
[ 14.448533] CPU: 0 PID: 1153 Comm: plymouthd Not tainted 3.16.0-rc1 #463
[ 14.448533] CPU: 0 PID: 1153 Comm: plymouthd Not tainted 3.16.0-rc1 #463
[ 14.448533] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
[ 14.448533] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
[ 14.448533] task: ffff880037a28f50 ti: ffff880037a2c000 task.ti:
ffff880037a2c000
[ 14.448533] task: ffff880037a28f50 ti: ffff880037a2c000 task.ti:
ffff880037a2c000
[ 14.448533] RIP: 0010:[<ffffffff810abee9>] [ 14.448533] RIP:
0010:[<ffffffff810abee9>] [<ffffffff810abee9>]
__lock_acquire+0xeb/0xf07
[<ffffffff810abee9>] __lock_acquire+0xeb/0xf07
[ 14.448533] RSP: 0018:ffff880037a2fcf0 EFLAGS: 00010002
[ 14.448533] RSP: 0018:ffff880037a2fcf0 EFLAGS: 00010002
[ 14.448533] RAX: 6b6b6b6b6b6b6b6b RBX: ffff880037a28f50 RCX: 0000000000000000
[ 14.448533] RAX: 6b6b6b6b6b6b6b6b RBX: ffff880037a28f50 RCX: 0000000000000000
[ 14.448533] RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffff88003b960200
[ 14.448533] RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffff88003b960200
[ 14.448533] RBP: ffff880037a2fd88 R08: 0000000000000001 R09: 0000000000000000
[ 14.448533] RBP: ffff880037a2fd88 R08: 0000000000000001 R09: 0000000000000000
[ 14.448533] R10: 0000000000000000 R11: 0000000000000000 R12: ffff880037a28f50
[ 14.448533] R10: 0000000000000000 R11: 0000000000000000 R12: ffff880037a28f50
[ 14.448533] R13: 0000000000000000 R14: 0000000000000001 R15: ffff88003b960200
[ 14.448533] R13: 0000000000000000 R14: 0000000000000001 R15: ffff88003b960200
[ 14.448533] FS: 00007fafb700d740(0000) GS:ffff88003fc00000(0000)
knlGS:0000000000000000
[ 14.448533] FS: 00007fafb700d740(0000) GS:ffff88003fc00000(0000)
knlGS:0000000000000000
[ 14.448533] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 14.448533] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 14.448533] CR2: 00007fd6cc054414 CR3: 00000000378ee000 CR4: 00000000000006f0
[ 14.448533] CR2: 00007fd6cc054414 CR3: 00000000378ee000 CR4: 00000000000006f0
[ 14.448533] Stack:
[ 14.448533] Stack:
[ 14.448533] ffff880000000000[ 14.448533] ffff880000000000
ffff880000000001 ffff880000000001 ffffffff00000000 ffffffff00000000
ffff880000000000 ffff880000000000
[ 14.448533] ffffffff00000000[ 14.448533] ffffffff00000000
ffff88003fdcd2b0 ffff88003fdcd2b0 ffffffff81c41d80 ffffffff81c41d80
ffff880037a2fe30 ffff880037a2fe30
[ 14.448533] ffffffff81303a7e[ 14.448533] ffffffff81303a7e
0000000000000000 0000000000000000 0000000000000000 0000000000000000
00000000009c0496 00000000009c0496
[ 14.448533] Call Trace:
[ 14.448533] Call Trace:
[ 14.448533] [<ffffffff81303a7e>] ? trace_hardirqs_on_thunk+0x3a/0x3f
[ 14.448533] [<ffffffff81303a7e>] ? trace_hardirqs_on_thunk+0x3a/0x3f
[ 14.448533] [<ffffffff810ace03>] lock_acquire+0xfe/0x146
[ 14.448533] [<ffffffff810ace03>] lock_acquire+0xfe/0x146
[ 14.448533] [<ffffffff811a97c5>] ? eventpoll_release_file+0x68/0xc1
[ 14.448533] [<ffffffff811a97c5>] ? eventpoll_release_file+0x68/0xc1
[ 14.448533] [<ffffffff811a97c5>] ? eventpoll_release_file+0x68/0xc1
[ 14.448533] [<ffffffff811a97c5>] ? eventpoll_release_file+0x68/0xc1
[ 14.448533] [<ffffffff81639ffa>] mutex_lock_nested+0x77/0x3f0
[ 14.448533] [<ffffffff81639ffa>] mutex_lock_nested+0x77/0x3f0
[ 14.448533] [<ffffffff811a97c5>] ? eventpoll_release_file+0x68/0xc1
[ 14.448533] [<ffffffff811a97c5>] ? eventpoll_release_file+0x68/0xc1
[ 14.448533] [<ffffffff811a97c5>] ? eventpoll_release_file+0x68/0xc1
[ 14.448533] [<ffffffff811a97c5>] ? eventpoll_release_file+0x68/0xc1
[ 14.448533] [<ffffffff811a97c5>] eventpoll_release_file+0x68/0xc1
[ 14.448533] [<ffffffff811a97c5>] eventpoll_release_file+0x68/0xc1
[ 14.448533] [<ffffffff8117451c>] __fput+0xc5/0x1b6
[ 14.448533] [<ffffffff8117451c>] __fput+0xc5/0x1b6
[ 14.448533] [<ffffffff81174643>] ____fput+0xe/0x10
[ 14.448533] [<ffffffff81174643>] ____fput+0xe/0x10
[ 14.448533] [<ffffffff81085643>] task_work_run+0x7d/0xa6
[ 14.448533] [<ffffffff81085643>] task_work_run+0x7d/0xa6
[ 14.448533] [<ffffffff81002799>] do_notify_resume+0x63/0x68
[ 14.448533] [<ffffffff81002799>] do_notify_resume+0x63/0x68
[ 14.448533] [<ffffffff8163df1a>] int_signal+0x12/0x17
[ 14.448533] [<ffffffff8163df1a>] int_signal+0x12/0x17
[ 14.448533] Code: [ 14.448533] Code: 85 85 c0 c0 75 75 27 27 31
31 d2 d2 4c 4c 89 89 ff ff 44 44 89 89 4c 4c 24 24 08 08 44 44 89 89
44 44 24 24 10 10 e8 e8 10 10 f7 f7 ff ff ff ff 48 48 85 85 c0 c0 44
44 8b 8b 44 44 24 24 10 10 44 44 8b 8b 4c 4c 24 24 08 08 0f 0f 84 84
1b 1b 0d 0d 00 00 00 00 <3e> <3e> ff ff 80 80 98 98 01 01 00 00 00 00
83 83 3d 3d 91 91 1e 1e cb cb 01 01 00 00 45 45 8b 8b b4 b4 24 24 50
50 08 08 00 00
[ 14.448533] RIP [ 14.448533] RIP [<ffffffff810abee9>]
__lock_acquire+0xeb/0xf07
[<ffffffff810abee9>] __lock_acquire+0xeb/0xf07
[ 14.448533] RSP <ffff880037a2fcf0>
[ 14.448533] RSP <ffff880037a2fcf0>
[ 14.448533] ---[ end trace b64669d4daf5e235 ]---
[ 14.448533] -[ end trace b64669d4daf5e235 ]-
[ 14.448533] Kernel panic - not syncing: Fatal exception
[ 14.448533] Kernel panic - not syncing: Fatal exception
[ 14.448533] Kernel Offset: 0x0 from 0xffffffff81000000 (relocation
range: 0xffffffff80000000-0xffffffff9fffffff)
[ 14.448533] Kernel Offset: 0x0 from 0xffffffff81000000 (relocation
range: 0xffffffff80000000-0xffffffff9fffffff)
[ 14.448533] -[ end Kernel panic - not syncing: Fatal exception
[ 14.448533] -[ end Kernel panic - not syncing: Fatal exception
> fs/eventpoll.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/eventpoll.c b/fs/eventpoll.c
> index b73e062..b10b48c 100644
> --- a/fs/eventpoll.c
> +++ b/fs/eventpoll.c
> @@ -910,7 +910,7 @@ static const struct file_operations eventpoll_fops = {
> void eventpoll_release_file(struct file *file)
> {
> struct eventpoll *ep;
> - struct epitem *epi;
> + struct epitem *epi, *next;
>
> /*
> * We don't want to get "file->f_lock" because it is not
> @@ -926,7 +926,7 @@ void eventpoll_release_file(struct file *file)
> * Besides, ep_remove() acquires the lock, so we can't hold it here.
> */
> mutex_lock(&epmutex);
> - list_for_each_entry_rcu(epi, &file->f_ep_links, fllink) {
> + list_for_each_entry_safe(epi, next, &file->f_ep_links, fllink) {
> ep = epi->ep;
> mutex_lock_nested(&ep->mtx, 0);
> ep_remove(ep, epi);
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] epoll: fix use-after-free in eventpoll_release_file
2014-06-17 2:58 [PATCH] epoll: fix use-after-free in eventpoll_release_file Konstantin Khlebnikov
2014-06-17 3:02 ` Konstantin Khlebnikov
@ 2014-06-17 5:50 ` Cyrill Gorcunov
2014-06-17 16:40 ` Jason Baron
2 siblings, 0 replies; 4+ messages in thread
From: Cyrill Gorcunov @ 2014-06-17 5:50 UTC (permalink / raw)
To: Konstantin Khlebnikov
Cc: Andrew Morton, linux-kernel, Sasha Levin, Jason Baron,
Linus Torvalds, Stable
On Tue, Jun 17, 2014 at 06:58:05AM +0400, Konstantin Khlebnikov wrote:
> This fixes use-after-free of epi->fllink.next inside list loop macro.
> This loop actually releases elements in the body. List is rcu-protected
> but here we cannot hold rcu_read_lock because we need to lock mutex inside.
>
> Obvious solution is to use list_for_each_entry_safe(). RCU-ness isn't essential
> because nobody can change this list under us, it's final fput for this file.
>
> Bug is here since ae10b2b4eb01bedc91d29d5c5bb9e416fd806c40
> ("epoll: optimize EPOLL_CTL_DEL using rcu")
>
> Signed-off-by: Konstantin Khlebnikov <koct9i@gmail.com>
> Reported-by: Cyrill Gorcunov <gorcunov@openvz.org>
> Cc: Stable <stable@vger.kernel.org> # 3.13+
> Cc: Sasha Levin <sasha.levin@oracle.com>
> Cc: Jason Baron <jbaron@akamai.com>
Acked-by: Cyrill Gorcunov <gorcunov@openvz.org>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] epoll: fix use-after-free in eventpoll_release_file
2014-06-17 2:58 [PATCH] epoll: fix use-after-free in eventpoll_release_file Konstantin Khlebnikov
2014-06-17 3:02 ` Konstantin Khlebnikov
2014-06-17 5:50 ` Cyrill Gorcunov
@ 2014-06-17 16:40 ` Jason Baron
2 siblings, 0 replies; 4+ messages in thread
From: Jason Baron @ 2014-06-17 16:40 UTC (permalink / raw)
To: Konstantin Khlebnikov
Cc: Andrew Morton, linux-kernel@vger.kernel.org, Cyrill Gorcunov,
Sasha Levin, Linus Torvalds, Stable
Hi,
Indeed, eventpoll_release_file() is on the 'write' side of the rcu
here (the read-side being in reverse_path_check_proc()). It's a race,
in that 'struct epitem' has to be freed by rcu before the next loop
iteration such that epi->fllink.next pointer is no longer valid. So,
I suspect that makes this more difficult to hit, but obviously it
happens.
Thanks for finding/fixing this!
Acked-by: Jason Baron <jbaron@akamai.com>
On 06/16/2014 10:58 PM, Konstantin Khlebnikov wrote:
> This fixes use-after-free of epi->fllink.next inside list loop macro.
> This loop actually releases elements in the body. List is rcu-protected
> but here we cannot hold rcu_read_lock because we need to lock mutex inside.
>
> Obvious solution is to use list_for_each_entry_safe(). RCU-ness isn't essential
> because nobody can change this list under us, it's final fput for this file.
>
> Bug is here since ae10b2b4eb01bedc91d29d5c5bb9e416fd806c40
> ("epoll: optimize EPOLL_CTL_DEL using rcu")
>
> Signed-off-by: Konstantin Khlebnikov <koct9i@gmail.com>
> Reported-by: Cyrill Gorcunov <gorcunov@openvz.org>
> Cc: Stable <stable@vger.kernel.org> # 3.13+
> Cc: Sasha Levin <sasha.levin@oracle.com>
> Cc: Jason Baron <jbaron@akamai.com>
> ---
> fs/eventpoll.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/eventpoll.c b/fs/eventpoll.c
> index b73e062..b10b48c 100644
> --- a/fs/eventpoll.c
> +++ b/fs/eventpoll.c
> @@ -910,7 +910,7 @@ static const struct file_operations eventpoll_fops = {
> void eventpoll_release_file(struct file *file)
> {
> struct eventpoll *ep;
> - struct epitem *epi;
> + struct epitem *epi, *next;
>
> /*
> * We don't want to get "file->f_lock" because it is not
> @@ -926,7 +926,7 @@ void eventpoll_release_file(struct file *file)
> * Besides, ep_remove() acquires the lock, so we can't hold it here.
> */
> mutex_lock(&epmutex);
> - list_for_each_entry_rcu(epi, &file->f_ep_links, fllink) {
> + list_for_each_entry_safe(epi, next, &file->f_ep_links, fllink) {
> ep = epi->ep;
> mutex_lock_nested(&ep->mtx, 0);
> ep_remove(ep, epi);
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-06-17 16:40 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-06-17 2:58 [PATCH] epoll: fix use-after-free in eventpoll_release_file Konstantin Khlebnikov
2014-06-17 3:02 ` Konstantin Khlebnikov
2014-06-17 5:50 ` Cyrill Gorcunov
2014-06-17 16:40 ` Jason Baron
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox