[Devel] Re: [PATCH 1/9] Add a user_namespace as creator/owner of uts_namespace

2011-02-23 Thread David Howells 
Serge E. Hallyn se...@hallyn.com wrote:

  struct uts_namespace {
   struct kref kref;
   struct new_utsname name;
 + struct user_namespace *user_ns;
  };

If a uts_namespace belongs to a user_namespace, should CLONE_NEWUSER then
imply CLONE_NEWUTS?

Or is uts_namespace::user_ns more an implication that the set of users in
user_namespace are the only ones authorised to alter the uts data.

I presume that the uts_namespace of a process must be owned by one of the
user_namespaces in the alternating inheritance chain of namespaces and their
creators leading from current_user_ns() to init_user_ns.

With that in mind, looking at patch 3:

-   if (!capable(CAP_SYS_ADMIN))
+   if (!ns_capable(current-nsproxy-uts_ns-user_ns, CAP_SYS_ADMIN))

what is it you're actually asking?  I presume it's 'does this user have
CAP_SYS_ADMIN capability over objects belonging to the uts_namespace's
user_namespace?'

So, to look at the important bit of patch 2:

-int cap_capable(struct task_struct *tsk, const struct cred *cred, int cap,
-   int audit)
+int cap_capable(struct task_struct *tsk, const struct cred *cred,
+   struct user_namespace *targ_ns, int cap, int audit)
 {
-   return cap_raised(cred-cap_effective, cap) ? 0 : -EPERM;
+   for (;;) {
+   /* The creator of the user namespace has all caps. */
+   if (targ_ns != init_user_ns  targ_ns-creator == cred-user)
+   return 0;
+
+   /* Do we have the necessary capabilities? */
+   if (targ_ns == cred-user-user_ns)
+   return cap_raised(cred-cap_effective, cap) ? 0 : 
-EPERM;
+
+   /* Have we tried all of the parent namespaces? */
+   if (targ_ns == init_user_ns)
+   return -EPERM;
+
+   /* If you have the capability in a parent user ns you have it
+* in the over all children user namespaces as well, so see
+* if this process has the capability in the parent user
+* namespace.
+*/
+   targ_ns = targ_ns-creator-user_ns;
+   }
+
+   /* We never get here */
+   return -EPERM;
 }

On entry, as we're called from ns_capable(), cred-user is the user that the
current process is running as, and, as such, may be in a separate namespace
from uts_namespace - which may itself be in a separate namespace from
init_user_ns.

So, assume for the sake of argument that there are three user_namespaces along
the chain from the calling process to the root, and that the uts_namespace
belongs to the middle one.

if (targ_ns != init_user_ns  targ_ns-creator == cred-user)
return 0;

Can never match because targ_ns-creator cannot be cred-user; even if the
uts_namespace belongs to our namespace, given that the creator lies outside
our namespace.

if (targ_ns == cred-user-user_ns)
return cap_raised(cred-cap_effective, cap) ? 0 : 
-EPERM;

Can only match if we are in the target user_namespace (ie. the one to which
uts_namespace belongs), whether or not we have CAP_SYS_ADMIN.

Which means that unless the uts_namespace belongs to our user_namespace, we
cannot change it.  Is that correct?

So ns_capable() restricts you to only doing interesting things to objects that
belong to a user_namespace if they are in your own user_namespace.  Is that
correct?

If that is so, is the loop required for ns_capable()?


Looking further at patch 2:

#define nsown_capable(cap) (ns_capable(current_user_ns(), (cap)))

Given what I've said above, I presume the loop isn't necessary here either.


I think you're using ns_capable() in two different ways:

 (1) You're using it to see if a process has power over its descendents in a
 user_namespace that can be traced back to a clone() that it did with
 CLONE_NEWUSER.

 For example, automatically granting a process permission to kill
 descendents in a namespace it created.

 (2) You're using it to see if a process can access objects that might be
 outside its own user_namespace.

 For example, setting the hostname.

Is it worth giving two different interfaces to make this clearer (even if they
actually do the same thing)?


Sorry if this seems rambly, but I'm trying to get my head round your code.

David
 
___
Containers mailing list
contain...@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

___
Devel mailing list
Devel@openvz.org
https://openvz.org/mailman/listinfo/devel

[Devel] Re: [PATCH 1/9] Add a user_namespace as creator/owner of uts_namespace

2011-02-23 Thread Eric W. Biederman 
David Howells dhowe...@redhat.com writes:

 Serge E. Hallyn se...@hallyn.com wrote:

  struct uts_namespace {
  struct kref kref;
  struct new_utsname name;
 +struct user_namespace *user_ns;
  };

 If a uts_namespace belongs to a user_namespace, should CLONE_NEWUSER then
 imply CLONE_NEWUTS?

 Or is uts_namespace::user_ns more an implication that the set of users in
 user_namespace are the only ones authorised to alter the uts data.

The later.

 I presume that the uts_namespace of a process must be owned by one of the
 user_namespaces in the alternating inheritance chain of namespaces and their
 creators leading from current_user_ns() to init_user_ns.

 With that in mind, looking at patch 3:

 - if (!capable(CAP_SYS_ADMIN))
 + if (!ns_capable(current-nsproxy-uts_ns-user_ns, CAP_SYS_ADMIN))

 what is it you're actually asking?  I presume it's 'does this user have
 CAP_SYS_ADMIN capability over objects belonging to the uts_namespace's
 user_namespace?'

Yes.

 So, to look at the important bit of patch 2:

 -int cap_capable(struct task_struct *tsk, const struct cred *cred, int cap,
 - int audit)
 +int cap_capable(struct task_struct *tsk, const struct cred *cred,
 + struct user_namespace *targ_ns, int cap, int audit)
  {
 - return cap_raised(cred-cap_effective, cap) ? 0 : -EPERM;
 + for (;;) {
 + /* The creator of the user namespace has all caps. */
 + if (targ_ns != init_user_ns  targ_ns-creator == cred-user)
 + return 0;
 +
 + /* Do we have the necessary capabilities? */
 + if (targ_ns == cred-user-user_ns)
 + return cap_raised(cred-cap_effective, cap) ? 0 : 
 -EPERM;
 +
 + /* Have we tried all of the parent namespaces? */
 + if (targ_ns == init_user_ns)
 + return -EPERM;
 +
 + /* If you have the capability in a parent user ns you have it
 +  * in the over all children user namespaces as well, so see
 +  * if this process has the capability in the parent user
 +  * namespace.
 +  */
 + targ_ns = targ_ns-creator-user_ns;
 + }
 +
 + /* We never get here */
 + return -EPERM;
  }

 On entry, as we're called from ns_capable(), cred-user is the user that the
 current process is running as, and, as such, may be in a separate namespace
 from uts_namespace - which may itself be in a separate namespace from
 init_user_ns.

 So, assume for the sake of argument that there are three user_namespaces along
 the chain from the calling process to the root, and that the uts_namespace
 belongs to the middle one.

So we have the nested stack of:
user_ns3-creator-user_ns == user_ns2
user_ns2-creator-user_ns == init_user_ns
uts_ns2-user_ns == user_ns2


   if (targ_ns != init_user_ns  targ_ns-creator == cred-user)
   return 0;

 Can never match because targ_ns-creator cannot be cred-user; even if the
 uts_namespace belongs to our namespace, given that the creator lies outside
 our namespace.

Initially we come in with targ_ns == user_ns2 and cred-user-user_ns in
one of (user_ns3, user_ns2, or init_user_ns).

targ_ns takes on values user_ns2 and init_user_ns.

So when targ_ns becomes init_user_ns.  If the user in question is
uts_ns2-user_ns-creator.  This check will indeed match.

   if (targ_ns == cred-user-user_ns)
   return cap_raised(cred-cap_effective, cap) ? 0 : 
 -EPERM;

 Can only match if we are in the target user_namespace (ie. the one to which
 uts_namespace belongs), whether or not we have CAP_SYS_ADMIN.

As before targ_ns takes on values of user_ns2 and init_user_ns.

Which means this check will match if we have CAP_SYS_ADMIN in
init_user_ns or in user_ns2.

 Which means that unless the uts_namespace belongs to our user_namespace, we
 cannot change it.  Is that correct?

No.  If you are root in a parent namespace you can also change it.

 So ns_capable() restricts you to only doing interesting things to objects that
 belong to a user_namespace if they are in your own user_namespace.  Is that
 correct?

No.  Root outside your user namespace is also allowed to do interesting
things.

 If that is so, is the loop required for ns_capable()?

Yes.


 Looking further at patch 2:

   #define nsown_capable(cap) (ns_capable(current_user_ns(), (cap)))

 Given what I've said above, I presume the loop isn't necessary here either.


 I think you're using ns_capable() in two different ways:

  (1) You're using it to see if a process has power over its descendents in a
  user_namespace that can be traced back to a clone() that it did with
  CLONE_NEWUSER.

  For example, automatically granting a process permission to kill
  descendents in a namespace it created.

  (2) You're using it to see if a process can access objects that might be
  outside its own user_namespace.

  For

[Devel] Re: [PATCH 1/9] Add a user_namespace as creator/owner of uts_namespace

2011-02-23 Thread David Howells 
Eric W. Biederman ebied...@xmission.com wrote:

  Which means that unless the uts_namespace belongs to our user_namespace, we
  cannot change it.  Is that correct?
 
 No.  If you are root in a parent namespace you can also change it.

But surely, by definition, if you're a user in this namespace, you can't also
be root in a parent namespace...

For the case I worked through current_user() is a member of current_user_ns()
and can't also be a member of its parent, grandparent, etc. - or can it?

David
___
Containers mailing list
contain...@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

___
Devel mailing list
Devel@openvz.org
https://openvz.org/mailman/listinfo/devel

[Devel] Re: [PATCH 1/9] Add a user_namespace as creator/owner of uts_namespace

2011-02-18 Thread Daniel Lezcano 
On 02/17/2011 04:02 PM, Serge E. Hallyn wrote:
 copy_process() handles CLONE_NEWUSER before the rest of the
 namespaces.  So in the case of clone(CLONE_NEWUSER|CLONE_NEWUTS)
 the new uts namespace will have the new user namespace as its
 owner.  That is what we want, since we want root in that new
 userns to be able to have privilege over it.

 Changelog:
   Feb 15: don't set uts_ns-user_ns if we didn't create
   a new uts_ns.

 Signed-off-by: Serge E. Hallynserge.hal...@canonical.com

Acked-by: Daniel Lezcano daniel.lezc...@free.fr

A couple of comments.

 ---
   include/linux/utsname.h |3 +++
   init/version.c  |2 ++
   kernel/nsproxy.c|5 +
   kernel/user.c   |8 ++--
   kernel/utsname.c|4 
   5 files changed, 20 insertions(+), 2 deletions(-)

 diff --git a/include/linux/utsname.h b/include/linux/utsname.h
 index 69f3997..85171be 100644
 --- a/include/linux/utsname.h
 +++ b/include/linux/utsname.h
 @@ -37,9 +37,12 @@ struct new_utsname {
   #includelinux/nsproxy.h
   #includelinux/err.h

 +struct user_namespace;
 +
   struct uts_namespace {
   struct kref kref;
   struct new_utsname name;
 + struct user_namespace *user_ns;
   };
   extern struct uts_namespace init_uts_ns;

 diff --git a/init/version.c b/init/version.c
 index adff586..97bb86f 100644
 --- a/init/version.c
 +++ b/init/version.c
 @@ -21,6 +21,7 @@ extern int version_string(LINUX_VERSION_CODE);
   int version_string(LINUX_VERSION_CODE);
   #endif

 +extern struct user_namespace init_user_ns;
   struct uts_namespace init_uts_ns = {
   .kref = {
   .refcount   = ATOMIC_INIT(2),
 @@ -33,6 +34,7 @@ struct uts_namespace init_uts_ns = {
   .machine= UTS_MACHINE,
   .domainname = UTS_DOMAINNAME,
   },
 + .user_ns =init_user_ns,
   };
   EXPORT_SYMBOL_GPL(init_uts_ns);

 diff --git a/kernel/nsproxy.c b/kernel/nsproxy.c
 index f74e6c0..034dc2e 100644
 --- a/kernel/nsproxy.c
 +++ b/kernel/nsproxy.c
 @@ -74,6 +74,11 @@ static struct nsproxy *create_new_namespaces(unsigned long 
 flags,
   err = PTR_ERR(new_nsp-uts_ns);
   goto out_uts;
   }
 + if (new_nsp-uts_ns != tsk-nsproxy-uts_ns) {
 + put_user_ns(new_nsp-uts_ns-user_ns);
 + new_nsp-uts_ns-user_ns = task_cred_xxx(tsk, user)-user_ns;
 + get_user_ns(new_nsp-uts_ns-user_ns);
 + }

IMO you should add a comment telling this code assume create_user_ns was 
called before (via copy_cred).


   new_nsp-ipc_ns = copy_ipcs(flags, tsk-nsproxy-ipc_ns);
   if (IS_ERR(new_nsp-ipc_ns)) {

[ ... ]

   static struct uts_namespace *create_uts_ns(void)
   {
 @@ -40,6 +41,8 @@ static struct uts_namespace *clone_uts_ns(struct 
 uts_namespace *old_ns)

   down_read(uts_sem);
   memcpy(ns-name,old_ns-name, sizeof(ns-name));
 + ns-user_ns = old_ns-user_ns;
 + get_user_ns(ns-user_ns);

ns-user_ns = get_user_ns(old_ns-user_ns);

___
Containers mailing list
contain...@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

___
Devel mailing list
Devel@openvz.org
https://openvz.org/mailman/listinfo/devel

[Devel] Re: [PATCH 1/9] Add a user_namespace as creator/owner of uts_namespace

2011-02-18 Thread Andrew Morton 
On Thu, 17 Feb 2011 15:02:57 +
Serge E. Hallyn se...@hallyn.com wrote:

 +/*
 + * userns count is 1 for root user, 1 for init_uts_ns,
 + * and 1 for... ?
 + */

?
___
Containers mailing list
contain...@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

___
Devel mailing list
Devel@openvz.org
https://openvz.org/mailman/listinfo/devel

[Devel] Re: [PATCH 1/9] Add a user_namespace as creator/owner of uts_namespace

2011-02-17 Thread Eric W. Biederman 
Serge E. Hallyn se...@hallyn.com writes:

 copy_process() handles CLONE_NEWUSER before the rest of the
 namespaces.  So in the case of clone(CLONE_NEWUSER|CLONE_NEWUTS)
 the new uts namespace will have the new user namespace as its
 owner.  That is what we want, since we want root in that new
 userns to be able to have privilege over it.

Acked-by: Eric W. Biederman ebied...@xmission.com


 Changelog:
   Feb 15: don't set uts_ns-user_ns if we didn't create
   a new uts_ns.

 Signed-off-by: Serge E. Hallyn serge.hal...@canonical.com
 ---
  include/linux/utsname.h |3 +++
  init/version.c  |2 ++
  kernel/nsproxy.c|5 +
  kernel/user.c   |8 ++--
  kernel/utsname.c|4 
  5 files changed, 20 insertions(+), 2 deletions(-)

 diff --git a/include/linux/utsname.h b/include/linux/utsname.h
 index 69f3997..85171be 100644
 --- a/include/linux/utsname.h
 +++ b/include/linux/utsname.h
 @@ -37,9 +37,12 @@ struct new_utsname {
  #include linux/nsproxy.h
  #include linux/err.h
  
 +struct user_namespace;
 +
  struct uts_namespace {
   struct kref kref;
   struct new_utsname name;
 + struct user_namespace *user_ns;
  };
  extern struct uts_namespace init_uts_ns;
  
 diff --git a/init/version.c b/init/version.c
 index adff586..97bb86f 100644
 --- a/init/version.c
 +++ b/init/version.c
 @@ -21,6 +21,7 @@ extern int version_string(LINUX_VERSION_CODE);
  int version_string(LINUX_VERSION_CODE);
  #endif
  
 +extern struct user_namespace init_user_ns;
  struct uts_namespace init_uts_ns = {
   .kref = {
   .refcount   = ATOMIC_INIT(2),
 @@ -33,6 +34,7 @@ struct uts_namespace init_uts_ns = {
   .machine= UTS_MACHINE,
   .domainname = UTS_DOMAINNAME,
   },
 + .user_ns = init_user_ns,
  };
  EXPORT_SYMBOL_GPL(init_uts_ns);
  
 diff --git a/kernel/nsproxy.c b/kernel/nsproxy.c
 index f74e6c0..034dc2e 100644
 --- a/kernel/nsproxy.c
 +++ b/kernel/nsproxy.c
 @@ -74,6 +74,11 @@ static struct nsproxy *create_new_namespaces(unsigned long 
 flags,
   err = PTR_ERR(new_nsp-uts_ns);
   goto out_uts;
   }
 + if (new_nsp-uts_ns != tsk-nsproxy-uts_ns) {
 + put_user_ns(new_nsp-uts_ns-user_ns);
 + new_nsp-uts_ns-user_ns = task_cred_xxx(tsk, user)-user_ns;
 + get_user_ns(new_nsp-uts_ns-user_ns);
 + }
  
   new_nsp-ipc_ns = copy_ipcs(flags, tsk-nsproxy-ipc_ns);
   if (IS_ERR(new_nsp-ipc_ns)) {
 diff --git a/kernel/user.c b/kernel/user.c
 index 5c598ca..9e03e9c 100644
 --- a/kernel/user.c
 +++ b/kernel/user.c
 @@ -17,9 +17,13 @@
  #include linux/module.h
  #include linux/user_namespace.h
  
 +/*
 + * userns count is 1 for root user, 1 for init_uts_ns,
 + * and 1 for... ?
 + */
  struct user_namespace init_user_ns = {
   .kref = {
 - .refcount   = ATOMIC_INIT(2),
 + .refcount   = ATOMIC_INIT(3),
   },
   .creator = root_user,
  };
 @@ -47,7 +51,7 @@ static struct kmem_cache *uid_cachep;
   */
  static DEFINE_SPINLOCK(uidhash_lock);
  
 -/* root_user.__count is 2, 1 for init task cred, 1 for init_user_ns-creator 
 */
 +/* root_user.__count is 2, 1 for init task cred, 1 for init_user_ns-user_ns 
 */
  struct user_struct root_user = {
   .__count= ATOMIC_INIT(2),
   .processes  = ATOMIC_INIT(1),
 diff --git a/kernel/utsname.c b/kernel/utsname.c
 index 8a82b4b..a7b3a8d 100644
 --- a/kernel/utsname.c
 +++ b/kernel/utsname.c
 @@ -14,6 +14,7 @@
  #include linux/utsname.h
  #include linux/err.h
  #include linux/slab.h
 +#include linux/user_namespace.h
  
  static struct uts_namespace *create_uts_ns(void)
  {
 @@ -40,6 +41,8 @@ static struct uts_namespace *clone_uts_ns(struct 
 uts_namespace *old_ns)
  
   down_read(uts_sem);
   memcpy(ns-name, old_ns-name, sizeof(ns-name));
 + ns-user_ns = old_ns-user_ns;
 + get_user_ns(ns-user_ns);
   up_read(uts_sem);
   return ns;
  }
 @@ -71,5 +74,6 @@ void free_uts_ns(struct kref *kref)
   struct uts_namespace *ns;
  
   ns = container_of(kref, struct uts_namespace, kref);
 + put_user_ns(ns-user_ns);
   kfree(ns);
  }
___
Containers mailing list
contain...@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

___
Devel mailing list
Devel@openvz.org
https://openvz.org/mailman/listinfo/devel