From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758095AbbEWRyi (ORCPT ); Sat, 23 May 2015 13:54:38 -0400 Received: from out03.mta.xmission.com ([166.70.13.233]:33907 "EHLO out03.mta.xmission.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754116AbbEWRyc (ORCPT ); Sat, 23 May 2015 13:54:32 -0400 From: ebiederm@xmission.com (Eric W. Biederman) To: Lukasz Pawelczyk Cc: "David S. Miller" , "Kirill A. Shutemov" , "Serge E. Hallyn" , Al Viro , Alexey Dobriyan , Andrew Morton , Andy Lutomirski , Casey Schaufler , Christoph Hellwig , David Howells , Eric Dumazet , Fabian Frederick , Greg KH , Ingo Molnar , Ionut Alexa , James Morris , Jeff Layton , Joe Perches , Jonathan Corbet , Kees Cook , Mauro Carvalho Chehab , Michal Hocko , Miklos Szeredi , Nick Kralevich , Oleg Nesterov , Paul Moore , Peter Hurley , Peter Zijlstra , Rik van Riel , Serge Hallyn , Stephen Smalley , Tejun Heo , Zefan Li , Rafal Krypa , linux-doc@vger.kernel.org, linux-api@vger.kernel.org, linux-kernel@vger.kernel.org, linux-security-module@vger.kernel.org, containers@lists.linux-foundation.org, Lukasz Pawelczyk References: <1432209222-8479-1-git-send-email-l.pawelczyk@samsung.com> <1432209222-8479-2-git-send-email-l.pawelczyk@samsung.com> Date: Sat, 23 May 2015 12:49:27 -0500 In-Reply-To: <1432209222-8479-2-git-send-email-l.pawelczyk@samsung.com> (Lukasz Pawelczyk's message of "Thu, 21 May 2015 13:53:35 +0200") Message-ID: <878ucf2nh4.fsf@x220.int.ebiederm.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-XM-AID: U2FsdGVkX1/oR8B48J/ubC57CSVOxJS9M8PTO0TG9iE= X-SA-Exim-Connect-IP: 67.3.205.90 X-SA-Exim-Mail-From: ebiederm@xmission.com X-Spam-Report: * -1.0 ALL_TRUSTED Passed through trusted hosts only via SMTP * 0.0 TVD_RCVD_IP Message was received from an IP address * 0.7 XMSubLong Long Subject * 1.5 XMNoVowels Alpha-numberic number with no vowels * 1.2 LotsOfNums_01 BODY: Lots of long strings of numbers * 0.0 T_TM2_M_HEADER_IN_MSG BODY: No description available. * 0.8 BAYES_50 BODY: Bayes spam probability is 40 to 60% * [score: 0.5000] * -0.0 DCC_CHECK_NEGATIVE Not listed in DCC * [sa05 1397; Body=1 Fuz1=1 Fuz2=1] * 0.0 T_TooManySym_04 7+ unique symbols in subject * 0.0 T_TooManySym_01 4+ unique symbols in subject * 0.0 T_TooManySym_03 6+ unique symbols in subject * 0.0 T_TooManySym_02 5+ unique symbols in subject X-Spam-DCC: XMission; sa05 1397; Body=1 Fuz1=1 Fuz2=1 X-Spam-Combo: ***;Lukasz Pawelczyk X-Spam-Relay-Country: X-Spam-Timing: total 1614 ms - load_scoreonly_sql: 0.06 (0.0%), signal_user_changed: 7 (0.5%), b_tie_ro: 6 (0.4%), parse: 1.87 (0.1%), extract_message_metadata: 18 (1.1%), get_uri_detail_list: 3.1 (0.2%), tests_pri_-1000: 12 (0.7%), tests_pri_-950: 1.25 (0.1%), tests_pri_-900: 1.09 (0.1%), tests_pri_-400: 37 (2.3%), check_bayes: 36 (2.2%), b_tokenize: 15 (0.9%), b_tok_get_all: 10 (0.6%), b_comp_prob: 2.9 (0.2%), b_tok_touch_all: 5.0 (0.3%), b_finish: 0.71 (0.0%), tests_pri_0: 1526 (94.5%), tests_pri_500: 4.9 (0.3%), rewrite_mail: 0.00 (0.0%) Subject: Re: [PATCH 1/8] kernel/exit.c: make sure current's nsproxy != NULL while checking caps X-Spam-Flag: No X-SA-Exim-Version: 4.2.1 (built Wed, 24 Sep 2014 11:00:52 -0600) X-SA-Exim-Scanned: Yes (on in01.mta.xmission.com) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Lukasz Pawelczyk writes: > There is a rare case where current's nsproxy might be NULL but we are > required to check for credentials and capabilities. It sometimes happens > during an exit_group() syscall while destroying user's session (logging > out). > > My understanding is that while we have to lock the task to get task's > nsproxy and check whether it's NULL, for the 'current' we don't have to > and it's expected not to be NULL. There is a code in the kernel > currently that does current->nsproxy->user_ns without any checks. > And include/linux/nsproxy.h confirms that: > > 2. when accessing (i.e. reading) current task's namespaces - no > precautions should be taken - just dereference the pointers > > There seem to be no crash currently because of this, but with accessing > nsproxy from LSM hooks there is. This is the backtrace: > > 0 smk_tskacc (task=0xffff88003b0b92e0, obj_known=0x2 , mode=2, a=0xffff88003be53dd8) at security/smack/smack_access.c:261 > 1 0xffffffff8130e2aa in smk_curacc (obj_known=, mode=, a=) at security/smack/smack_access.c:318 > 2 0xffffffff8130a50d in smack_task_kill (p=0xffff88003b0b92e0, info=, sig=, secid=) at security/smack/smack_lsm.c:2071 > 3 0xffffffff812ea4f6 in security_task_kill (p=, info=, sig=, secid=) at security/security.c:952 > 4 0xffffffff8109ac80 in check_kill_permission (sig=15, info=0x0 , t=0xffff88003b0b8000) at kernel/signal.c:796 > 5 0xffffffff8109d3ab in group_send_sig_info (sig=15, info=0x0 , p=0xffff88003b0b8000) at kernel/signal.c:1296 > 6 0xffffffff8108e527 in forget_original_parent (father=) at kernel/exit.c:575 > 7 exit_notify (group_dead=, tsk=) at kernel/exit.c:606 > 8 do_exit (code=) at kernel/exit.c:775 > 9 0xffffffff8108ec0f in do_group_exit (exit_code=0) at kernel/exit.c:891 > 10 0xffffffff8108ec84 in SYSC_exit_group (error_code=) at kernel/exit.c:902 > 11 SyS_exit_group (error_code=) at kernel/exit.c:900 > > This backtrace clearly shows that there is an LSM hook task_kill() that > happens during an exit_group() syscall and that this happens after > exit_task_namespaces(). LSM hooks with namespaces might need nsproxy to > be able to check for capabilities. At this point this is impossible. The > current's nsproxy is already NULL/destroyed. > > This is the case because exit_task_namespaces() is called before the > exit_notify() where all of the above happens. This patch changes their > order. Nacked-by: "Eric W. Biederman" current->nsproxy->user_ns does not exist, and changing where exit_task_namespaces is fragile and I am really not interested in messing with it right now, to solve a problem that does not exist. > > Signed-off-by: Lukasz Pawelczyk > --- > kernel/exit.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/kernel/exit.c b/kernel/exit.c > index 22fcc05..da1bb18 100644 > --- a/kernel/exit.c > +++ b/kernel/exit.c > @@ -742,7 +742,6 @@ void do_exit(long code) > exit_fs(tsk); > if (group_dead) > disassociate_ctty(1); > - exit_task_namespaces(tsk); > exit_task_work(tsk); > exit_thread(); > > @@ -763,6 +762,13 @@ void do_exit(long code) > > TASKS_RCU(tasks_rcu_i = __srcu_read_lock(&tasks_rcu_exit_srcu)); > exit_notify(tsk, group_dead); > + > + /* > + * This should be after all things that potentially require > + * process's namespaces (e.g. capability checks). > + */ > + exit_task_namespaces(tsk); > + > proc_exit_connector(tsk); > #ifdef CONFIG_NUMA > task_lock(tsk);