Re: [PATCH -mm 5/7] add user namespace

[Cedric Le Goater]

Kirill Korotaev wrote:
> I wonder about another namespace coupling I found thinking
> about your patches.
> 
> Lets take a look at sys_setpriority() or any other function calling
> find_user():
> it can change the priority for all user or group processes like:
> 
> do_each_thread_ve(g, p) {
>    if (p->uid == who)
>        error = set_one_prio(p, niceval, error);
> } while_each_thread_ve(g, p);

eh. this is openvz code ! thanks :)

> which essentially means that user-namespace becomes coupled with
> process-namespace. Sure, we can check in every such place for
>  p->nsproxy->user_ns == current->nsproxy->user_ns
> condition. But this a way IMHO leading to kernel full of security
> crap which is hardly maintainable.

only 4 syscalls use find_user() : sys_setpriority, sys_getpriority,
sys_ioprio_set, sys_ioprio_get and they use it very simply to check if a
user_struct exists for a given uid. So, it should be OK. But please see the
attached patch.

> Another example of not so evident coupling here:
> user structure maintains number of processes/opened
> files/sigpending/locked_shm etc.
> if a single user can belong to different proccess/ipc/... namespaces
> all these becomes unusable.

this is the purpose of execns.

user namespace can't be unshared through the unshare syscall(). they can
only be unshared through execns() which flushes the previous image of the
process. However, the execns patch still needs to close files without the
close-on-exec flag. I didn't do it yet. lazy me :)

> Small patch comment:
> - what is the reason in adding 2nd arg to find_user()?

The main reason is alloc_uid() in clone_user_ns() and find_user() inherited
the same prototype maybe abusively. Here's a patch.

thanks,

C.

From: Cedric Le Goater <clg@xxxxxxxxxx>
Subject: remove user namespace arg from find_user

This patch removes the user namespace argument from find_user().

find_user() is always called from the current context, hence the user
namespace can be determined with current->nsproxy->user_ns in
find_user().

Signed-off-by: Cedric Le Goater <clg@xxxxxxxxxx>
Cc: Andrew Morton <akpm@xxxxxxxx>
Cc: Kirill Korotaev <dev@xxxxxxxxxx>
Cc: Andrey Savochkin <saw@xxxxx>
Cc: Eric W. Biederman <ebiederm@xxxxxxxxxxxx>
Cc: Herbert Poetzl <herbert@xxxxxxxxxxxx>
Cc: Sam Vilain <sam.vilain@xxxxxxxxxxxxxxx>
Cc: Serge E. Hallyn <serue@xxxxxxxxxx>
Cc: Dave Hansen <haveblue@xxxxxxxxxx>

---
 fs/ioprio.c           |    4 ++--
 include/linux/sched.h |    2 +-
 kernel/sys.c          |    4 ++--
 kernel/user.c         |    3 ++-
 4 files changed, 7 insertions(+), 6 deletions(-)

Index: 2.6.18-rc1-mm1/fs/ioprio.c
===================================================================
--- 2.6.18-rc1-mm1.orig/fs/ioprio.c
+++ 2.6.18-rc1-mm1/fs/ioprio.c
@@ -102,7 +102,7 @@ asmlinkage long sys_ioprio_set(int which
 			if (!who)
 				user = current->user;
 			else
-				user = find_user(current->nsproxy->user_ns, who);
+				user = find_user(who);

 			if (!user)
 				break;
@@ -172,7 +172,7 @@ asmlinkage long sys_ioprio_get(int which
 			if (!who)
 				user = current->user;
 			else
-				user = find_user(current->nsproxy->user_ns, who);
+				user = find_user(who);

 			if (!user)
 				break;
Index: 2.6.18-rc1-mm1/include/linux/sched.h
===================================================================
--- 2.6.18-rc1-mm1.orig/include/linux/sched.h
+++ 2.6.18-rc1-mm1/include/linux/sched.h
@@ -539,7 +539,7 @@ struct user_struct {
 	uid_t uid;
 };

-extern struct user_struct *find_user(struct user_namespace *, uid_t);
+extern struct user_struct *find_user(uid_t);

 extern struct user_struct root_user;
 #define INIT_USER (&root_user)
Index: 2.6.18-rc1-mm1/kernel/sys.c
===================================================================
--- 2.6.18-rc1-mm1.orig/kernel/sys.c
+++ 2.6.18-rc1-mm1/kernel/sys.c
@@ -545,7 +545,7 @@ asmlinkage long sys_setpriority(int whic
 				who = current->uid;
 			else
 				if ((who != current->uid) &&
-					!(user = find_user(current->nsproxy->user_ns, who)))
+					!(user = find_user(who)))
 					goto out_unlock;	/* No processes for this user */

 			do_each_thread(g, p)
@@ -604,7 +604,7 @@ asmlinkage long sys_getpriority(int whic
 				who = current->uid;
 			else
 				if ((who != current->uid) &&
-					!(user = find_user(current->nsproxy->user_ns, who)))
+					!(user = find_user(who)))
 					goto out_unlock;	/* No processes for this user */

 			do_each_thread(g, p)
Index: 2.6.18-rc1-mm1/kernel/user.c
===================================================================
--- 2.6.18-rc1-mm1.orig/kernel/user.c
+++ 2.6.18-rc1-mm1/kernel/user.c
@@ -205,10 +205,11 @@ void free_user_ns(struct kref *kref)
  *
  * If the user_struct could not be found, return NULL.
  */
-struct user_struct *find_user(struct user_namespace *ns, uid_t uid)
+struct user_struct *find_user(uid_t uid)
 {
 	struct user_struct *ret;
 	unsigned long flags;
+	struct user_namespace *ns = current->nsproxy->user_ns;

 	spin_lock_irqsave(&uidhash_lock, flags);
 	ret = uid_hash_find(uid, uidhashentry(ns, uid));

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