From mboxrd@z Thu Jan 1 00:00:00 1970 From: Oleg Nesterov Subject: Re: [PATCH v9 11/11] seccomp: implement SECCOMP_FILTER_FLAG_TSYNC Date: Wed, 9 Jul 2014 20:05:20 +0200 Message-ID: <20140709180520.GA2560@redhat.com> References: <1403911380-27787-1-git-send-email-keescook@chromium.org> <1403911380-27787-12-git-send-email-keescook@chromium.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: <1403911380-27787-12-git-send-email-keescook@chromium.org> Sender: linux-mips-bounce@linux-mips.org Errors-to: linux-mips-bounce@linux-mips.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-subscribe: List-owner: List-post: List-archive: To: Kees Cook Cc: linux-kernel@vger.kernel.org, Andy Lutomirski , "Michael Kerrisk (man-pages)" , Alexei Starovoitov , Andrew Morton , Daniel Borkmann , Will Drewry , Julien Tinnes , David Drysdale , linux-api@vger.kernel.org, x86@kernel.org, linux-arm-kernel@lists.infradead.org, linux-mips@linux-mips.org, linux-arch@vger.kernel.org, linux-security-module@vger.kernel.org List-Id: linux-arch.vger.kernel.org First of all, sorry for delay ;) So far I quickly glanced at this series and everything look fine, but I am confused by the signal_group_exit() check, On 06/27, Kees Cook wrote: > > To make sure that de_thread() is actually able > to kill other threads during an exec, any sighand holders need to check > if they've been scheduled to be killed, and to give up on their work. Probably this connects to that check below? I can't understand this... > + /* > + * Make sure we cannot change seccomp or nnp state via TSYNC > + * while another thread is in the middle of calling exec. > + */ > + if (flags & SECCOMP_FILTER_FLAG_TSYNC && > + mutex_lock_killable(¤t->signal->cred_guard_mutex)) > + goto out_free; -EINVAL looks a bit confusing in this case, but this is cosemtic because userspace won't see this error-code anyway. > spin_lock_irq(¤t->sighand->siglock); > + if (unlikely(signal_group_exit(current->signal))) { > + /* If thread is dying, return to process the signal. */ OK, this doesn't hurt, but why? You could check __fatal_signal_pending() with the same effect. And since we hold this mutex, exec (de_thread) can be the source of that SIGKILL. We take this mutex specially to avoid the race with exec. So why do we need to abort if we race with kill() or exit_grouo() ? Oleg. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com ([209.132.183.28]:16586 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754666AbaGISH3 (ORCPT ); Wed, 9 Jul 2014 14:07:29 -0400 Date: Wed, 9 Jul 2014 20:05:20 +0200 From: Oleg Nesterov Subject: Re: [PATCH v9 11/11] seccomp: implement SECCOMP_FILTER_FLAG_TSYNC Message-ID: <20140709180520.GA2560@redhat.com> References: <1403911380-27787-1-git-send-email-keescook@chromium.org> <1403911380-27787-12-git-send-email-keescook@chromium.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1403911380-27787-12-git-send-email-keescook@chromium.org> Sender: linux-arch-owner@vger.kernel.org List-ID: To: Kees Cook Cc: linux-kernel@vger.kernel.org, Andy Lutomirski , "Michael Kerrisk (man-pages)" , Alexei Starovoitov , Andrew Morton , Daniel Borkmann , Will Drewry , Julien Tinnes , David Drysdale , linux-api@vger.kernel.org, x86@kernel.org, linux-arm-kernel@lists.infradead.org, linux-mips@linux-mips.org, linux-arch@vger.kernel.org, linux-security-module@vger.kernel.org Message-ID: <20140709180520.VYh5d12U0oMVPmI3e1mO56XAsSGWoM3FBa4PeoXT2Hw@z> First of all, sorry for delay ;) So far I quickly glanced at this series and everything look fine, but I am confused by the signal_group_exit() check, On 06/27, Kees Cook wrote: > > To make sure that de_thread() is actually able > to kill other threads during an exec, any sighand holders need to check > if they've been scheduled to be killed, and to give up on their work. Probably this connects to that check below? I can't understand this... > + /* > + * Make sure we cannot change seccomp or nnp state via TSYNC > + * while another thread is in the middle of calling exec. > + */ > + if (flags & SECCOMP_FILTER_FLAG_TSYNC && > + mutex_lock_killable(¤t->signal->cred_guard_mutex)) > + goto out_free; -EINVAL looks a bit confusing in this case, but this is cosemtic because userspace won't see this error-code anyway. > spin_lock_irq(¤t->sighand->siglock); > + if (unlikely(signal_group_exit(current->signal))) { > + /* If thread is dying, return to process the signal. */ OK, this doesn't hurt, but why? You could check __fatal_signal_pending() with the same effect. And since we hold this mutex, exec (de_thread) can be the source of that SIGKILL. We take this mutex specially to avoid the race with exec. So why do we need to abort if we race with kill() or exit_grouo() ? Oleg.