Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754887AbaFXSCK (ORCPT ); Tue, 24 Jun 2014 14:02:10 -0400 Received: from mail-oa0-f52.google.com ([209.85.219.52]:61991 "EHLO mail-oa0-f52.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752650AbaFXSCF (ORCPT ); Tue, 24 Jun 2014 14:02:05 -0400 MIME-Version: 1.0 In-Reply-To: <20140624165216.GA29272@redhat.com> References: <1403560693-21809-1-git-send-email-keescook@chromium.org> <1403560693-21809-4-git-send-email-keescook@chromium.org> <20140624165216.GA29272@redhat.com> Date: Tue, 24 Jun 2014 11:02:04 -0700 X-Google-Sender-Auth: VjwTwMiWAlt1LW-P62uIPo62kpo Message-ID: Subject: Re: [PATCH v7 3/9] seccomp: introduce writer locking From: Kees Cook To: Oleg Nesterov Cc: LKML , Andy Lutomirski , Alexei Starovoitov , "Michael Kerrisk (man-pages)" , Andrew Morton , Daniel Borkmann , Will Drewry , Julien Tinnes , David Drysdale , Linux API , "x86@kernel.org" , "linux-arm-kernel@lists.infradead.org" , linux-mips@linux-mips.org, linux-arch , linux-security-module Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Jun 24, 2014 at 9:52 AM, Oleg Nesterov wrote: > Kees, > > I am still trying to force myself to read and try to understand what > this series does ;) Just a minor nit so far. The use-case this solves is when a userspace process does not control (or know) when a thread is spawned (e.g. via shared library init, or LD_PRELOAD) but wants to make sure seccomp filters have been applied to it. Specifically, Chrome, when loading some proprietary graphics drivers, will have those drivers spawning threads before there has been an ability to call seccomp. While some games can be played to get ahead of it, it's not always possible, or racey, depending on the drivers. With seccomp thread-sync, we can be certain that all threads have had the filter applied. > > On 06/23, Kees Cook wrote: >> >> @@ -1142,6 +1168,7 @@ static struct task_struct *copy_process(unsigned long clone_flags, >> { >> int retval; >> struct task_struct *p; >> + unsigned long irqflags; >> >> if ((clone_flags & (CLONE_NEWNS|CLONE_FS)) == (CLONE_NEWNS|CLONE_FS)) >> return ERR_PTR(-EINVAL); >> @@ -1196,7 +1223,6 @@ static struct task_struct *copy_process(unsigned long clone_flags, >> goto fork_out; >> >> ftrace_graph_init_task(p); >> - get_seccomp_filter(p); >> >> rt_mutex_init_task(p); >> >> @@ -1434,7 +1460,13 @@ static struct task_struct *copy_process(unsigned long clone_flags, >> p->parent_exec_id = current->self_exec_id; >> } >> >> - spin_lock(¤t->sighand->siglock); >> + spin_lock_irqsave(¤t->sighand->siglock, irqflags); >> + >> + /* >> + * Copy seccomp details explicitly here, in case they were changed >> + * before holding tasklist_lock. >> + */ >> + copy_seccomp(p); >> >> /* >> * Process group and session signals need to be delivered to just the >> @@ -1446,7 +1478,7 @@ static struct task_struct *copy_process(unsigned long clone_flags, >> */ >> recalc_sigpending(); >> if (signal_pending(current)) { >> - spin_unlock(¤t->sighand->siglock); >> + spin_unlock_irqrestore(¤t->sighand->siglock, irqflags); >> write_unlock_irq(&tasklist_lock); >> retval = -ERESTARTNOINTR; >> goto bad_fork_free_pid; >> @@ -1486,7 +1518,7 @@ static struct task_struct *copy_process(unsigned long clone_flags, >> } >> >> total_forks++; >> - spin_unlock(¤t->sighand->siglock); >> + spin_unlock_irqrestore(¤t->sighand->siglock, irqflags); >> write_unlock_irq(&tasklist_lock); >> proc_fork_connector(p); >> cgroup_post_fork(p); > > It seems that the only change copy_process() needs is copy_seccomp() under the locks. > Everythinh else (irqflags games) looks obviously unneeded? I got irq lock dep warnings without these changes. If they're unneeded, that's totally fine by me, but some change (either this or markings to keep lockdep happy) is needed. >> @@ -524,6 +528,9 @@ static long seccomp_set_mode(unsigned long seccomp_mode, char __user *filter) >> } >> #endif >> >> + if (unlikely(!lock_task_sighand(current, &irqflags))) >> + goto out_free; >> + > > Unless this task is exiting (namely, it has already called exit_notify()), > lock_task_sighand(current) must not fail. Looks like you can simply do > spin_lock_irq(->siglock). Okay. I wasn't sure if I needed to be extra careful here or not. I opted for just using lock_task_sighand since that seemed to be the most used. :) -Kees -- Kees Cook Chrome OS Security -- 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/