linux-kernel - Re: [PATCH 3/4] tty: Iterate only thread group leaders in __do

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: <20180112164234.GA21532@redhat.com>
Date:   Fri, 12 Jan 2018 17:42:34 +0100
From:   Oleg Nesterov <oleg@...hat.com>
To:     Kirill Tkhai <ktkhai@...tuozzo.com>
Cc:     linux-kernel@...r.kernel.org, gregkh@...uxfoundation.org,
        jslaby@...e.com, viro@...iv.linux.org.uk, keescook@...omium.org,
        serge@...lyn.com, james.l.morris@...cle.com, luto@...nel.org,
        john.johansen@...onical.com, mingo@...nel.org,
        akpm@...ux-foundation.org, mhocko@...e.com, peterz@...radead.org
Subject: Re: [PATCH 3/4] tty: Iterate only thread group leaders in __do_SAK()

On 01/12, Kirill Tkhai wrote:
>
> How about this patch instead of the whole set? I left thread iterations
> and added sighand locking for visability.

Kirill, I didn't really read this series so I don't quite understand what
are you actually trying to do...

__do_SAK() is racy anyway, a process can open tty right after it was checked,
and I do not understand why should we care about races with execve.

IOW, I do not understand why we can't simply use rcu_read_lock() after
do_each_pid_task/while_each_pid_task. Yes we can miss the new process/thread,
but if the creator process had this tty opened it should be killed by us so
fork/clone can't succeed: both do_fork() and send_sig() take the same lock
and do_fork() checks signal_pending() under ->siglock.

No?

And whatever we do, I think you are right and for_each_process() makes more
sense, and in the likely case all sub-threads should share the same file_struct.
So perhaps we should start with the simple cleanup? Say,

	for_each_process(p) {
		if (p->signal->tty == tty) {
			tty_notice(tty, "SAK: killed process %d (%s): by controlling tty\n",
				   task_pid_nr(p), p->comm);
			goto kill;
		}

		files = NULL;
		for_each_thread(p, t) {
				if (t->files == files) /* racy but we do not care */
					continue;

				task_lock(t);
				files = t->files;
				i = iterate_fd(files, 0, this_tty, tty);
				task_unlock(t);

				if (i != 0) {
					tty_notice(tty, "SAK: killed process %d (%s): by fd#%d\n",
						   task_pid_nr(p), p->comm, i - 1);
					goto kill;
				}
		}

		continue;
kill:
		force_sig(SIGKILL, p);
	}

(see the uncompiled/untested patch below), then make another change to avoid
tasklist_lock?

Oleg.


--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -2704,7 +2704,8 @@ void __do_SAK(struct tty_struct *tty)
 #ifdef TTY_SOFT_SAK
 	tty_hangup(tty);
 #else
-	struct task_struct *g, *p;
+	struct task_struct *p, *t;
+	struct files_struct files;
 	struct pid *session;
 	int		i;
 
@@ -2725,22 +2726,34 @@ void __do_SAK(struct tty_struct *tty)
 	} while_each_pid_task(session, PIDTYPE_SID, p);
 
 	/* Now kill any processes that happen to have the tty open */
-	do_each_thread(g, p) {
+	for_each_process(p) {
 		if (p->signal->tty == tty) {
 			tty_notice(tty, "SAK: killed process %d (%s): by controlling tty\n",
 				   task_pid_nr(p), p->comm);
-			send_sig(SIGKILL, p, 1);
-			continue;
+			goto kill;
 		}
-		task_lock(p);
-		i = iterate_fd(p->files, 0, this_tty, tty);
-		if (i != 0) {
-			tty_notice(tty, "SAK: killed process %d (%s): by fd#%d\n",
-				   task_pid_nr(p), p->comm, i - 1);
-			force_sig(SIGKILL, p);
+
+		files = NULL;
+		for_each_thread(p, t) {
+				if (t->files == files) /* racy but we do not care */
+					continue;
+
+				task_lock(t);
+				files = t->files;
+				i = iterate_fd(files, 0, this_tty, tty);
+				task_unlock(t);
+
+				if (i != 0) {
+					tty_notice(tty, "SAK: killed process %d (%s): by fd#%d\n",
+						   task_pid_nr(p), p->comm, i - 1);
+					goto kill;
+				}
 		}
-		task_unlock(p);
-	} while_each_thread(g, p);
+
+		continue;
+kill:
+		force_sig(SIGKILL, p);
+	}
 	read_unlock(&tasklist_lock);
 #endif
 }