Re: [PATCH] user_ns: use correct check for single-threadedness

[Eric W. Biederman]

Kees Cook <keescook@xxxxxxxxxxxx> writes:

> On Tue, Jul 28, 2015 at 1:55 PM, Ricky Zhou <rickyz@xxxxxxxxxx> wrote:
>> On Tue, Jul 28, 2015 at 11:17 AM, Eric W. Biederman
>> <ebiederm@xxxxxxxxxxxx> wrote:
>>> Kees Cook <keescook@xxxxxxxxxxxx> writes:
>>>
>>>> From: Ricky Zhou <rickyz@xxxxxxxxxxxx>
>>>>
>>>> Checking mm_users > 1 does not mean a process is multithreaded. For
>>>> example, reading /proc/PID/maps temporarily increments mm_users, allowing
>>>> other processes to (accidentally) interfere with unshare() calls.
>>>>
>>>> This fixes observed failures of unshare(CLONE_NEWUSER) incorrectly
>>>> returning EINVAL if another processes happened to be simultaneously
>>>> reading the maps file.
>>>>
>>>> Signed-off-by: Ricky Zhou <rickyz@xxxxxxxxxxxx>
>>>> Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx>
>>>> Cc: stable@xxxxxxxxxxxxxxx
>>>
>>> This looks like a good fix.  Any chance you can drudge up the commit where
>>> this hack came in so that Greg & Company know how far to back port this?
>>
>> userns_install in user_namespace.c (affects setns of a user
>> namespace): cde1975bc242f3e1072bde623ef378e547b73f91.
>>
>> The check in check_unshare_flags is a little more complex. The
>> incorrect check was added in
>> cf2e340f4249b781b3d2beb41e891d08581f0e10 but I don't think it would
>> have triggered under any supported combination of flags at that point.
>>
>> From 50804fe3737ca6a5942fdc2057a18a8141d00141 until
>> 6e556ce209b09528dbf1931cbfd5d323e1345926, the bug affected
>> unshare(CLONE_NEWPID).
>
> That's back to v3.8, so this goes quite a way, it seems.

This patch was marked as CC' stable.  The question I am asking is this
problem bad enough that backporting this change to stable makes sense?

And even more if this patch can wait for the merge window to be merged
or if there this needs to be expidited, and get in before 4.2 -final.

It sounds like this is one of those rare bugs someone hit once or twice,
just often enough for this to be tracked down, and the important thing
is that this fix get into the kernel's code base.

So unless I am otherwise informed I will assume that this is a change
that can stand to wait until the merge window to be merged (so it has a
full development cycle of review and discussion).  But that is worth
backporting after that because it actually causes problems that people
actually hit.

Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/