linux-kernel - Re: [PATCH v2 1/3] Revert "do_SAK: Don't recursively take the tasklist

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: <87shb4floe.fsf@xmission.com>
Date:   Wed, 17 Jan 2018 11:18:57 -0600
From:   ebiederm@...ssion.com (Eric W. Biederman)
To:     Kirill Tkhai <ktkhai@...tuozzo.com>
Cc:     gregkh@...uxfoundation.org, jslaby@...e.com, oleg@...hat.com,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/3] Revert "do_SAK: Don't recursively take the tasklist_lock"

Kirill Tkhai <ktkhai@...tuozzo.com> writes:

> This reverts commit 20ac94378de5.
>
> send_sig() does not take tasklist_lock for a long time,
> so this commit and the problem it solves are not relevant
> anymore.
>
> Also, the problem of force_sig() is it clears SIGNAL_UNKILLABLE
> flag, thus even global init may be killed by __do_SAK(),
> which is definitely not the expected behavior.

Actually it is.

SAK should kill everything that has the tty open.  If init opens the tty
I am so sorry, it can not operate correctly.  init should not have your
tty open.

The alternative and perhaps the better option is to simply remove SAK
support, if we can not make it race free.

But yes SAK very much does care about races.  

> Came from discussion in "tty: Iterate only thread group leaders in __do_SAK()"
> https://lkml.org/lkml/2018/1/11/492
>
> Suggested-by: Oleg Nesterov <oleg@...hat.com>
> Signed-off-by: Kirill Tkhai <ktkhai@...tuozzo.com>
> ---
>  drivers/tty/tty_io.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> index dc60aeea87d8..84715ba1aee2 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -2737,7 +2737,7 @@ void __do_SAK(struct tty_struct *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);
> +			send_sig(SIGKILL, p, 1);
>  		}
>  		task_unlock(p);
>  	} while_each_thread(g, p);