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]

Date:   Wed, 17 Jan 2018 21:43:39 +0100
From:   Oleg Nesterov <oleg@...hat.com>
To:     "Eric W. Biederman" <ebiederm@...ssion.com>
Cc:     Kirill Tkhai <ktkhai@...tuozzo.com>, gregkh@...uxfoundation.org,
        jslaby@...e.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/3] Revert "do_SAK: Don't recursively take the
 tasklist_lock"

On 01/17, Eric W. Biederman wrote:
>
> Oleg Nesterov <oleg@...hat.com> writes:
>
> >> 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.
> >
> > OK, but how this connects to this change?
> >
> > Again, this force_sig() doesn't match other send_sig()'s in __do_SAK(),
> > and Kirill is going to turn them all into send_sig_info(SEND_SIG_FORCED).
> > Just we need to discuss whether we need to skip the global init or not
> > but this is another story.
> >
> > So why do you dislike this change?
> >
> > force_sig() should die anyway. At least in its current form, it should not
> > be used unless task == current. But this is off-topic.
>
> I see that as a fair criticism of force_sig,
> and a good argument to use send_sig(SIGKILL, SEND_SIG_FORCED).
>
> Which will kill the global init.

and iiuc you think this is right. I won't argue, but again, this needs some
discussion imo.

And in fact Kirill was going to do this before anything else, it was me who
(rightly or not) suggested to do this after cleanups because this is the user
visible change.

> What I don't like is a bunch of patches to introduce races and make
> something more racy

Why? I do not see how this series can add the new problems or races, technically
it looks correct to me.

> So we either need to say do_SAK is broken.  In which case the proper fix
> is to just delete the thing.

I personally never used it so I am fine with your suggestion ;)

> Or we need not to ensure the final
> implemenation is an atomic kill of everything that has the tty open.

Then I think we need some changes in drivers/tty/, with or without Kirill's
changes.

May be some flag set/cleared by __do_SAK() which should make tty_open() fail.
Not sure about tty's passed via unix sockets... and actually I have no idea
how much much paranoia __do_SAK() needs.

Oleg.