linux-kernel - Re: perf: use-after-free in 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]

Date:   Wed, 15 Mar 2017 17:43:02 +0100
From:   Oleg Nesterov <oleg@...hat.com>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     Dmitry Vyukov <dvyukov@...gle.com>, Ingo Molnar <mingo@...hat.com>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        LKML <linux-kernel@...r.kernel.org>,
        Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
        syzkaller <syzkaller@...glegroups.com>
Subject: Re: perf: use-after-free in perf_release

On 03/14, Peter Zijlstra wrote:
>
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -10346,6 +10346,17 @@ void perf_event_free_task(struct task_struct *task)
>  			continue;
>
>  		mutex_lock(&ctx->mutex);
> +		raw_spin_lock_irq(&ctx->lock);
> +		/*
> +		 * Destroy the task <-> ctx relation and mark the context dead.
> +		 *
> +		 * This is important because even though the task hasn't been
> +		 * exposed yet the context has been (through child_list).
> +		 */
> +		RCU_INIT_POINTER(task->perf_event_ctxp[ctxn], NULL);
> +		WRITE_ONCE(ctx->task, TASK_TOMBSTONE);
> +		put_task_struct(task); /* cannot be last */
> +		raw_spin_unlock_irq(&ctx->lock);

Agreed, this is what I had in mind. Although you know, I spent 3
hours looking at your patch and I still can't convince myself I am
really sure it closes all races ;)

OK, I believe this is correct. And iiuc both RCU_INIT_POINTER(NULL)
and put_task_struct() are not strictly necessary? At least until we
add WARN_ON(tsk->usage != 2) before free_task() in copy process().


---------------------------------------------------------------------
This is off-topic, but to me list_for_each_entry(event->child_list)
in perf_event_release_kernel() looks very confusing and misleading.
And list_first_entry_or_null(), we do not really need NULL if list
is empty, tmp == child should be F even if we use list_first_entry().
And given that we already have list_is_last(), it would be nice to
add list_is_first() and cleanup perf_event_release_kernel() a bit:

	--- x/kernel/events/core.c
	+++ x/kernel/events/core.c
	@@ -4152,7 +4152,7 @@ static void put_event(struct perf_event 
	 int perf_event_release_kernel(struct perf_event *event)
	 {
		struct perf_event_context *ctx = event->ctx;
	-	struct perf_event *child, *tmp;
	+	struct perf_event *child;
	 
		/*
		 * If we got here through err_file: fput(event_file); we will not have
	@@ -4190,8 +4190,9 @@ int perf_event_release_kernel(struct per
	 
	 again:
		mutex_lock(&event->child_mutex);
	-	list_for_each_entry(child, &event->child_list, child_list) {
	-
	+	if (!list_empty(&event->child_list)) {
	+		child = list_first_entry(&event->child_list,
	+					 struct perf_event, child_list);
			/*
			 * Cannot change, child events are not migrated, see the
			 * comment with perf_event_ctx_lock_nested().
	@@ -4221,9 +4222,7 @@ again:
			 * state, if child is still the first entry, it didn't get freed
			 * and we can continue doing so.
			 */
	-		tmp = list_first_entry_or_null(&event->child_list,
	-					       struct perf_event, child_list);
	-		if (tmp == child) {
	+		if (list_is_first(child, &event->child_list)) {
				perf_remove_from_context(child, DETACH_GROUP);
				list_del(&child->child_list);
				free_event(child);

But we can't, because

	static inline int list_is_first(const struct list_head *list,
					const struct list_head *head)
	{
		return list->prev == head;
	}

won't work, "child" can be freed so we can't dereference it, and

	static inline int list_is_first(const struct list_head *list,
					const struct list_head *head)
	{
		return head->next == list;
	}

won't be symmetrical with list_is_last() we already have.

Oleg.