linux-kernel - Re: [RFC][PATCH v2 5/5] signal: Don't allow accessing signal

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: <20170405161812.GD14536@redhat.com>
Date:   Wed, 5 Apr 2017 18:18:12 +0200
From:   Oleg Nesterov <oleg@...hat.com>
To:     "Eric W. Biederman" <ebiederm@...ssion.com>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        Aleksa Sarai <asarai@...e.com>,
        Andy Lutomirski <luto@...capital.net>,
        Attila Fazekas <afazekas@...hat.com>,
        Jann Horn <jann@...jh.net>, Kees Cook <keescook@...omium.org>,
        Michal Hocko <mhocko@...nel.org>,
        Ulrich Obergfell <uobergfe@...hat.com>,
        linux-kernel@...r.kernel.org, linux-api@...r.kernel.org
Subject: Re: [RFC][PATCH v2 5/5] signal: Don't allow accessing
        signal_struct by old threads after exec

On 04/02, Eric W. Biederman wrote:
>
> Add exec_id to signal_struct and compare it at a few choice moments.

I really dislike this change no matter what, sorry.

Firstly, task_struct->*_exec_id should simply die (I already have the
patch), or at least they should be moved into signal_struct simply
because this is per-process thing.

> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -995,6 +995,10 @@ static int __send_signal(int sig, struct siginfo *info, struct task_struct *t,
>  			from_ancestor_ns || (info == SEND_SIG_FORCED)))
>  		goto ret;
>
> +	/* Don't allow thread group signals after exec */
> +	if (group && (t->signal->exec_id != t->self_exec_id))
> +		goto ret;

Hmm. Either we do not need this exec_id check at all, or we should not
take "group" into account; a fatal signal (say SIGKILL) will kill the
whole thread-group.

> @@ -1247,7 +1251,8 @@ struct sighand_struct *__lock_task_sighand(struct task_struct *tsk,
>  		 * must see ->sighand == NULL.
>  		 */
>  		spin_lock(&sighand->siglock);
> -		if (likely(sighand == tsk->sighand)) {
> +		if (likely((sighand == tsk->sighand) &&
> +			   (tsk->self_exec_id == tsk->signal->exec_id))) {

Oh, this doesn't look good to me. Yes, with your approach we probably need
this to, say, ensure that posix-cpu-timer can't kill the process after exec,
but I'd rather add the exit_state check into run_posix_timers().

But OK, suppose that we fix the problems with signal-after-exec.

====================================================================
Now lets fix another problem. A mt exec suceeds and apllication does
sys_seccomp(SECCOMP_FILTER_FLAG_TSYNC) which fails because it finds
another (zombie) SECCOMP_MODE_FILTER thread.

And after we fix this problem, what else we will need to fix?

I really think that - whatever we do - there should be no other threads
after exec, even zombies.

Oleg.