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

[ebiederm@xmission.com (Eric W. Biederman)]

Andy Lutomirski <luto@amacapital.net> writes:

> On Mon, Dec 8, 2014 at 2:11 PM, Eric W. Biederman <ebiederm@xmission.com> wrote:
>>
>> - 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"
>
>>   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.

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