linux-kernel - Re: uprobes/perf: KASAN: use-after-free in uprobe_perf

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20180222170427.GQ25181@hirez.programming.kicks-ass.net>
Date:   Thu, 22 Feb 2018 18:04:27 +0100
From:   Peter Zijlstra <peterz@...radead.org>
To:     Oleg Nesterov <oleg@...hat.com>
Cc:     Prashant Bhole <bhole_prashant_q7@....ntt.co.jp>,
        Ingo Molnar <mingo@...hat.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Jiri Olsa <jolsa@...hat.com>,
        Namhyung Kim <namhyung@...nel.org>,
        linux-kernel@...r.kernel.org
Subject: Re: uprobes/perf: KASAN: use-after-free in uprobe_perf_close

On Thu, Feb 22, 2018 at 05:37:15PM +0100, Oleg Nesterov wrote:
> On 02/22, Prashant Bhole wrote:

> > After debugging, found that uprobe_perf_close() is called after task has
> > been terminated and uprobe_perf_close() tries to access task_struct of the
> > terminated process.
> 
> Oh. You can't imagine how much I forgot this code ;) I will recheck, but at
> first glance you are right. We can't rely on _free_event()->put_ctx() which
> does put_task_struct() after event->destroy(), the exiting task does
> put_task_struct(current) itself and sets child_ctx->task = TASK_TOMBSTONE in
> perf_event_exit_task_context().
> 
> In short, nothing protects event->hw.target. But uprobe_perf_open() should be
> safe, perf_init_event() is called when the caller has the additional reference.
> 
> I am wondering if this was wrong from the very beginning or it was broken later,
> but I won't even try to check.

b2fe8ba674e8 ("uprobes/perf: Avoid uprobe_apply() whenever possible")

Seems to have added that PF_EXITING test that dereferences the target
pointer.

> And. What about other users of event->hw.target? Say, task_bp_pinned(). It doesn't
> dereference this pointer, How can we trust the result of "iter->hw.target == tsk"
> if hw.target can be freed and then re-alloced?

I _think_ that one is ok because it looks at available slots for the
task at init time, at that point the target task must exist.

> This all makes me think that we should change (fix) kernel/events/core.c...

That's going to be mighty dodgy though, holding a reference on the task
will avoid the task from dying which will avoid the events from being
destroyed which will avoid the task from dying which will... if you get
my drift :-)

And I suspect the proposed patch already suffers that problem.
pmu::destroy really should not be looking at that pointer I'm afraid.