* 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 related [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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.