Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1201049 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] user_ns: use correct check for single-threadedness ebiederm@xmission.com (Eric W. Biederman) - 2015-08-05 20:30 +0200
    Re: [PATCH] user_ns: use correct check for single-threadedness Kees Cook <keescook@chromium.org> - 2015-08-05 21:50 +0200

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

Kees Cook <keescook@chromium.org> writes:

> On Tue, Jul 28, 2015 at 1:55 PM, Ricky Zhou <rickyz@google.com> wrote:
>> On Tue, Jul 28, 2015 at 11:17 AM, Eric W. Biederman
>> <ebiederm@xmission.com> wrote:
>>> Kees Cook <keescook@chromium.org> writes:
>>>
>>>> From: Ricky Zhou <rickyz@chromium.org>
>>>>
>>>> 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@chromium.org>
>>>> Signed-off-by: Kees Cook <keescook@chromium.org>
>>>> Cc: stable@vger.kernel.org
>>>
>>> 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1201082

On Wed, Aug 5, 2015 at 11:13 AM, Eric W. Biederman
<ebiederm@xmission.com> wrote:
> Kees Cook <keescook@chromium.org> writes:
>
>> On Tue, Jul 28, 2015 at 1:55 PM, Ricky Zhou <rickyz@google.com> wrote:
>>> On Tue, Jul 28, 2015 at 11:17 AM, Eric W. Biederman
>>> <ebiederm@xmission.com> wrote:
>>>> Kees Cook <keescook@chromium.org> writes:
>>>>
>>>>> From: Ricky Zhou <rickyz@chromium.org>
>>>>>
>>>>> 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@chromium.org>
>>>>> Signed-off-by: Kees Cook <keescook@chromium.org>
>>>>> Cc: stable@vger.kernel.org
>>>>
>>>> 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?

I have no problem dropping the CC. At the time it seemed like a clear
bug fix appropriate for stable. If you feel differently, please remove
the CC. :)

-Kees

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web