timerfd: use-after-free in timerfd_remove_cancel

[Dmitry Vyukov]

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.

[Mateusz Guzik]

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)

[Dmitry Vyukov]

On Tue, Jan 31, 2017 at 3:06 AM, Mateusz Guzik <mgu...@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.

[Thomas Gleixner]

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);
}
}

[Thomas Gleixner]

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.

[Dmitry Vyukov]

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.