Re: [CFT][PATCH 6/7] userns: Add a knob to disable setgroups on a per user namespace basis

Andy Lutomirski [off-list ref] writes:

On Mon, Dec 8, 2014 at 2:11 PM, Eric W. Biederman [off-list ref] wrote:

quoted

- Expose the knob to user space through a proc file /proc/<pid>/setgroups

  A value of 0 means the setgroups system call is disabled in the

"deny"

quoted

  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"

quoted

- Descedent user namespaces inherit the value of setgroups from

s/Descedent/Descendent/

Bah.  I updated everything but the changelog comment.

quoted

--- 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.

Since these are independent atomic variables yes that ordering issue
seems to be the case.

For me it was the natural ordering of the checks so I didn't even bother
to think about what happens when you reorder them.

Eric