From mboxrd@z Thu Jan 1 00:00:00 1970 From: ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org (Eric W. Biederman) Subject: Re: [CFT][PATCH 6/7] userns: Add a knob to disable setgroups on a per user namespace basis Date: Mon, 08 Dec 2014 17:30:07 -0600 Message-ID: <87a92xn2io.fsf@x220.int.ebiederm.org> References: <52e0643bd47b1e5c65921d6e00aea1f724bb510a.1417281801.git.luto@amacapital.net> <87h9xez20g.fsf@x220.int.ebiederm.org> <87mw75ygwp.fsf@x220.int.ebiederm.org> <87fvcxyf28.fsf_-_@x220.int.ebiederm.org> <874mtdyexp.fsf_-_@x220.int.ebiederm.org> <87a935u3nj.fsf@x220.int.ebiederm.org> <87388xodlj.fsf@x220.int.ebiederm.org> <87h9x5re41.fsf_-_@x220.int.ebiederm.org> <87mw6xpzb0.fsf_-_@x220.int.ebiederm.org> <87ppbtn4mv.fsf@x220.int.ebiederm.org> Mime-Version: 1.0 Content-Type: text/plain Return-path: In-Reply-To: (Andy Lutomirski's message of "Mon, 8 Dec 2014 14:48:54 -0800") Sender: linux-api-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Andy Lutomirski Cc: Linux Containers , Josh Triplett , Andrew Morton , Kees Cook , Michael Kerrisk-manpages , Linux API , linux-man , "linux-kernel@vger.kernel.org" , LSM , Casey Schaufler , "Serge E. Hallyn" , Richard Weinberger , Kenton Varda , stable List-Id: linux-api@vger.kernel.org Andy Lutomirski writes: > On Mon, Dec 8, 2014 at 2:44 PM, Eric W. Biederman wrote: >> Andy Lutomirski writes: >> >>> On Mon, Dec 8, 2014 at 2:11 PM, Eric W. Biederman wrote: >>>> >>>> - Expose the knob to user space through a proc file /proc//setgroups >>>> >>>> A value of 0 means the setgroups system call is disabled in the >>> >>> "deny" >>> >>>> current processes user namespace and can not be enabled in the >>>> future in this user namespace. >>>> >>>> A value of 1 means the segtoups system call is enabled. >>>> >>> >>> "allow" >>> >>>> - Descedent user namespaces inherit the value of setgroups from >>> >>> s/Descedent/Descendent/ >> >> Bah. I updated everything but the changelog comment. >> >>>> --- a/kernel/groups.c >>>> +++ b/kernel/groups.c >>>> @@ -222,6 +222,7 @@ bool may_setgroups(void) >>>> * the user namespace has been established. >>>> */ >>>> return userns_gid_mappings_established(user_ns) && >>>> + userns_setgroups_allowed(user_ns) && >>>> ns_capable(user_ns, CAP_SETGID); >>>> } >>> >>> Can you add a comment explaining the ordering? For example: >> >> I need to think on what I can say to make it clear. >> Perhaps: /* Careful the order of these checks is important. */ >> >>> We need to check for a gid mapping before checking setgroups_allowed >>> because an unprivileged user can create a userns with setgroups >>> allowed, then disallow setgroups and add a mapping. If we check in >>> the opposite order, then we have a race: we could see that setgroups >>> is allowed before the user clears the bit and then see that there is a >>> gid mapping after the other thread is done. >> > > This text was actually my suggested comment text. Now I see. > If you put smp_rmb() in this function with a comment like that, then I > think it will all make sense and be obviously correct (even with most > of the other barriers removed). Right. Given that we have to be careful when using these things anyway what I was hoping to achieve with the barriers appears impossible, and confusing so I will see about just adding barriers where we need them for real. Sigh. Eric