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: <1efc4393-cf08-e30e-b6c1-bdeada1d8590@virtuozzo.com>
Date:   Thu, 18 Jan 2018 12:59:38 +0300
From:   Kirill Tkhai <ktkhai@...tuozzo.com>
To:     "Eric W. Biederman" <ebiederm@...ssion.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"

On 17.01.2018 20:18, Eric W. Biederman wrote:
> 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.  

As Oleg pointed, this patch does not introduce new races in comparison
to what we have now. Please, look at the discussion we started here:
https://lkml.org/lkml/2018/1/12/171. You wasn't in CC, as there was
not reverted your patch 20ac94378de5. Oleg suggested an accurate way
to revert the commit firstly before doing all the things, because
currently there is an ambiguity with force_sig() and send_sig() in
the single function. My patch introduces uniformity in the function.
> 
>> 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);