Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1759483AbbFBS3b (ORCPT ); Tue, 2 Jun 2015 14:29:31 -0400 Received: from mx1.redhat.com ([209.132.183.28]:47213 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754415AbbFBS3W (ORCPT ); Tue, 2 Jun 2015 14:29:22 -0400 Date: Tue, 2 Jun 2015 20:28:29 +0200 From: Oleg Nesterov To: Tycho Andersen Cc: linux-kernel@vger.kernel.org, linux-api@vger.kernel.org, Kees Cook , Andy Lutomirski , Will Drewry , Roland McGrath , Pavel Emelyanov , "Serge E. Hallyn" Subject: Re: [PATCH] seccomp: add ptrace commands for suspend/resume Message-ID: <20150602182829.GA23449@redhat.com> References: <1433186918-9626-1-git-send-email-tycho.andersen@canonical.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1433186918-9626-1-git-send-email-tycho.andersen@canonical.com> User-Agent: Mutt/1.5.18 (2008-05-17) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2762 Lines: 102 On 06/01, Tycho Andersen wrote: > > --- a/include/linux/seccomp.h > +++ b/include/linux/seccomp.h > @@ -25,6 +25,9 @@ struct seccomp_filter; > struct seccomp { > int mode; > struct seccomp_filter *filter; > +#ifdef CONFIG_CHECKPOINT_RESTORE > + bool suspended; > +#endif Then afaics you need to change copy_seccomp() to clear ->suspended. At least if the child is not traced. > @@ -691,6 +697,11 @@ u32 seccomp_phase1(struct seccomp_data *sd) > int this_syscall = sd ? sd->nr : > syscall_get_nr(current, task_pt_regs(current)); > > +#ifdef CONFIG_CHECKPOINT_RESTORE > + if (unlikely(current->seccomp.suspended)) > + return SECCOMP_PHASE1_OK; > +#endif > + I am wondering if PTRACE_SUSPEND_SECCOMP can just clear/set TIF_SECCOMP. Of course, it is not that resume_seccomp() can simply do set_tsk_thread_flag, it should be more careful. And prctl_set_seccomp() paths will need some changes. Probably not, this would be more complex. So perhaps it would be better to add PTRACE_O_SUSPEND_SECCOMP? This also solves the problem with the killed tracer. Except TIF_NOTSC... But why do we bother to play with TIF_NOTSC, could you explain? > +int suspend_seccomp(struct task_struct *task) > +{ > + int ret = -EACCES; > + > + spin_lock_irq(&task->sighand->siglock); > + > + if (!capable(CAP_SYS_ADMIN)) > + goto out; I am puzzled ;) Why do we need ->siglock? And even if we need it, why we can't check CAP_SYS_ADMIN lockless? And I am not sure I understand why do we need the additional security check, but I leave this to you and Andy. If you have the rights to trace this task, then you can do anything the tracee could do without the filtering. > + > + task->seccomp.suspended = true; > + > +#ifdef TIF_NOTSC > + if (task->seccomp.mode == SECCOMP_MODE_STRICT) > + clear_tsk_thread_flag(task, TIF_NOTSC); > +#endif > + > + ret = 0; > +out: > + spin_unlock_irq(&task->sighand->siglock); > + > + return ret; > +} > + > +int resume_seccomp(struct task_struct *task) > +{ > + int ret = -EACCES; > + > + spin_lock_irq(&task->sighand->siglock); > + > + if (!capable(CAP_SYS_ADMIN)) > + goto out; > + > + task->seccomp.suspended = false; > + > +#ifdef TIF_NOTSC > + if (task->seccomp.mode == SECCOMP_MODE_STRICT) > + set_tsk_thread_flag(task, TIF_NOTSC); > +#endif > + > + ret = 0; > +out: > + spin_unlock_irq(&task->sighand->siglock); > + > + return ret; > +} > +#endif /* CONFIG_CHECKPOINT_RESTORE */ Well, I do not think we need 2 helpers, just one which takes a boolean will look better, imo. Oleg. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/