Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933902AbbFJRVJ (ORCPT ); Wed, 10 Jun 2015 13:21:09 -0400 Received: from mail-lb0-f179.google.com ([209.85.217.179]:32935 "EHLO mail-lb0-f179.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753250AbbFJRVA (ORCPT ); Wed, 10 Jun 2015 13:21:00 -0400 MIME-Version: 1.0 In-Reply-To: <20150610163149.GA5092@redhat.com> References: <1433897388-9567-1-git-send-email-tycho.andersen@canonical.com> <20150610163149.GA5092@redhat.com> From: Andy Lutomirski Date: Wed, 10 Jun 2015 10:20:38 -0700 Message-ID: Subject: Re: [PATCH v4] seccomp: add ptrace options for suspend/resume To: Oleg Nesterov Cc: Tycho Andersen , "linux-kernel@vger.kernel.org" , Linux API , Kees Cook , Will Drewry , Roland McGrath , Pavel Emelyanov , "Serge E. Hallyn" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3762 Lines: 96 On Wed, Jun 10, 2015 at 9:31 AM, Oleg Nesterov wrote: > On 06/09, Andy Lutomirski wrote: >> >> On Tue, Jun 9, 2015 at 5:49 PM, Tycho Andersen >> > >> > @@ -556,6 +556,15 @@ static int ptrace_setoptions(struct task_struct *child, unsigned long data) >> > if (data & ~(unsigned long)PTRACE_O_MASK) >> > return -EINVAL; >> > >> > + if (unlikely(data & PTRACE_O_SUSPEND_SECCOMP)) { > > Well, we should do this if > > (data & O_SUSPEND) && !(flags & O_SUSPEND) > > or at least if > > (data ^ flags) & O_SUSPEND > > >> > + if (!config_enabled(CONFIG_CHECKPOINT_RESTORE) || >> > + !config_enabled(CONFIG_SECCOMP)) >> > + return -EINVAL; >> > + >> > + if (!capable(CAP_SYS_ADMIN)) >> > + return -EPERM; >> >> I tend to think that we should also require that current not be using >> seccomp. Otherwise, in principle, there's a seccomp bypass for >> privileged-but-seccomped programs. > > Andy, I simply can't understand why do we need any security check at all. > > OK, yes, in theory we can have a seccomped CAP_SYS_ADMIN process, seccomp > doesn't filter ptrace, you hack that process and force it to attach to > another CAP_SYS_ADMIN/seccomped process, etc, etc... Looks too paranoid > to me. I've sometimes considered having privileged processes I write fork and seccomp their child. Of course, if you're allowing ptrace through your seccomp filter, you open a giant can of worms, but I think we should take the more paranoid approach to start and relax it later as needed. After all, for the intended use of this patch, stuff will break regardless of what we do if the ptracer is itself seccomped. I could be convinced that if the ptracer is outside seccomp then we shouldn't need the CAP_SYS_ADMIN check. That would at least make this work in a user namespace. >> > @@ -590,6 +590,10 @@ void secure_computing_strict(int this_syscall) >> > { >> > int mode = current->seccomp.mode; >> > >> > + if (config_enabled(CONFIG_CHECKPOINT_RESTORE) && >> > + unlikely(current->ptrace & PT_SUSPEND_SECCOMP)) >> > + return; >> > + >> > if (mode == 0) >> > return; >> > else if (mode == SECCOMP_MODE_STRICT) >> > @@ -691,6 +695,10 @@ u32 seccomp_phase1(struct seccomp_data *sd) >> > int this_syscall = sd ? sd->nr : >> > syscall_get_nr(current, task_pt_regs(current)); >> > >> > + if (config_enabled(CONFIG_CHECKPOINT_RESTORE) && >> > + unlikely(current->ptrace & PT_SUSPEND_SECCOMP)) >> > + return SECCOMP_PHASE1_OK; >> > + >> >> If it's not hard, it might still be nice to try to fold this into >> mode. This code is rather hot. If it would be a mess, then don't >> worry about it for now. > > IMO, this would be a mess ;) At least compared to this simple patch. > > Suppose we add SECCOMP_MODE_SUSPENDED. Not only this adds the problems > with detach if the tracer dies. > > We need to change copy_seccomp(). And it is not clear what should we > do if the child is traced too. > > We need to change prctl_set_seccomp() paths. > > And even the "tracee->seccomp.mode = SECCOMP_MODE_SUSPENDED" code needs > some locking even if the tracee is stopped, we need to avoid the races > with SECCOMP_FILTER_FLAG_TSYNC from other threads. > Agreed. Let's hold off until this becomes a problem (if it ever does). --Andy -- 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/