From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752173AbbEYLd1 (ORCPT ); Mon, 25 May 2015 07:33:27 -0400 Received: from mailout1.w1.samsung.com ([210.118.77.11]:34007 "EHLO mailout1.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751821AbbEYLdY (ORCPT ); Mon, 25 May 2015 07:33:24 -0400 X-AuditID: cbfec7f5-f794b6d000001495-f2-55630880ceb2 Message-id: <1432553598.1781.4.camel@samsung.com> Subject: Re: [PATCH 1/8] kernel/exit.c: make sure current's nsproxy != NULL while checking caps From: Lukasz Pawelczyk To: "Eric W. Biederman" 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 Date: Mon, 25 May 2015 13:33:18 +0200 In-reply-to: <878ucf2nh4.fsf@x220.int.ebiederm.org> References: <1432209222-8479-1-git-send-email-l.pawelczyk@samsung.com> <1432209222-8479-2-git-send-email-l.pawelczyk@samsung.com> <878ucf2nh4.fsf@x220.int.ebiederm.org> Content-type: text/plain; charset=UTF-8 X-Mailer: Evolution 3.12.11 (3.12.11-1.fc21) MIME-version: 1.0 Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA02Sa0iTYRTHefY+77vX1epxWj50QRhUJGlpUaeIKIJ46UNYFFRfbOmbRk5l c6OiD1uamTkpzdJpF7PytlhuGenwmm7dSFNRZ6lhVnSxC2Fe1rTWCPz2O+f/O+d8OTyjqGEX 8UeTUkVNkipRycnws2lXV7iBj923xvhqOVwx7oRiq4WDwQdTHFjufORgpOksguL2dAxfT3sw zDxIl8I757AUsjyTHKSVWjmY+bwKKqraJPC0sJqFnLd7wDIxhKBo6C2G5+fVYLlYwoB9PJOD koxbGLrqijn4nj3MQW6mSQpVdw0sVL0/Dp/Sxll476hmoGr6KYKBS9cZqGk8g8DZOY3AZWqS wM+eKSmM5pYz4O22YnjkKGXghfEDhvaOF1KYKmtD0O51sVtXCkNfvFgwG0ycUGR4iYX7FW6J UGsekAo3bDqhx3FASG8dZYXy31ZOsJeHCbbKc5zwuMCDhcarFqmQ11eGBIfbwAmlOXlsdOhB 2eY4MfGoXtSs3nJIltBV6GFSfi07bn7tQAZ0NTQLBfCUrKMTVifr54W0Y9DKZSEZryC3Ec2v rcP+4iei9oxK7LPkJJL2FnT+tXg+iKho2WPe1+bIGjreUc/4OJispu5vuf8WMaR/DvXWfJD6 AkyW0e6GJomPA8ha+tJ8BflYQSyItk/O9TFDVtDcazcZ335KVlG3O85/NpBO5A1ivxJK7ZZR 5gIi5lkT5lmaeZZ2AzGVaIGoi03RHo5XR0VoVWqtLik+IjZZbUP+Zxp7iG47N7UgwiPlXPn+ hMP7FKxKrz2hbkGUZ5TB8sjff1vyONWJk6ImOUajSxS1LWgxj5Uh8sK6b3sVJF6VKh4TxRRR 8z+V8AGLDKjB+mbjTP1ANJtmNL6Oba3YEVJtJ29GQhNJ8JfOpR6bVz98ybSkOX/T2GDgvWan DB+6v82m/9yhmx/UY3KHk1PZR4qLMi7vPrO+d3uO7FRtQUw+Znp/rAgMSY4Oco3e0/clZTeO KDbcjXkyOd5XsivK3vhuXla/+06LNyI9bNKlxNoEVWQYo9Gq/gCHQSn9SAMAAA== Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On sob, 2015-05-23 at 12:49 -0500, Eric W. Biederman wrote: > 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. I must have missed the moment where current->nsproxy->user_ns was removed. I obviously even don't use it in my patches anymore (replaced with cred->user_ns). Back when I started to write my patches and wanted to use current->nsproxy->user_ns in LSM hooks the problem was real. Fortunately current->cred->user_ns does not exhibit the same issue. I'll drop this patch. Sorry for the confusion. > > > > > 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); > -- > To unsubscribe from this list: send the line "unsubscribe linux-security-module" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Lukasz Pawelczyk Samsung R&D Institute Poland Samsung Electronics