[PATCH] userns: move user access out of the mutex

[PATCH] userns: move user access out of the mutex

[Jann Horn]

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);
 
 	/*
 	 * 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

Re: [PATCH] userns: move user access out of the mutex

[Christian Brauner]

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

Christian

>  
>  	/*
>  	 * 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
> 

Re: [PATCH] userns: move user access out of the mutex

[Jann Horn]

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

Re: [PATCH] userns: move user access out of the mutex

[Christian Brauner]

On Tue, Jun 26, 2018 at 04:06:45PM +0200, Jann Horn wrote:
> 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>

Acked-by: Christian Brauner <christian@brauner.io>

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

Thanks!

> 
> > >       /*
> > >        * 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
> > >

Re: [PATCH] userns: move user access out of the mutex

[Serge E. Hallyn]

Quoting Christian Brauner (christian.brauner@canonical.com):
> On Tue, Jun 26, 2018 at 04:06:45PM +0200, Jann Horn wrote:
> > 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>
> 
> Acked-by: Christian Brauner <christian@brauner.io>

Acked-by: Serge Hallyn <serge@hallyn.com>

thanks.

-serge