Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20180625163419.216578-1-jannh@google.com> <20180626130757.GA10042@gmail.com>
In-Reply-To: <20180626130757.GA10042@gmail.com>
From:   Jann Horn <jannh@google.com>
Date:   Tue, 26 Jun 2018 16:06:45 +0200
Message-ID: <CAG48ez2WwQVWtfh7sQCqn1E52V7V4_LxZaU+kV_r-YGOzNAsXg@mail.gmail.com>
Subject: Re: [PATCH] userns: move user access out of the mutex
To:     christian.brauner@canonical.com
Cc:     "Eric W. Biederman" <ebiederm@xmission.com>,
        kernel list <linux-kernel@vger.kernel.org>,
        security@kernel.org, Andy Lutomirski <luto@amacapital.net>,
        Kees Cook <keescook@chromium.org>,
        "Serge E. Hallyn" <serge@hallyn.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Jun 26, 2018 at 3:08 PM Christian Brauner
<christian.brauner@canonical.com> wrote:
>
> On Mon, Jun 25, 2018 at 06:34:19PM +0200, Jann Horn wrote:
> > The old code would hold the userns_state_mutex indefinitely if
> > memdup_user_nul stalled due to e.g. a userfault region. Prevent that by
> > moving the memdup_user_nul in front of the mutex_lock().
> >
> > Note: This changes the error precedence of invalid buf/count/*ppos vs
> > map already written / capabilities missing.
> >
> > Fixes: 22d917d80e84 ("userns: Rework the user_namespace adding uid/gid...")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Jann Horn <jannh@google.com>
> > ---
> >  kernel/user_namespace.c | 24 ++++++++++--------------
> >  1 file changed, 10 insertions(+), 14 deletions(-)
> >
> > diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
> > index c3d7583fcd21..e5222b5fb4fe 100644
> > --- a/kernel/user_namespace.c
> > +++ b/kernel/user_namespace.c
> > @@ -859,7 +859,16 @@ static ssize_t map_write(struct file *file, const char __user *buf,
> >       unsigned idx;
> >       struct uid_gid_extent extent;
> >       char *kbuf = NULL, *pos, *next_line;
> > -     ssize_t ret = -EINVAL;
> > +     ssize_t ret;
> > +
> > +     /* Only allow < page size writes at the beginning of the file */
> > +     if ((*ppos != 0) || (count >= PAGE_SIZE))
> > +             return -EINVAL;
> > +
> > +     /* Slurp in the user data */
> > +     kbuf = memdup_user_nul(buf, count);
> > +     if (IS_ERR(kbuf))
> > +             return PTR_ERR(kbuf);
>
> I'm not opposed to this but what is annoying is the changed error
> reporting you pointed out. It seems conceptually way cleaner if missing
> permissions are reported before more specific internal errors.
>
> The question I have is how bad it would be if memdup_user_nul() stalled
> and if you see any issues security wise. Given that the mutex is only
> taken on operations that are mostly performed when creating or setting
> up a new user namespace
>
> map_write()
> create_user_ns()
> proc_setgroups_write()
> userns_may_setgroups()
>
> and not when actually interacting with it it seems the worst that
> happens is that creation of new user namespaces is not possible anymore.
> That shouldn't have any effect on the host though which I would see as a
> real issue. But I might be missing context. :)

userns_may_setgroups() is involved in sys_setgroups() via
may_setgroups(), so if one thread is blocking the userns_state_mutex,
nobody can log in anymore.

> >       /*
> >        * The userns_state_mutex serializes all writes to any given map.
> > @@ -895,19 +904,6 @@ static ssize_t map_write(struct file *file, const char __user *buf,
> >       if (cap_valid(cap_setid) && !file_ns_capable(file, ns, CAP_SYS_ADMIN))
> >               goto out;
> >
> > -     /* Only allow < page size writes at the beginning of the file */
> > -     ret = -EINVAL;
> > -     if ((*ppos != 0) || (count >= PAGE_SIZE))
> > -             goto out;
> > -
> > -     /* Slurp in the user data */
> > -     kbuf = memdup_user_nul(buf, count);
> > -     if (IS_ERR(kbuf)) {
> > -             ret = PTR_ERR(kbuf);
> > -             kbuf = NULL;
> > -             goto out;
> > -     }
> > -
> >       /* Parse the user data */
> >       ret = -EINVAL;
> >       pos = kbuf;
> > --
> > 2.18.0.rc2.346.g013aa6912e-goog
> >