* timerfd: use-after-free in timerfd_remove_cancel
@ 2017-01-30 18:41 Dmitry Vyukov
2017-01-31 2:06 ` Mateusz Guzik
2017-01-31 11:33 ` Thomas Gleixner
0 siblings, 2 replies; 6+ messages in thread
From: Dmitry Vyukov @ 2017-01-30 18:41 UTC (permalink / raw)
To: Al Viro, Thomas Gleixner, linux-fsdevel, LKML; +Cc: syzkaller
Hello,
The following program triggers use-after-free in timerfd_remove_cancel:
https://gist.githubusercontent.com/dvyukov/202576d437c84ffbbe52e9ccd77e1b44/raw/5562bff8626a73627157331ea2b837f59080ac84/gistfile1.txt
BUG: KASAN: use-after-free in __list_del include/linux/list.h:104
[inline] at addr ffff88006bab1410
BUG: KASAN: use-after-free in __list_del_entry
include/linux/list.h:119 [inline] at addr ffff88006bab1410
BUG: KASAN: use-after-free in list_del_rcu include/linux/rculist.h:129
[inline] at addr ffff88006bab1410
BUG: KASAN: use-after-free in timerfd_remove_cancel fs/timerfd.c:120
[inline] at addr ffff88006bab1410
BUG: KASAN: use-after-free in timerfd_release+0x28e/0x310
fs/timerfd.c:209 at addr ffff88006bab1410
Write of size 8 by task a.out/2897
CPU: 3 PID: 2897 Comm: a.out Not tainted 4.10.0-rc5+ #201
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
Call Trace:
__dump_stack lib/dump_stack.c:15 [inline]
dump_stack+0x2ee/0x3ef lib/dump_stack.c:51
kasan_object_err+0x1c/0x70 mm/kasan/report.c:165
print_address_description mm/kasan/report.c:203 [inline]
kasan_report_error+0x17b/0x430 mm/kasan/report.c:287
kasan_report mm/kasan/report.c:307 [inline]
__asan_report_store8_noabort+0x3e/0x40 mm/kasan/report.c:333
__list_del include/linux/list.h:104 [inline]
__list_del_entry include/linux/list.h:119 [inline]
list_del_rcu include/linux/rculist.h:129 [inline]
timerfd_remove_cancel fs/timerfd.c:120 [inline]
timerfd_release+0x28e/0x310 fs/timerfd.c:209
__fput+0x332/0x7f0 fs/file_table.c:208
____fput+0x15/0x20 fs/file_table.c:244
task_work_run+0x18a/0x260 kernel/task_work.c:116
tracehook_notify_resume include/linux/tracehook.h:191 [inline]
exit_to_usermode_loop+0x23b/0x2a0 arch/x86/entry/common.c:160
prepare_exit_to_usermode arch/x86/entry/common.c:190 [inline]
syscall_return_slowpath+0x4d3/0x570 arch/x86/entry/common.c:259
entry_SYSCALL_64_fastpath+0xc0/0xc2
RIP: 0033:0x4054e1
RSP: 002b:00007f46e349fd60 EFLAGS: 00000293 ORIG_RAX: 0000000000000003
RAX: 0000000000000000 RBX: 0000000000000000 RCX: 00000000004054e1
RDX: 0000000000000000 RSI: 0000000000801000 RDI: 0000000000000004
RBP: 00007f46e349fdd0 R08: 0000000000000000 R09: 00007f46a3fe7700
R10: 00007f46a3fe79d0 R11: 0000000000000293 R12: 0000000000000000
R13: 0000000000000000 R14: 00007f46e34a09c0 R15: 00007f46e34a0700
Object at ffff88006bab1300, in cache kmalloc-512 size: 512
Allocated:
PID = 2899
[<ffffffff812b1506>] save_stack_trace+0x16/0x20 arch/x86/kernel/stacktrace.c:57
[<ffffffff81a0cdd3>] save_stack+0x43/0xd0 mm/kasan/kasan.c:502
[<ffffffff81a0d09a>] set_track mm/kasan/kasan.c:514 [inline]
[<ffffffff81a0d09a>] kasan_kmalloc+0xaa/0xd0 mm/kasan/kasan.c:605
[<ffffffff81a098eb>] kmem_cache_alloc_trace+0x10b/0x670 mm/slab.c:3629
[<ffffffff81b8ab2f>] kmalloc include/linux/slab.h:490 [inline]
[<ffffffff81b8ab2f>] kzalloc include/linux/slab.h:636 [inline]
[<ffffffff81b8ab2f>] SYSC_timerfd_create fs/timerfd.c:398 [inline]
[<ffffffff81b8ab2f>] SyS_timerfd_create+0x1df/0x2d0 fs/timerfd.c:376
[<ffffffff841c9c81>] entry_SYSCALL_64_fastpath+0x1f/0xc2
Freed:
PID = 0
[<ffffffff812b1506>] save_stack_trace+0x16/0x20 arch/x86/kernel/stacktrace.c:57
[<ffffffff81a0cdd3>] save_stack+0x43/0xd0 mm/kasan/kasan.c:502
[<ffffffff81a0d70f>] set_track mm/kasan/kasan.c:514 [inline]
[<ffffffff81a0d70f>] kasan_slab_free+0x6f/0xb0 mm/kasan/kasan.c:578
[<ffffffff81a0b5c3>] __cache_free mm/slab.c:3505 [inline]
[<ffffffff81a0b5c3>] kfree+0xd3/0x250 mm/slab.c:3822
[<ffffffff81608502>] __rcu_reclaim kernel/rcu/rcu.h:113 [inline]
[<ffffffff81608502>] rcu_do_batch.isra.70+0x8c2/0xdf0 kernel/rcu/tree.c:2780
[<ffffffff81608ea2>] invoke_rcu_callbacks kernel/rcu/tree.c:3043 [inline]
[<ffffffff81608ea2>] __rcu_process_callbacks kernel/rcu/tree.c:3010 [inline]
[<ffffffff81608ea2>] rcu_process_callbacks+0x472/0xc70 kernel/rcu/tree.c:3027
[<ffffffff841ccfbf>] __do_softirq+0x31f/0xbe7 kernel/softirq.c:284
Memory state around the buggy address:
ffff88006bab1300: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
ffff88006bab1380: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>ffff88006bab1400: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
^
ffff88006bab1480: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
ffff88006bab1500: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
==================================================================
Seems that ctx->might_cancel is racy.
On commit fd694aaa46c7ed811b72eb47d5eb11ce7ab3f7f1.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: timerfd: use-after-free in timerfd_remove_cancel 2017-01-30 18:41 timerfd: use-after-free in timerfd_remove_cancel Dmitry Vyukov @ 2017-01-31 2:06 ` Mateusz Guzik 2017-01-31 8:19 ` Dmitry Vyukov 2017-01-31 11:33 ` Thomas Gleixner 1 sibling, 1 reply; 6+ messages in thread From: Mateusz Guzik @ 2017-01-31 2:06 UTC (permalink / raw) To: Dmitry Vyukov; +Cc: Al Viro, Thomas Gleixner, linux-fsdevel, LKML, syzkaller On Mon, Jan 30, 2017 at 07:41:59PM +0100, Dmitry Vyukov wrote: > Hello, > > The following program triggers use-after-free in timerfd_remove_cancel: > https://gist.githubusercontent.com/dvyukov/202576d437c84ffbbe52e9ccd77e1b44/raw/5562bff8626a73627157331ea2b837f59080ac84/gistfile1.txt > > BUG: KASAN: use-after-free in __list_del include/linux/list.h:104 > [inline] at addr ffff88006bab1410 > BUG: KASAN: use-after-free in __list_del_entry > include/linux/list.h:119 [inline] at addr ffff88006bab1410 > BUG: KASAN: use-after-free in list_del_rcu include/linux/rculist.h:129 > [inline] at addr ffff88006bab1410 > BUG: KASAN: use-after-free in timerfd_remove_cancel fs/timerfd.c:120 > [inline] at addr ffff88006bab1410 > BUG: KASAN: use-after-free in timerfd_release+0x28e/0x310 > fs/timerfd.c:209 at addr ffff88006bab1410 > Write of size 8 by task a.out/2897 > [..] > Seems that ctx->might_cancel is racy. > Indeed it is. Can you try the patch below please. If it works I'll send it in a nicer form. diff --git a/fs/timerfd.c b/fs/timerfd.c index c173cc1..63f91c3 100644 --- a/fs/timerfd.c +++ b/fs/timerfd.c @@ -112,14 +112,30 @@ void timerfd_clock_was_set(void) rcu_read_unlock(); } +static void timerfd_set_cancel(struct timerfd_ctx *ctx) +{ + if (ctx->might_cancel) + return; + + spin_lock(&cancel_lock); + if (!ctx->might_cancel) { + ctx->might_cancel = true; + list_add_rcu(&ctx->clist, &cancel_list); + } + spin_unlock(&cancel_lock); +} + static void timerfd_remove_cancel(struct timerfd_ctx *ctx) { + if (!ctx->might_cancel) + return; + + spin_lock(&cancel_lock); if (ctx->might_cancel) { ctx->might_cancel = false; - spin_lock(&cancel_lock); list_del_rcu(&ctx->clist); - spin_unlock(&cancel_lock); } + spin_unlock(&cancel_lock); } static bool timerfd_canceled(struct timerfd_ctx *ctx) @@ -134,16 +150,10 @@ static void timerfd_setup_cancel(struct timerfd_ctx *ctx, int flags) { if ((ctx->clockid == CLOCK_REALTIME || ctx->clockid == CLOCK_REALTIME_ALARM) && - (flags & TFD_TIMER_ABSTIME) && (flags & TFD_TIMER_CANCEL_ON_SET)) { - if (!ctx->might_cancel) { - ctx->might_cancel = true; - spin_lock(&cancel_lock); - list_add_rcu(&ctx->clist, &cancel_list); - spin_unlock(&cancel_lock); - } - } else if (ctx->might_cancel) { + (flags & TFD_TIMER_ABSTIME) && (flags & TFD_TIMER_CANCEL_ON_SET)) + timerfd_set_cancel(ctx); + else timerfd_remove_cancel(ctx); - } } static ktime_t timerfd_get_remaining(struct timerfd_ctx *ctx) ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: timerfd: use-after-free in timerfd_remove_cancel 2017-01-31 2:06 ` Mateusz Guzik @ 2017-01-31 8:19 ` Dmitry Vyukov 0 siblings, 0 replies; 6+ messages in thread From: Dmitry Vyukov @ 2017-01-31 8:19 UTC (permalink / raw) To: Mateusz Guzik; +Cc: Al Viro, Thomas Gleixner, linux-fsdevel, LKML, syzkaller On Tue, Jan 31, 2017 at 3:06 AM, Mateusz Guzik <mguzik@redhat.com> wrote: > On Mon, Jan 30, 2017 at 07:41:59PM +0100, Dmitry Vyukov wrote: >> Hello, >> >> The following program triggers use-after-free in timerfd_remove_cancel: >> https://gist.githubusercontent.com/dvyukov/202576d437c84ffbbe52e9ccd77e1b44/raw/5562bff8626a73627157331ea2b837f59080ac84/gistfile1.txt >> >> BUG: KASAN: use-after-free in __list_del include/linux/list.h:104 >> [inline] at addr ffff88006bab1410 >> BUG: KASAN: use-after-free in __list_del_entry >> include/linux/list.h:119 [inline] at addr ffff88006bab1410 >> BUG: KASAN: use-after-free in list_del_rcu include/linux/rculist.h:129 >> [inline] at addr ffff88006bab1410 >> BUG: KASAN: use-after-free in timerfd_remove_cancel fs/timerfd.c:120 >> [inline] at addr ffff88006bab1410 >> BUG: KASAN: use-after-free in timerfd_release+0x28e/0x310 >> fs/timerfd.c:209 at addr ffff88006bab1410 >> Write of size 8 by task a.out/2897 >> > [..] >> Seems that ctx->might_cancel is racy. >> > > Indeed it is. Can you try the patch below please. If it works I'll send > it in a nicer form. If the reproducer does not crash kernel (assuming you tested the patch), than there is nothing else I can do to test it. > diff --git a/fs/timerfd.c b/fs/timerfd.c > index c173cc1..63f91c3 100644 > --- a/fs/timerfd.c > +++ b/fs/timerfd.c > @@ -112,14 +112,30 @@ void timerfd_clock_was_set(void) > rcu_read_unlock(); > } > > +static void timerfd_set_cancel(struct timerfd_ctx *ctx) > +{ > + if (ctx->might_cancel) > + return; /\/\/\/\/\/\ But this is not OK. This is a data race. We will get back to you with a data race report soon. If you want to play smart, you need at least READ_ONCE/WRITE_ONCE for variables not protected with locks. However, it looks like this code still has atomicity violation: if I do two calls, one that needs to setup cancel and one that does not, I can end up with an inconsistent outcome -- e.g. timer is setup as if it needs to be in cancel_list but it is not added to the cancel_list; or vice versa -- timer is setup as if it does not need cancel but it is added to the cancel_list. > + spin_lock(&cancel_lock); > + if (!ctx->might_cancel) { > + ctx->might_cancel = true; > + list_add_rcu(&ctx->clist, &cancel_list); > + } > + spin_unlock(&cancel_lock); > +} > + > static void timerfd_remove_cancel(struct timerfd_ctx *ctx) > { > + if (!ctx->might_cancel) > + return; > + > + spin_lock(&cancel_lock); > if (ctx->might_cancel) { > ctx->might_cancel = false; > - spin_lock(&cancel_lock); > list_del_rcu(&ctx->clist); > - spin_unlock(&cancel_lock); > } > + spin_unlock(&cancel_lock); > } > > static bool timerfd_canceled(struct timerfd_ctx *ctx) > @@ -134,16 +150,10 @@ static void timerfd_setup_cancel(struct timerfd_ctx *ctx, int flags) > { > if ((ctx->clockid == CLOCK_REALTIME || > ctx->clockid == CLOCK_REALTIME_ALARM) && > - (flags & TFD_TIMER_ABSTIME) && (flags & TFD_TIMER_CANCEL_ON_SET)) { > - if (!ctx->might_cancel) { > - ctx->might_cancel = true; > - spin_lock(&cancel_lock); > - list_add_rcu(&ctx->clist, &cancel_list); > - spin_unlock(&cancel_lock); > - } > - } else if (ctx->might_cancel) { > + (flags & TFD_TIMER_ABSTIME) && (flags & TFD_TIMER_CANCEL_ON_SET)) > + timerfd_set_cancel(ctx); > + else > timerfd_remove_cancel(ctx); > - } > } > > static ktime_t timerfd_get_remaining(struct timerfd_ctx *ctx) ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: timerfd: use-after-free in timerfd_remove_cancel 2017-01-30 18:41 timerfd: use-after-free in timerfd_remove_cancel Dmitry Vyukov 2017-01-31 2:06 ` Mateusz Guzik @ 2017-01-31 11:33 ` Thomas Gleixner 2017-01-31 11:45 ` Thomas Gleixner 1 sibling, 1 reply; 6+ messages in thread From: Thomas Gleixner @ 2017-01-31 11:33 UTC (permalink / raw) To: Dmitry Vyukov; +Cc: Al Viro, linux-fsdevel, LKML, syzkaller On Mon, 30 Jan 2017, Dmitry Vyukov wrote: > > Seems that ctx->might_cancel is racy. Yes, it is. Fix below. 8<------------------- --- a/fs/timerfd.c +++ b/fs/timerfd.c @@ -40,9 +40,12 @@ struct timerfd_ctx { short unsigned settime_flags; /* to show in fdinfo */ struct rcu_head rcu; struct list_head clist; - bool might_cancel; + unsigned long flags; }; +/* Bit positions for ctx->flags */ +#define MIGHT_CANCEL 0 + static LIST_HEAD(cancel_list); static DEFINE_SPINLOCK(cancel_lock); @@ -99,7 +102,7 @@ void timerfd_clock_was_set(void) rcu_read_lock(); list_for_each_entry_rcu(ctx, &cancel_list, clist) { - if (!ctx->might_cancel) + if (!test_bit(MIGHT_CANCEL, &ctx->flags)) continue; spin_lock_irqsave(&ctx->wqh.lock, flags); if (ctx->moffs != moffs) { @@ -114,8 +117,7 @@ void timerfd_clock_was_set(void) static void timerfd_remove_cancel(struct timerfd_ctx *ctx) { - if (ctx->might_cancel) { - ctx->might_cancel = false; + if (test_and_clear_bit(MIGHT_CANCEL, &ctx->flags)) { spin_lock(&cancel_lock); list_del_rcu(&ctx->clist); spin_unlock(&cancel_lock); @@ -124,7 +126,7 @@ static void timerfd_remove_cancel(struct static bool timerfd_canceled(struct timerfd_ctx *ctx) { - if (!ctx->might_cancel || ctx->moffs != KTIME_MAX) + if (!test_bit(MIGHT_CANCEL, &ctx->flags) || ctx->moffs != KTIME_MAX) return false; ctx->moffs = ktime_mono_to_real(0); return true; @@ -135,13 +137,12 @@ static void timerfd_setup_cancel(struct if ((ctx->clockid == CLOCK_REALTIME || ctx->clockid == CLOCK_REALTIME_ALARM) && (flags & TFD_TIMER_ABSTIME) && (flags & TFD_TIMER_CANCEL_ON_SET)) { - if (!ctx->might_cancel) { - ctx->might_cancel = true; + if (test_and_set_bit(MIGHT_CANCEL, &ctx->flags)) { spin_lock(&cancel_lock); list_add_rcu(&ctx->clist, &cancel_list); spin_unlock(&cancel_lock); } - } else if (ctx->might_cancel) { + } else { timerfd_remove_cancel(ctx); } } ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: timerfd: use-after-free in timerfd_remove_cancel 2017-01-31 11:33 ` Thomas Gleixner @ 2017-01-31 11:45 ` Thomas Gleixner 2017-01-31 12:09 ` Dmitry Vyukov 0 siblings, 1 reply; 6+ messages in thread From: Thomas Gleixner @ 2017-01-31 11:45 UTC (permalink / raw) To: Dmitry Vyukov; +Cc: Al Viro, linux-fsdevel, LKML, syzkaller On Tue, 31 Jan 2017, Thomas Gleixner wrote: > On Mon, 30 Jan 2017, Dmitry Vyukov wrote: > > > > Seems that ctx->might_cancel is racy. > > Yes, it is. Fix below. And the fix is racy as well. Darn, we really need to lock the context to avoid that mess. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: timerfd: use-after-free in timerfd_remove_cancel 2017-01-31 11:45 ` Thomas Gleixner @ 2017-01-31 12:09 ` Dmitry Vyukov 0 siblings, 0 replies; 6+ messages in thread From: Dmitry Vyukov @ 2017-01-31 12:09 UTC (permalink / raw) To: Thomas Gleixner; +Cc: Al Viro, linux-fsdevel, LKML, syzkaller On Tue, Jan 31, 2017 at 12:45 PM, Thomas Gleixner <tglx@linutronix.de> wrote: > On Tue, 31 Jan 2017, Thomas Gleixner wrote: > >> On Mon, 30 Jan 2017, Dmitry Vyukov wrote: >> > >> > Seems that ctx->might_cancel is racy. >> >> Yes, it is. Fix below. > > And the fix is racy as well. Darn, we really need to lock the context to > avoid that mess. Yes. I think we need to lock most of timerfd_settime. Otherwise we can end up with a timer that needs to be in the cancel list, but it is actually not; or vice versa. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-01-31 12:10 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-01-30 18:41 timerfd: use-after-free in timerfd_remove_cancel Dmitry Vyukov 2017-01-31 2:06 ` Mateusz Guzik 2017-01-31 8:19 ` Dmitry Vyukov 2017-01-31 11:33 ` Thomas Gleixner 2017-01-31 11:45 ` Thomas Gleixner 2017-01-31 12:09 ` Dmitry Vyukov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®