lkml.org 
[lkml]   [2017]   [Jan]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: timerfd: use-after-free in timerfd_remove_cancel
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)

\
 
 \ /
  Last update: 2017-01-31 09:20    [W:0.060 / U:0.016 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site