| Message ID | 20170131020638.hf2pzz55u4tymvot@dhcp-1-212.brq.redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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) -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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)