linux-kernel - [PATCH -mm] utrace: fix utrace_maybe_reap() vs find_matching

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>] [day] [month] [year] [list]

Date:	Thu, 21 Jan 2010 17:39:21 +0100
From:	Oleg Nesterov <oleg@...hat.com>
To:	Andrew Morton <akpm@...ux-foundation.org>
Cc:	Alexey Dobriyan <adobriyan@...il.com>,
	Andi Kleen <andi@...stfloor.org>,
	Ananth Mavinakayanahalli <ananth@...ibm.com>,
	Christoph Hellwig <hch@...radead.org>,
	"Frank Ch. Eigler" <fche@...hat.com>, Ingo Molnar <mingo@...e.hu>,
	Peter Zijlstra <peterz@...radead.org>,
	Roland McGrath <roland@...hat.com>,
	linux-kernel@...r.kernel.org, utrace-devel@...hat.com,
	CAI Qian <caiqian@...hat.com>
Subject: [PATCH -mm] utrace: fix utrace_maybe_reap() vs
	find_matching_engine() race

(on top of utrace-core.patch)

The comment in utrace_maybe_reap() correctly explains why
utrace_attach_task/utrace_control/etc can't modify or use
attaching/attached lists.  But find_matching_engine() can
scan ->attached under utrace->lock without any checks, it
can race with utrace_maybe_reap() destroying list nodes.

Change utrace_maybe_reap() to empty ->attached before it
drops utrace->lock, update the comments a bit.

Reported-by: CAI Qian <caiqian@...hat.com>
Signed-off-by: Oleg Nesterov <oleg@...hat.com>
Signed-off-by: Roland McGrath <roland@...hat.com>
---
 kernel/utrace.c |   23 ++++++++++++++++-------
 1 files changed, 16 insertions(+), 7 deletions(-)

--- V1/kernel/utrace.c~8_REAP_FIND_RACE	2009-12-18 01:58:37.000000000 +0100
+++ V1/kernel/utrace.c	2010-01-21 17:31:18.000000000 +0100
@@ -1,7 +1,7 @@
 /*
  * utrace infrastructure interface for debugging user processes
  *
- * Copyright (C) 2006-2009 Red Hat, Inc.  All rights reserved.
+ * Copyright (C) 2006-2010 Red Hat, Inc.  All rights reserved.
  *
  * This copyrighted material is made available to anyone wishing to use,
  * modify, copy, or redistribute it subject to the terms and conditions
@@ -859,6 +859,7 @@ void utrace_maybe_reap(struct task_struc
 		       bool reap)
 {
 	struct utrace_engine *engine, *next;
+	struct list_head attached;
 
 	spin_lock(&utrace->lock);
 
@@ -897,16 +898,24 @@ void utrace_maybe_reap(struct task_struc
 	}
 
 	/*
-	 * utrace_add_engine() checks ->utrace_flags != 0.
-	 * Since @utrace->reap is set, nobody can set or clear
-	 * UTRACE_EVENT(REAP) in @engine->flags or change
-	 * @engine->ops, and nobody can change @utrace->attached.
+	 * utrace_add_engine() checks ->utrace_flags != 0.  Since
+	 * @utrace->reap is set, nobody can set or clear UTRACE_EVENT(REAP)
+	 * in @engine->flags or change @engine->ops and nobody can change
+	 * @utrace->attached after we drop the lock.
 	 */
 	target->utrace_flags = 0;
-	splice_attaching(utrace);
+
+	/*
+	 * We clear out @utrace->attached before we drop the lock so
+	 * that find_matching_engine() can't come across any old engine
+	 * while we are busy tearing it down.
+	 */
+	list_replace_init(&utrace->attached, &attached);
+	list_splice_tail_init(&utrace->attaching, &attached);
+
 	spin_unlock(&utrace->lock);
 
-	list_for_each_entry_safe(engine, next, &utrace->attached, entry) {
+	list_for_each_entry_safe(engine, next, &attached, entry) {
 		if (engine->flags & UTRACE_EVENT(REAP))
 			engine->ops->report_reap(engine, target);
 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/