Re: [PATCH v2 1/3] Revert "do_SAK: Don't recursively take the tasklist_lock"

[Kirill Tkhai]

On 17.01.2018 20:49, Eric W. Biederman wrote:
> Oleg Nesterov <oleg@xxxxxxxxxx> writes:
> 
>> On 01/17, Eric W. Biederman wrote:
>>
>>> Kirill Tkhai <ktkhai@xxxxxxxxxxxxx> 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.
>>
>> OK, but then we need "force" in other places too. __do_SAK() does send_sig(SIGKILL)
>> in do_each_pid_task(PIDTYPE_SID) and if signal->tty == tty.
>>
>> Plus force_sig() is not rcu-friendly.
>>
>> So I personally agree with this change. Whether we want to kill the global init
>> or not should be discussed, if we want to do this __do_SAK() should use
>> SEND_SIG_FORCED and this is what Kirill is going to do (iiuc), but this needs
>> another patch.
> 
> To operate correctly, do_SAK() needs to kill everything that has the tty
> open.  Unless we can make that guarantee I don't see the point of
> changing do_SAK.

do_SAK() doesn't kill everything at the moment, and it wasn't able to do that
10 years ago, when the reverted commit was introduced. There are two ways
to obtain dead tty:
1)when fd send via unix sockets
2)when a process open foreign /proc/[foreign_pid/fd/fdX

If someone wants to fix that, this possibly should be made outside __do_SAK().
But these races were 10+ years ago, when SAK was implemented, and my patchset
does not add new races to already existing.
 
> It would be better to give up on do_SAK altogether than to keep do_SAK
> limping along and failing to meet it's security guarantees.
> 
> If there are real world races, let's document those and say do_SAK has
> been broken for X number of years and just remove it.  Right now that
> seems the more reasonable course.
> 
> Unless there truly is someone using do_SAK to ensure they have a tty all
> to themselves.
We can remove it, but someone may already use this interface, and this will
break the compatibility.

Anyway, my patchset does not aim to check either SAK is broken or not. My
patchset just make the same functionality as now, but faster.