forgot to mention...
On 09/30, Oleg Nesterov wrote:
>
> On 09/23, Jann Horn wrote:
> >
> > One reason for doing this is that it prevents an attacker from sending an
> > arbitrary signal to a parent process after performing 2^32-1 execve()
> > calls.
No, sets ->exit_signal = SIGCHLD. So the only problem is that the parent
can do clone(SIGKILL), then do execve() 2^32-1 times, then it can be killed
by SIGKILL from the exiting child.
Honestly, I do not think this is security problem.
> I think we should simply kill self/parent_exec_id's. I am going to send
> the patch below after re-check/testing.
Yes, I think this makes sense anyway.
Oleg.
> --- x/include/linux/sched.h
> +++ x/include/linux/sched.h
> @@ -1677,9 +1677,6 @@ struct task_struct {
> #endif
> struct seccomp seccomp;
>
> -/* Thread group tracking */
> - u32 parent_exec_id;
> - u32 self_exec_id;
> /* Protection of (de-)allocation: mm, files, fs, tty, keyrings, mems_allowed,
> * mempolicy */
> spinlock_t alloc_lock;
> --- x/fs/exec.c
> +++ x/fs/exec.c
> @@ -1163,6 +1163,14 @@ static int de_thread(struct task_struct
> no_thread_group:
> /* we have changed execution domain */
> tsk->exit_signal = SIGCHLD;
> + if (!list_empty(&father->children)) {
> + struct task_struct *child;
> +
> + read_lock(&tasklist_lock);
> + list_for_each_entry(child, &father->children, sibling)
> + child->exit_signal = SIGCHLD;
> + read_unlock(&tasklist_lock);
> + }
>
> exit_itimers(sig);
> flush_itimer_signals();
> @@ -1306,9 +1314,6 @@ void setup_new_exec(struct linux_binprm
> set_dumpable(current->mm, suid_dumpable);
> }
>
> - /* An exec changes our domain. We are no longer part of the thread
> - group */
> - current->self_exec_id++;
> flush_signal_handlers(current, 0);
> do_close_on_exec(current->files);
> }
> --- x/kernel/fork.c
> +++ x/kernel/fork.c
> @@ -1573,13 +1573,10 @@ static struct task_struct *copy_process(
> write_lock_irq(&tasklist_lock);
>
> /* CLONE_PARENT re-uses the old parent */
> - if (clone_flags & (CLONE_PARENT|CLONE_THREAD)) {
> + if (clone_flags & (CLONE_PARENT|CLONE_THREAD))
> p->real_parent = current->real_parent;
> - p->parent_exec_id = current->parent_exec_id;
> - } else {
> + else
> p->real_parent = current;
> - p->parent_exec_id = current->self_exec_id;
> - }
>
> spin_lock(¤t->sighand->siglock);
>
> --- x/kernel/signal.c
> +++ x/kernel/signal.c
> @@ -1585,15 +1585,6 @@ bool do_notify_parent(struct task_struct
> BUG_ON(!tsk->ptrace &&
> (tsk->group_leader != tsk || !thread_group_empty(tsk)));
>
> - if (sig != SIGCHLD) {
> - /*
> - * This is only possible if parent == real_parent.
> - * Check if it has changed security domain.
> - */
> - if (tsk->parent_exec_id != tsk->parent->self_exec_id)
> - sig = SIGCHLD;
> - }
> -
> info.si_signo = sig;
> info.si_errno = 0;
> /*
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
