[rfc] fcntl: Add F_GETOWNER_UIDS option

[rfc] fcntl: Add F_GETOWNER_UIDS option

[Cyrill Gorcunov]

Hi guys,

I would like to get opinions on this proposal.
What do you think? Is there some security check
I'm missing?

---
From: Cyrill Gorcunov <gorcunov@openvz.org>
Subject: fcntl: Add F_GETOWNER_UIDS option

When we restore file descriptors we would like
them to look exactly as they were at dumping time.

With help of fcntl it's almost possible, the missing
snippet is file owners UIDs.

To be able to read their values the F_GETOWNER_UIDS
is introduced.

This option is valid iif CONFIG_CHECKPOINT_RESTORE
is turned on, otherwise returning -EINVAL.

Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
---
 fs/fcntl.c                  |   33 +++++++++++++++++++++++++++++++++
 include/asm-generic/fcntl.h |    4 ++++
 security/selinux/hooks.c    |    1 +
 3 files changed, 38 insertions(+)

Index: linux-2.6.git/fs/fcntl.c
===================================================================
--- linux-2.6.git.orig/fs/fcntl.c
+++ linux-2.6.git/fs/fcntl.c
@@ -20,6 +20,7 @@
 #include <linux/signal.h>
 #include <linux/rcupdate.h>
 #include <linux/pid_namespace.h>
+#include <linux/user_namespace.h>
 
 #include <asm/poll.h>
 #include <asm/siginfo.h>
@@ -340,6 +341,35 @@ static int f_getown_ex(struct file *filp
 	return ret;
 }
 
+#ifdef CONFIG_CHECKPOINT_RESTORE
+static int f_getowner_uids(struct file *filp, unsigned long arg)
+{
+	struct user_namespace *user_ns = current_user_ns();
+	const struct cred *cred = current_cred();
+	uid_t * __user dst = (void * __user)arg;
+	uid_t src[2];
+	int err;
+
+	read_lock(&filp->f_owner.lock);
+	src[0] = filp->f_owner.uid;
+	src[1] = filp->f_owner.euid;
+	read_unlock(&filp->f_owner.lock);
+
+	src[0] = user_ns_map_uid(user_ns, cred, src[0]);
+	src[1] = user_ns_map_uid(user_ns, cred, src[1]);
+
+	err  = put_user(src[0], &dst[0]);
+	err |= put_user(src[1], &dst[1]);
+
+	return err;
+}
+#else
+static int f_getowner_uids(struct file *filp, unsigned long arg)
+{
+	return -EINVAL;
+}
+#endif
+
 static long do_fcntl(int fd, unsigned int cmd, unsigned long arg,
 		struct file *filp)
 {
@@ -396,6 +426,9 @@ static long do_fcntl(int fd, unsigned in
 	case F_SETOWN_EX:
 		err = f_setown_ex(filp, arg);
 		break;
+	case F_GETOWNER_UIDS:
+		err = f_getowner_uids(filp, arg);
+		break;
 	case F_GETSIG:
 		err = filp->f_owner.signum;
 		break;
Index: linux-2.6.git/include/asm-generic/fcntl.h
===================================================================
--- linux-2.6.git.orig/include/asm-generic/fcntl.h
+++ linux-2.6.git/include/asm-generic/fcntl.h
@@ -120,6 +120,10 @@
 #define F_GETOWN_EX	16
 #endif
 
+#ifndef F_GETOWNER_UIDS
+#define F_GETOWNER_UIDS	17
+#endif
+
 #define F_OWNER_TID	0
 #define F_OWNER_PID	1
 #define F_OWNER_PGRP	2
Index: linux-2.6.git/security/selinux/hooks.c
===================================================================
--- linux-2.6.git.orig/security/selinux/hooks.c
+++ linux-2.6.git/security/selinux/hooks.c
@@ -3138,6 +3138,7 @@ static int selinux_file_fcntl(struct fil
 	case F_GETFL:
 	case F_GETOWN:
 	case F_GETSIG:
+	case F_GETOWNER_UIDS:
 		/* Just check FD__USE permission */
 		err = file_has_perm(cred, file, 0);
 		break;

Re: [rfc] fcntl: Add F_GETOWNER_UIDS option

[Oleg Nesterov]

On 03/26, Cyrill Gorcunov wrote:
>
> +#ifdef CONFIG_CHECKPOINT_RESTORE
> +static int f_getowner_uids(struct file *filp, unsigned long arg)
> +{
> +	struct user_namespace *user_ns = current_user_ns();
> +	const struct cred *cred = current_cred();
> +	uid_t * __user dst = (void * __user)arg;
> +	uid_t src[2];
> +	int err;
> +
> +	read_lock(&filp->f_owner.lock);
> +	src[0] = filp->f_owner.uid;
> +	src[1] = filp->f_owner.euid;
> +	read_unlock(&filp->f_owner.lock);
> +
> +	src[0] = user_ns_map_uid(user_ns, cred, src[0]);
> +	src[1] = user_ns_map_uid(user_ns, cred, src[1]);

Why?

In this case user_ns_map_uid() is "nop", it should always return
the last arg, no?

Oleg.

Re: [rfc] fcntl: Add F_GETOWNER_UIDS option

[Cyrill Gorcunov]

On Mon, Mar 26, 2012 at 06:43:47PM +0200, Oleg Nesterov wrote:
> On 03/26, Cyrill Gorcunov wrote:
> >
> > +#ifdef CONFIG_CHECKPOINT_RESTORE
> > +static int f_getowner_uids(struct file *filp, unsigned long arg)
> > +{
> > +	struct user_namespace *user_ns = current_user_ns();
> > +	const struct cred *cred = current_cred();
> > +	uid_t * __user dst = (void * __user)arg;
> > +	uid_t src[2];
> > +	int err;
> > +
> > +	read_lock(&filp->f_owner.lock);
> > +	src[0] = filp->f_owner.uid;
> > +	src[1] = filp->f_owner.euid;
> > +	read_unlock(&filp->f_owner.lock);
> > +
> > +	src[0] = user_ns_map_uid(user_ns, cred, src[0]);
> > +	src[1] = user_ns_map_uid(user_ns, cred, src[1]);
> 
> Why?
> 
> In this case user_ns_map_uid() is "nop", it should always return
> the last arg, no?

Yes, but I wanted to be on safe side, and if one day user_ns_map_uid
get changed this function won't be security hole. Or I miss something
in general?

	Cyrill

Re: [rfc] fcntl: Add F_GETOWNER_UIDS option

[Oleg Nesterov]

On 03/26, Cyrill Gorcunov wrote:
>
> On Mon, Mar 26, 2012 at 06:43:47PM +0200, Oleg Nesterov wrote:
> > On 03/26, Cyrill Gorcunov wrote:
> > >
> > > +#ifdef CONFIG_CHECKPOINT_RESTORE
> > > +static int f_getowner_uids(struct file *filp, unsigned long arg)
> > > +{
> > > +	struct user_namespace *user_ns = current_user_ns();
> > > +	const struct cred *cred = current_cred();
> > > +	uid_t * __user dst = (void * __user)arg;
> > > +	uid_t src[2];
> > > +	int err;
> > > +
> > > +	read_lock(&filp->f_owner.lock);
> > > +	src[0] = filp->f_owner.uid;
> > > +	src[1] = filp->f_owner.euid;
> > > +	read_unlock(&filp->f_owner.lock);
> > > +
> > > +	src[0] = user_ns_map_uid(user_ns, cred, src[0]);
> > > +	src[1] = user_ns_map_uid(user_ns, cred, src[1]);
> >
> > Why?
> >
> > In this case user_ns_map_uid() is "nop", it should always return
> > the last arg, no?
>
> Yes, but I wanted to be on safe side, and if one day user_ns_map_uid
> get changed this function won't be security hole.

Can't understand.

user_ns_map_uid() should translate uid_t from one namespace to another,
in this case the namespace is the same.

user_ns_map_uid(cred->user_ns, cred) must be the identical mapping,
no matter how we change the implementation.

What I think you need is
user_ns_map_uid(current_user_ns(), filp->f_owner.cred), the only
problem is that f_owner.cred doesn't exist.

> Or I miss something
> in general?

Or me. Add Serge, may be I missed something.

Oleg.

Re: [rfc] fcntl: Add F_GETOWNER_UIDS option

[Cyrill Gorcunov]

On Tue, Mar 27, 2012 at 05:25:34PM +0200, Oleg Nesterov wrote:
> user_ns_map_uid() should translate uid_t from one namespace to another,
> in this case the namespace is the same.
> 
> user_ns_map_uid(cred->user_ns, cred) must be the identical mapping,
> no matter how we change the implementation.
> 
> What I think you need is
> user_ns_map_uid(current_user_ns(), filp->f_owner.cred), the only
> problem is that f_owner.cred doesn't exist.
> 

Hmm, I was confused by likely() in user_ns_map_uid. But indeed, I think
you're so right. Is there some reason why we can't carry f_owner.cred
pointer?

> > Or I miss something
> > in general?
> 
> Or me. Add Serge, may be I missed something.
> 
> Oleg.
> 

	Cyrill

Re: [rfc] fcntl: Add F_GETOWNER_UIDS option

[Serge E. Hallyn]

Quoting Cyrill Gorcunov (gorcunov@openvz.org):
> On Tue, Mar 27, 2012 at 05:25:34PM +0200, Oleg Nesterov wrote:
> > user_ns_map_uid() should translate uid_t from one namespace to another,
> > in this case the namespace is the same.
> > 
> > user_ns_map_uid(cred->user_ns, cred) must be the identical mapping,
> > no matter how we change the implementation.
> > 
> > What I think you need is
> > user_ns_map_uid(current_user_ns(), filp->f_owner.cred), the only
> > problem is that f_owner.cred doesn't exist.
> > 
> 
> Hmm, I was confused by likely() in user_ns_map_uid. But indeed, I think
> you're so right. Is there some reason why we can't carry f_owner.cred
> pointer?

We would need that for this, yes.  However, Eric is working on a new
patchset which changes the cross-userns uid mappings.  I think it's
worth simply leaving a comment that this will need to be addressed,
and leave in the unconverted uid.

thanks,
-serge

Re: [rfc] fcntl: Add F_GETOWNER_UIDS option

[Cyrill Gorcunov]

On Tue, Mar 27, 2012 at 10:29:23PM +0000, Serge E. Hallyn wrote:
> Quoting Cyrill Gorcunov (gorcunov@openvz.org):
> > On Tue, Mar 27, 2012 at 05:25:34PM +0200, Oleg Nesterov wrote:
> > > user_ns_map_uid() should translate uid_t from one namespace to another,
> > > in this case the namespace is the same.
> > > 
> > > user_ns_map_uid(cred->user_ns, cred) must be the identical mapping,
> > > no matter how we change the implementation.
> > > 
> > > What I think you need is
> > > user_ns_map_uid(current_user_ns(), filp->f_owner.cred), the only
> > > problem is that f_owner.cred doesn't exist.
> > > 
> > 
> > Hmm, I was confused by likely() in user_ns_map_uid. But indeed, I think
> > you're so right. Is there some reason why we can't carry f_owner.cred
> > pointer?
> 
> We would need that for this, yes.  However, Eric is working on a new
> patchset which changes the cross-userns uid mappings.  I think it's
> worth simply leaving a comment that this will need to be addressed,
> and leave in the unconverted uid.

Hi Serge, thanks for info. But if it will be unconverted uid, can't
be there some security problem with that which I missed?

	Cyrill

Re: [rfc] fcntl: Add F_GETOWNER_UIDS option

[Serge E. Hallyn]

Quoting Cyrill Gorcunov (gorcunov@openvz.org):
> On Tue, Mar 27, 2012 at 10:29:23PM +0000, Serge E. Hallyn wrote:
> > Quoting Cyrill Gorcunov (gorcunov@openvz.org):
> > > On Tue, Mar 27, 2012 at 05:25:34PM +0200, Oleg Nesterov wrote:
> > > > user_ns_map_uid() should translate uid_t from one namespace to another,
> > > > in this case the namespace is the same.
> > > > 
> > > > user_ns_map_uid(cred->user_ns, cred) must be the identical mapping,
> > > > no matter how we change the implementation.
> > > > 
> > > > What I think you need is
> > > > user_ns_map_uid(current_user_ns(), filp->f_owner.cred), the only
> > > > problem is that f_owner.cred doesn't exist.
> > > > 
> > > 
> > > Hmm, I was confused by likely() in user_ns_map_uid. But indeed, I think
> > > you're so right. Is there some reason why we can't carry f_owner.cred
> > > pointer?
> > 
> > We would need that for this, yes.  However, Eric is working on a new
> > patchset which changes the cross-userns uid mappings.  I think it's
> > worth simply leaving a comment that this will need to be addressed,
> > and leave in the unconverted uid.
> 
> Hi Serge, thanks for info. But if it will be unconverted uid, can't
> be there some security problem with that which I missed?

Noone is really using the user namespaces right now, but rather than
adding the cred (and refcounting concerns), my suggestion for now
would be to hardcode a check in modown() that current_user_ns() ==
&init_user_ns.

I *did* have a patch in the past which added the cred to fown, but
no idea where it is right now...

-serge

Re: [rfc] fcntl: Add F_GETOWNER_UIDS option

[Eric W. Biederman]

"Serge E. Hallyn" <serge@hallyn.com> writes:

> Quoting Cyrill Gorcunov (gorcunov@openvz.org):
>> On Tue, Mar 27, 2012 at 10:29:23PM +0000, Serge E. Hallyn wrote:
>> > Quoting Cyrill Gorcunov (gorcunov@openvz.org):
>> > > On Tue, Mar 27, 2012 at 05:25:34PM +0200, Oleg Nesterov wrote:
>> > > > user_ns_map_uid() should translate uid_t from one namespace to another,
>> > > > in this case the namespace is the same.
>> > > > 
>> > > > user_ns_map_uid(cred->user_ns, cred) must be the identical mapping,
>> > > > no matter how we change the implementation.
>> > > > 
>> > > > What I think you need is
>> > > > user_ns_map_uid(current_user_ns(), filp->f_owner.cred), the only
>> > > > problem is that f_owner.cred doesn't exist.
>> > > > 
>> > > 
>> > > Hmm, I was confused by likely() in user_ns_map_uid. But indeed, I think
>> > > you're so right. Is there some reason why we can't carry f_owner.cred
>> > > pointer?
>> > 
>> > We would need that for this, yes.  However, Eric is working on a new
>> > patchset which changes the cross-userns uid mappings.  I think it's
>> > worth simply leaving a comment that this will need to be addressed,
>> > and leave in the unconverted uid.
>> 
>> Hi Serge, thanks for info. But if it will be unconverted uid, can't
>> be there some security problem with that which I missed?

I would suggest the easy route and create a KCONFIG dependency
on !CONFIG_USER_NS until the code for that is a little farther along.

Hopefully later this week or begginning of next week I should be posting
my patches and seeing how well the rest of the world takes them.

> Noone is really using the user namespaces right now, but rather than
> adding the cred (and refcounting concerns), my suggestion for now
> would be to hardcode a check in modown() that current_user_ns() ==
> &init_user_ns.
>
> I *did* have a patch in the past which added the cred to fown, but
> no idea where it is right now...

So I guess there are two questions.
- Does it make sense besides translation to add a cred here in general?

- How will it work with the user_namespace?

  I am just about ready to post a patchset that at the edges of
  userspace maps all uid and gids into uid and gids in the initial user
  namespace.

  There is a lot of noise but in net the code is terribly simple, easy
  to maintain and measurably faster (when compiled out) than what we are
  doing now, and most importantly it has a reasonable chance of catching
  all of the permission checks.

  And I have already converted fcntl.  So once my patchset goes in it
  should take all of 5 seconds to get the compile error and fix the code
  to be user_namespace friendly.

Eric

Re: [rfc] fcntl: Add F_GETOWNER_UIDS option

[Cyrill Gorcunov]

On Tue, Mar 27, 2012 at 07:22:48PM -0700, Eric W. Biederman wrote:
> >> Hi Serge, thanks for info. But if it will be unconverted uid, can't
> >> be there some security problem with that which I missed?
> 
> I would suggest the easy route and create a KCONFIG dependency
> on !CONFIG_USER_NS until the code for that is a little farther along.
> 
> Hopefully later this week or begginning of next week I should be posting
> my patches and seeing how well the rest of the world takes them.
> 

CC me on them, please.

> > Noone is really using the user namespaces right now, but rather than
> > adding the cred (and refcounting concerns), my suggestion for now
> > would be to hardcode a check in modown() that current_user_ns() ==
> > &init_user_ns.

OK, thanks

> >
> > I *did* have a patch in the past which added the cred to fown, but
> > no idea where it is right now...
> 
> So I guess there are two questions.
> - Does it make sense besides translation to add a cred here in general?
> 

I personally fail to find a reason except uids translation.

> - How will it work with the user_namespace?
> 
>   I am just about ready to post a patchset that at the edges of
>   userspace maps all uid and gids into uid and gids in the initial user
>   namespace.

So, we could map tme into initial user namesapce right? And we could
require for a while that F_GETOWNER_UIDS should be called from initial
user namespace only. Then we could extend it for being called from any
user-namespace if such need appear. Or I miss something?

	Cyrill

Re: [rfc] fcntl: Add F_GETOWNER_UIDS option

[Cyrill Gorcunov]

On Wed, Mar 28, 2012 at 12:51:02AM -0700, Eric W. Biederman wrote:
> > And we could require for a while that F_GETOWNER_UIDS should be called
> > from initial user namespace only. Then we could extend it for being
> > called from any user-namespace if such need appear. Or I miss
> > something?
> 
> Yes.  All that is needed in the short term to do this is a Kconfig
> dependency that limits it a kernel with user namespace support not
> built in something like: "depends !USER_NS"
> 
> Or a check like:
> 	if (current_user_ns() != init_user_ns)
>         	return -EINVAL;
> 
> Basically the mapping would ultimately become:
> uid = from_kuid(current_user_ns(), fown->uid);
> euid = from_kuid(current_user_ns(), fown->euid);
> 
> The different types allow a compile error if you forget the translation.

OK, thanks for the hint!

	Cyrill

Re: [rfc] fcntl: Add F_GETOWNER_UIDS option

[Cyrill Gorcunov]

Something like below i think (yet untested).

	Cyrill
---
From: Cyrill Gorcunov <gorcunov@openvz.org>
Subject: fcntl: Add F_GETOWNER_UIDS option v2

When we restore file descriptors we would like
them to look exactly as they were at dumping time.

With help of fcntl it's almost possible, the missing
snippet is file owners UIDs.

To be able to read their values the F_GETOWNER_UIDS
is introduced.

This option is valid iif CONFIG_CHECKPOINT_RESTORE
is turned on, otherwise returning -EINVAL.

Also the call must be done from initial user-namespace
for a while. This limitation migh be relaxed after, once
proper [e]uids mapping between namespaces and file owners
will be implemented.

Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
CC: "Eric W. Biederman" <ebiederm@xmission.com>
CC: "Serge E. Hallyn" <serge@hallyn.com>
CC: Oleg Nesterov <oleg@redhat.com>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: Pavel Emelyanov <xemul@parallels.com>
---
 fs/fcntl.c                  |   32 ++++++++++++++++++++++++++++++++
 include/asm-generic/fcntl.h |    4 ++++
 security/selinux/hooks.c    |    1 +
 3 files changed, 37 insertions(+)

Index: linux-2.6.git/fs/fcntl.c
===================================================================
--- linux-2.6.git.orig/fs/fcntl.c
+++ linux-2.6.git/fs/fcntl.c
@@ -20,6 +20,7 @@
 #include <linux/signal.h>
 #include <linux/rcupdate.h>
 #include <linux/pid_namespace.h>
+#include <linux/user_namespace.h>
 
 #include <asm/poll.h>
 #include <asm/siginfo.h>
@@ -340,6 +341,34 @@ static int f_getown_ex(struct file *filp
 	return ret;
 }
 
+#ifdef CONFIG_CHECKPOINT_RESTORE
+static int f_getowner_uids(struct file *filp, unsigned long arg)
+{
+	struct user_namespace *user_ns = current_user_ns();
+	uid_t * __user dst = (void * __user)arg;
+	uid_t src[2];
+	int err;
+
+        if (user_ns != &init_user_ns)
+                return -EINVAL;
+
+	read_lock(&filp->f_owner.lock);
+	src[0] = filp->f_owner.uid;
+	src[1] = filp->f_owner.euid;
+	read_unlock(&filp->f_owner.lock);
+
+	err  = put_user(src[0], &dst[0]);
+	err |= put_user(src[1], &dst[1]);
+
+	return err;
+}
+#else
+static int f_getowner_uids(struct file *filp, unsigned long arg)
+{
+	return -EINVAL;
+}
+#endif
+
 static long do_fcntl(int fd, unsigned int cmd, unsigned long arg,
 		struct file *filp)
 {
@@ -396,6 +425,9 @@ static long do_fcntl(int fd, unsigned in
 	case F_SETOWN_EX:
 		err = f_setown_ex(filp, arg);
 		break;
+	case F_GETOWNER_UIDS:
+		err = f_getowner_uids(filp, arg);
+		break;
 	case F_GETSIG:
 		err = filp->f_owner.signum;
 		break;
Index: linux-2.6.git/include/asm-generic/fcntl.h
===================================================================
--- linux-2.6.git.orig/include/asm-generic/fcntl.h
+++ linux-2.6.git/include/asm-generic/fcntl.h
@@ -120,6 +120,10 @@
 #define F_GETOWN_EX	16
 #endif
 
+#ifndef F_GETOWNER_UIDS
+#define F_GETOWNER_UIDS	17
+#endif
+
 #define F_OWNER_TID	0
 #define F_OWNER_PID	1
 #define F_OWNER_PGRP	2
Index: linux-2.6.git/security/selinux/hooks.c
===================================================================
--- linux-2.6.git.orig/security/selinux/hooks.c
+++ linux-2.6.git/security/selinux/hooks.c
@@ -3138,6 +3138,7 @@ static int selinux_file_fcntl(struct fil
 	case F_GETFL:
 	case F_GETOWN:
 	case F_GETSIG:
+	case F_GETOWNER_UIDS:
 		/* Just check FD__USE permission */
 		err = file_has_perm(cred, file, 0);
 		break;

Re: [rfc] fcntl: Add F_GETOWNER_UIDS option

[Serge E. Hallyn]

Quoting Cyrill Gorcunov (gorcunov@openvz.org):
> Something like below i think (yet untested).
> 
> 	Cyrill
> ---
> From: Cyrill Gorcunov <gorcunov@openvz.org>
> Subject: fcntl: Add F_GETOWNER_UIDS option v2
> 
> When we restore file descriptors we would like
> them to look exactly as they were at dumping time.
> 
> With help of fcntl it's almost possible, the missing
> snippet is file owners UIDs.
> 
> To be able to read their values the F_GETOWNER_UIDS
> is introduced.
> 
> This option is valid iif CONFIG_CHECKPOINT_RESTORE
> is turned on, otherwise returning -EINVAL.
> 
> Also the call must be done from initial user-namespace
> for a while. This limitation migh be relaxed after, once
> proper [e]uids mapping between namespaces and file owners
> will be implemented.
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
> CC: "Eric W. Biederman" <ebiederm@xmission.com>
> CC: "Serge E. Hallyn" <serge@hallyn.com>

Sorry to cause trouble, cause as i say it's not particularly important
right now, but note that what you're doing here only ensures that the
person doing the checkpoint is in init_user_ns.  It says nothing about
the credentials in the fown_struct.

If you want to go with this patch, it's fine by me.  If you want to
just add the struct cred to the f_owner and do proper uid conversion,
I'll support that too.  (Just grab a ref to the cred in
fs/fcntl.c:f_modown(), and drop the ref in fs/file_table.c:__fput() ).

Nit: note the indenting in the init_user_ns check below.

> CC: Oleg Nesterov <oleg@redhat.com>
> CC: Andrew Morton <akpm@linux-foundation.org>
> CC: Pavel Emelyanov <xemul@parallels.com>
> ---
>  fs/fcntl.c                  |   32 ++++++++++++++++++++++++++++++++
>  include/asm-generic/fcntl.h |    4 ++++
>  security/selinux/hooks.c    |    1 +
>  3 files changed, 37 insertions(+)
> 
> Index: linux-2.6.git/fs/fcntl.c
> ===================================================================
> --- linux-2.6.git.orig/fs/fcntl.c
> +++ linux-2.6.git/fs/fcntl.c
> @@ -20,6 +20,7 @@
>  #include <linux/signal.h>
>  #include <linux/rcupdate.h>
>  #include <linux/pid_namespace.h>
> +#include <linux/user_namespace.h>
>  
>  #include <asm/poll.h>
>  #include <asm/siginfo.h>
> @@ -340,6 +341,34 @@ static int f_getown_ex(struct file *filp
>  	return ret;
>  }
>  
> +#ifdef CONFIG_CHECKPOINT_RESTORE
> +static int f_getowner_uids(struct file *filp, unsigned long arg)
> +{
> +	struct user_namespace *user_ns = current_user_ns();
> +	uid_t * __user dst = (void * __user)arg;
> +	uid_t src[2];
> +	int err;
> +
> +        if (user_ns != &init_user_ns)
> +                return -EINVAL;
> +
> +	read_lock(&filp->f_owner.lock);
> +	src[0] = filp->f_owner.uid;
> +	src[1] = filp->f_owner.euid;
> +	read_unlock(&filp->f_owner.lock);
> +
> +	err  = put_user(src[0], &dst[0]);
> +	err |= put_user(src[1], &dst[1]);
> +
> +	return err;
> +}
> +#else
> +static int f_getowner_uids(struct file *filp, unsigned long arg)
> +{
> +	return -EINVAL;
> +}
> +#endif
> +
>  static long do_fcntl(int fd, unsigned int cmd, unsigned long arg,
>  		struct file *filp)
>  {
> @@ -396,6 +425,9 @@ static long do_fcntl(int fd, unsigned in
>  	case F_SETOWN_EX:
>  		err = f_setown_ex(filp, arg);
>  		break;
> +	case F_GETOWNER_UIDS:
> +		err = f_getowner_uids(filp, arg);
> +		break;
>  	case F_GETSIG:
>  		err = filp->f_owner.signum;
>  		break;
> Index: linux-2.6.git/include/asm-generic/fcntl.h
> ===================================================================
> --- linux-2.6.git.orig/include/asm-generic/fcntl.h
> +++ linux-2.6.git/include/asm-generic/fcntl.h
> @@ -120,6 +120,10 @@
>  #define F_GETOWN_EX	16
>  #endif
>  
> +#ifndef F_GETOWNER_UIDS
> +#define F_GETOWNER_UIDS	17
> +#endif
> +
>  #define F_OWNER_TID	0
>  #define F_OWNER_PID	1
>  #define F_OWNER_PGRP	2
> Index: linux-2.6.git/security/selinux/hooks.c
> ===================================================================
> --- linux-2.6.git.orig/security/selinux/hooks.c
> +++ linux-2.6.git/security/selinux/hooks.c
> @@ -3138,6 +3138,7 @@ static int selinux_file_fcntl(struct fil
>  	case F_GETFL:
>  	case F_GETOWN:
>  	case F_GETSIG:
> +	case F_GETOWNER_UIDS:
>  		/* Just check FD__USE permission */
>  		err = file_has_perm(cred, file, 0);
>  		break;

Re: [rfc] fcntl: Add F_GETOWNER_UIDS option

[Oleg Nesterov]

On 03/28, Serge E. Hallyn wrote:
>
> If you want to
> just add the struct cred to the f_owner and do proper uid conversion,
> I'll support that too.  (Just grab a ref to the cred in
> fs/fcntl.c:f_modown(), and drop the ref in fs/file_table.c:__fput() ).

In this case f_owner.*uid should go away, I guess. And sigio_perm()
should be unified with kill_ok_by_cred() somehow (modulo
security_file_send_sigiotask).

Right?

Oleg.

Re: [rfc] fcntl: Add F_GETOWNER_UIDS option

[Serge Hallyn]

Quoting Oleg Nesterov (oleg@redhat.com):
> On 03/28, Serge E. Hallyn wrote:
> >
> > If you want to
> > just add the struct cred to the f_owner and do proper uid conversion,
> > I'll support that too.  (Just grab a ref to the cred in
> > fs/fcntl.c:f_modown(), and drop the ref in fs/file_table.c:__fput() ).
> 
> In this case f_owner.*uid should go away, I guess.

Yup.

Which I guess is all the more reason *not* to do this unless we end up
not going with Eric's userns mapping patchset (which is unlikely).

> And sigio_perm()
> should be unified with kill_ok_by_cred() somehow (modulo
> security_file_send_sigiotask).
> 
> Right?

Maybe, but other differences include current being the signal sender in
one and recipient in the other, and CAP_KILL being relevent in only
one.

-serge

Re: [rfc] fcntl: Add F_GETOWNER_UIDS option

[Oleg Nesterov]

On 03/28, Serge Hallyn wrote:
>
> Quoting Oleg Nesterov (oleg@redhat.com):
> > On 03/28, Serge E. Hallyn wrote:
> > >
> > > If you want to
> > > just add the struct cred to the f_owner and do proper uid conversion,
> > > I'll support that too.  (Just grab a ref to the cred in
> > > fs/fcntl.c:f_modown(), and drop the ref in fs/file_table.c:__fput() ).
> >
> > In this case f_owner.*uid should go away, I guess.
>
> Yup.
>
> Which I guess is all the more reason *not* to do this unless we end up
> not going with Eric's userns mapping patchset (which is unlikely).

Agreed,

> > And sigio_perm()
> > should be unified with kill_ok_by_cred() somehow (modulo
> > security_file_send_sigiotask).
> >
> > Right?
>
> Maybe, but other differences include current being the signal sender in
> one and recipient in the other, and CAP_KILL being relevent in only
> one.

Yes, yes, sure. "current" is meaningless for sigio_perm().

That is why I asked, the "somehow" above is not clear to me.

Oleg.

Re: [rfc] fcntl: Add F_GETOWNER_UIDS option

[Cyrill Gorcunov]

On Wed, Mar 28, 2012 at 04:30:44PM -0500, Serge Hallyn wrote:
> Quoting Oleg Nesterov (oleg@redhat.com):
> > On 03/28, Serge E. Hallyn wrote:
> > >
> > > If you want to
> > > just add the struct cred to the f_owner and do proper uid conversion,
> > > I'll support that too.  (Just grab a ref to the cred in
> > > fs/fcntl.c:f_modown(), and drop the ref in fs/file_table.c:__fput() ).
> > 
> > In this case f_owner.*uid should go away, I guess.
> 
> Yup.
> 
> Which I guess is all the more reason *not* to do this unless we end up
> not going with Eric's userns mapping patchset (which is unlikely).
> 
> > And sigio_perm()
> > should be unified with kill_ok_by_cred() somehow (modulo
> > security_file_send_sigiotask).
> > 
> > Right?
> 
> Maybe, but other differences include current being the signal sender in
> one and recipient in the other, and CAP_KILL being relevent in only
> one.

Hi Serge, thanks a lot for comments! Replying to prev email --
I've skipped cred part intentionally, I guess we need to wait
until Eric's patches hit LKML (if I understand all right) then
I'll expand the patch. I'll think a bit more tomorrow, ok?

	Cyrill

Re: [rfc] fcntl: Add F_GETOWNER_UIDS option

[Serge E. Hallyn]

Quoting Cyrill Gorcunov (gorcunov@openvz.org):
> On Wed, Mar 28, 2012 at 04:30:44PM -0500, Serge Hallyn wrote:
> > Quoting Oleg Nesterov (oleg@redhat.com):
> > > On 03/28, Serge E. Hallyn wrote:
> > > >
> > > > If you want to
> > > > just add the struct cred to the f_owner and do proper uid conversion,
> > > > I'll support that too.  (Just grab a ref to the cred in
> > > > fs/fcntl.c:f_modown(), and drop the ref in fs/file_table.c:__fput() ).
> > > 
> > > In this case f_owner.*uid should go away, I guess.
> > 
> > Yup.
> > 
> > Which I guess is all the more reason *not* to do this unless we end up
> > not going with Eric's userns mapping patchset (which is unlikely).
> > 
> > > And sigio_perm()
> > > should be unified with kill_ok_by_cred() somehow (modulo
> > > security_file_send_sigiotask).
> > > 
> > > Right?
> > 
> > Maybe, but other differences include current being the signal sender in
> > one and recipient in the other, and CAP_KILL being relevent in only
> > one.
> 
> Hi Serge, thanks a lot for comments! Replying to prev email --
> I've skipped cred part intentionally, I guess we need to wait
> until Eric's patches hit LKML (if I understand all right) then
> I'll expand the patch. I'll think a bit more tomorrow, ok?

Sure.

Thinking about it, the cred being stored right now is the cred in the
container.  That's what you want for checkpoint, right?  So if someone
with the privs to do it checkpoints a task in a child userns, and restarts
that without doing so in a child user ns, he should be allowed to do so.

So what I'm saying is that it's not in-defensible to just not change
anything in your original patch until we can discuss Eric's set.

If we were to *not* go with Eric's set, then when using your proposed
patch for debugging purposes, would we want to show a list of uids,
starting with the uid in the reader's user namespace, up to the
container being investigated?  So for instance if init_user_ns spawned
userns1, and that spawned userns2, and root in userns1 is seeking this
info for a f_owner in userns2, then he should see two userids, the one
mapped into usern1, and the one in userns2.

In Eric's set, we may want to show only the kuid (since the mapped
userid can be found other ways), or for convenience we may want to show
both the kuid and the mapped uid.

-serge

Re: [rfc] fcntl: Add F_GETOWNER_UIDS option

[Cyrill Gorcunov]

On Thu, Mar 29, 2012 at 02:30:53AM +0000, Serge E. Hallyn wrote:
> Quoting Cyrill Gorcunov (gorcunov@openvz.org):
> > On Wed, Mar 28, 2012 at 04:30:44PM -0500, Serge Hallyn wrote:
> > > Quoting Oleg Nesterov (oleg@redhat.com):
> > > > On 03/28, Serge E. Hallyn wrote:
> > > > >
> > > > > If you want to
> > > > > just add the struct cred to the f_owner and do proper uid conversion,
> > > > > I'll support that too.  (Just grab a ref to the cred in
> > > > > fs/fcntl.c:f_modown(), and drop the ref in fs/file_table.c:__fput() ).
> > > > 
> > > > In this case f_owner.*uid should go away, I guess.
> > > 
> > > Yup.
> > > 
> > > Which I guess is all the more reason *not* to do this unless we end up
> > > not going with Eric's userns mapping patchset (which is unlikely).
> > > 
> > > > And sigio_perm()
> > > > should be unified with kill_ok_by_cred() somehow (modulo
> > > > security_file_send_sigiotask).
> > > > 
> > > > Right?
> > > 
> > > Maybe, but other differences include current being the signal sender in
> > > one and recipient in the other, and CAP_KILL being relevent in only
> > > one.
> > 
> > Hi Serge, thanks a lot for comments! Replying to prev email --
> > I've skipped cred part intentionally, I guess we need to wait
> > until Eric's patches hit LKML (if I understand all right) then
> > I'll expand the patch. I'll think a bit more tomorrow, ok?
> 
> Sure.
> 
> Thinking about it, the cred being stored right now is the cred in the
> container.  That's what you want for checkpoint, right?  So if someone

Hi Serge, sorry for delay, the stored creds are the ones a task has
at checkpoint time (we parse /proc/pid/status), and the dumper/restorer
works with root privileges so they should be able to change creds to
the former values on restore procedure.

> with the privs to do it checkpoints a task in a child userns, and restarts
> that without doing so in a child user ns, he should be allowed to do so.

I think so. Basically we require both checkpointer and restorer
to have admin rights before they do c/r (it might be relaxed in
future probably) and actually I think we're more oriented to
achieve stable c/r from init-namespace first (once this accomplished
then c/r from inside nested namespaces could be considered).

> So what I'm saying is that it's not in-defensible to just not change
> anything in your original patch until we can discuss Eric's set.
> 

Yes, I wanna take a look on Eric's set first just to get right
"picture" of everything. And I wanted to find a minimal solution
with current kernel code base which could be extended in future.

That said I guess the current init-ns-only approach should do the
trick for a while. And (thanks for pointing) I need to add a test
if a caller which tries to obtain uids has enought credentials
for that (probably CAP_FOWNER), right?

> If we were to *not* go with Eric's set, then when using your proposed
> patch for debugging purposes, would we want to show a list of uids,
> starting with the uid in the reader's user namespace, up to the
> container being investigated?  So for instance if init_user_ns spawned
> userns1, and that spawned userns2, and root in userns1 is seeking this
> info for a f_owner in userns2, then he should see two userids, the one
> mapped into usern1, and the one in userns2.
> 
> In Eric's set, we may want to show only the kuid (since the mapped
> userid can be found other ways), or for convenience we may want to show
> both the kuid and the mapped uid.

I suspect operating with kuid's will be a way more easier.

	Cyrill

Re: [rfc] fcntl: Add F_GETOWNER_UIDS option

[Serge Hallyn]

Quoting Cyrill Gorcunov (gorcunov@openvz.org):
> On Thu, Mar 29, 2012 at 02:30:53AM +0000, Serge E. Hallyn wrote:
> > Quoting Cyrill Gorcunov (gorcunov@openvz.org):
> > > On Wed, Mar 28, 2012 at 04:30:44PM -0500, Serge Hallyn wrote:
> > > > Quoting Oleg Nesterov (oleg@redhat.com):
> > > > > On 03/28, Serge E. Hallyn wrote:
> > > > > >
> > > > > > If you want to
> > > > > > just add the struct cred to the f_owner and do proper uid conversion,
> > > > > > I'll support that too.  (Just grab a ref to the cred in
> > > > > > fs/fcntl.c:f_modown(), and drop the ref in fs/file_table.c:__fput() ).
> > > > > 
> > > > > In this case f_owner.*uid should go away, I guess.
> > > > 
> > > > Yup.
> > > > 
> > > > Which I guess is all the more reason *not* to do this unless we end up
> > > > not going with Eric's userns mapping patchset (which is unlikely).
> > > > 
> > > > > And sigio_perm()
> > > > > should be unified with kill_ok_by_cred() somehow (modulo
> > > > > security_file_send_sigiotask).
> > > > > 
> > > > > Right?
> > > > 
> > > > Maybe, but other differences include current being the signal sender in
> > > > one and recipient in the other, and CAP_KILL being relevent in only
> > > > one.
> > > 
> > > Hi Serge, thanks a lot for comments! Replying to prev email --
> > > I've skipped cred part intentionally, I guess we need to wait
> > > until Eric's patches hit LKML (if I understand all right) then
> > > I'll expand the patch. I'll think a bit more tomorrow, ok?
> > 
> > Sure.
> > 
> > Thinking about it, the cred being stored right now is the cred in the
> > container.  That's what you want for checkpoint, right?  So if someone
> 
> Hi Serge, sorry for delay, the stored creds are the ones a task has
> at checkpoint time (we parse /proc/pid/status), and the dumper/restorer
> works with root privileges so they should be able to change creds to
> the former values on restore procedure.
> 
> > with the privs to do it checkpoints a task in a child userns, and restarts
> > that without doing so in a child user ns, he should be allowed to do so.
> 
> I think so. Basically we require both checkpointer and restorer
> to have admin rights before they do c/r (it might be relaxed in
> future probably) and actually I think we're more oriented to
> achieve stable c/r from init-namespace first (once this accomplished
> then c/r from inside nested namespaces could be considered).

Sounds good.

> > So what I'm saying is that it's not in-defensible to just not change
> > anything in your original patch until we can discuss Eric's set.
> > 
> 
> Yes, I wanna take a look on Eric's set first just to get right
> "picture" of everything. And I wanted to find a minimal solution
> with current kernel code base which could be extended in future.
> 
> That said I guess the current init-ns-only approach should do the
> trick for a while. And (thanks for pointing) I need to add a test
> if a caller which tries to obtain uids has enought credentials
> for that (probably CAP_FOWNER), right?

Sorry, I'm not sure which caller you mean.  Neither f_setown nor
f_getown require privilege right now.  Oh, you mean at restart?
f_setown to someone else's uid/pid means you may cause a signal to
be sent to them.  So CAP_KILL might be good?  You do through that
signal get *some* info about the file writes, though not contents.
So yeah, maybe (CAP_KILL|CAP_FOWNER).

> > If we were to *not* go with Eric's set, then when using your proposed
> > patch for debugging purposes, would we want to show a list of uids,
> > starting with the uid in the reader's user namespace, up to the
> > container being investigated?  So for instance if init_user_ns spawned
> > userns1, and that spawned userns2, and root in userns1 is seeking this
> > info for a f_owner in userns2, then he should see two userids, the one
> > mapped into usern1, and the one in userns2.
> > 
> > In Eric's set, we may want to show only the kuid (since the mapped
> > userid can be found other ways), or for convenience we may want to show
> > both the kuid and the mapped uid.
> 
> I suspect operating with kuid's will be a way more easier.

Yeah, I keep going back and forth on which makes more sense.  But
kuid's probably make more sense, even though they aren't what
userspace in container will see.  When you restore, the mapping
will give userspace what it expects;  and if you're going to
restart in a container with a different mapping, then you'll
have to convert the filesystem as well since its inodes will
store kuids, so may as well also convert the kuids in the
checkpoint image then.

-serge

Re: [rfc] fcntl: Add F_GETOWNER_UIDS option

[Cyrill Gorcunov]

On Fri, Mar 30, 2012 at 09:12:19AM -0500, Serge Hallyn wrote:
...
> > 
> > Yes, I wanna take a look on Eric's set first just to get right
> > "picture" of everything. And I wanted to find a minimal solution
> > with current kernel code base which could be extended in future.
> > 
> > That said I guess the current init-ns-only approach should do the
> > trick for a while. And (thanks for pointing) I need to add a test
> > if a caller which tries to obtain uids has enought credentials
> > for that (probably CAP_FOWNER), right?
> 
> Sorry, I'm not sure which caller you mean.  Neither f_setown nor
> f_getown require privilege right now.  Oh, you mean at restart?

I meant the dumper. Yes, at moment f_get/setown requires no privileges
but I'm not sure if uid/euid is same or less sensible information
than pid, that's why I though CAP_FOWNER might be worth to add, no?

> f_setown to someone else's uid/pid means you may cause a signal to
> be sent to them.  So CAP_KILL might be good?  You do through that
> signal get *some* info about the file writes, though not contents.
> So yeah, maybe (CAP_KILL|CAP_FOWNER).
...
> > I suspect operating with kuid's will be a way more easier.
> 
> Yeah, I keep going back and forth on which makes more sense.  But
> kuid's probably make more sense, even though they aren't what
> userspace in container will see.  When you restore, the mapping
> will give userspace what it expects;  and if you're going to
> restart in a container with a different mapping, then you'll
> have to convert the filesystem as well since its inodes will
> store kuids, so may as well also convert the kuids in the
> checkpoint image then.

Agreed (if only I'm not missimg somethig ;)

	Cyrill

Re: [rfc] fcntl: Add F_GETOWNER_UIDS option

[Serge E. Hallyn]

Quoting Cyrill Gorcunov (gorcunov@openvz.org):
> On Fri, Mar 30, 2012 at 09:12:19AM -0500, Serge Hallyn wrote:
> ...
> > > 
> > > Yes, I wanna take a look on Eric's set first just to get right
> > > "picture" of everything. And I wanted to find a minimal solution
> > > with current kernel code base which could be extended in future.
> > > 
> > > That said I guess the current init-ns-only approach should do the
> > > trick for a while. And (thanks for pointing) I need to add a test
> > > if a caller which tries to obtain uids has enought credentials
> > > for that (probably CAP_FOWNER), right?
> > 
> > Sorry, I'm not sure which caller you mean.  Neither f_setown nor
> > f_getown require privilege right now.  Oh, you mean at restart?
> 
> I meant the dumper. Yes, at moment f_get/setown requires no privileges
> but I'm not sure if uid/euid is same or less sensible information
> than pid, that's why I though CAP_FOWNER might be worth to add, no?

Hmm, I would say no, but that might be a good question for kees.

IMO it's not sensitive information and so no sense requiring privilege
(and encouraging handing out of extra privilage to get at the info)

Cc:ing kees.

> > f_setown to someone else's uid/pid means you may cause a signal to
> > be sent to them.  So CAP_KILL might be good?  You do through that
> > signal get *some* info about the file writes, though not contents.
> > So yeah, maybe (CAP_KILL|CAP_FOWNER).
> ...
> > > I suspect operating with kuid's will be a way more easier.
> > 
> > Yeah, I keep going back and forth on which makes more sense.  But
> > kuid's probably make more sense, even though they aren't what
> > userspace in container will see.  When you restore, the mapping
> > will give userspace what it expects;  and if you're going to
> > restart in a container with a different mapping, then you'll
> > have to convert the filesystem as well since its inodes will
> > store kuids, so may as well also convert the kuids in the
> > checkpoint image then.
> 
> Agreed (if only I'm not missimg somethig ;)
> 
> 	Cyrill

Re: [rfc] fcntl: Add F_GETOWNER_UIDS option

[Kees Cook]

On Fri, Mar 30, 2012 at 9:15 AM, Serge E. Hallyn <serge@hallyn.com> wrote:
> Quoting Cyrill Gorcunov (gorcunov@openvz.org):
>> On Fri, Mar 30, 2012 at 09:12:19AM -0500, Serge Hallyn wrote:
>> ...
>> > >
>> > > Yes, I wanna take a look on Eric's set first just to get right
>> > > "picture" of everything. And I wanted to find a minimal solution
>> > > with current kernel code base which could be extended in future.
>> > >
>> > > That said I guess the current init-ns-only approach should do the
>> > > trick for a while. And (thanks for pointing) I need to add a test
>> > > if a caller which tries to obtain uids has enought credentials
>> > > for that (probably CAP_FOWNER), right?
>> >
>> > Sorry, I'm not sure which caller you mean.  Neither f_setown nor
>> > f_getown require privilege right now.  Oh, you mean at restart?
>>
>> I meant the dumper. Yes, at moment f_get/setown requires no privileges
>> but I'm not sure if uid/euid is same or less sensible information
>> than pid, that's why I though CAP_FOWNER might be worth to add, no?
>
> Hmm, I would say no, but that might be a good question for kees.
>
> IMO it's not sensitive information and so no sense requiring privilege
> (and encouraging handing out of extra privilage to get at the info)

Nothing jumps out at me about just seeing uid/euid. Everything can be
construed as an information leak, but this don't seem like something
that needs special protection.

-Kees

-- 
Kees Cook
ChromeOS Security

Re: [rfc] fcntl: Add F_GETOWNER_UIDS option

[Cyrill Gorcunov]

On Fri, Mar 30, 2012 at 12:46:51PM -0700, Kees Cook wrote:
> >>
> >> I meant the dumper. Yes, at moment f_get/setown requires no privileges
> >> but I'm not sure if uid/euid is same or less sensible information
> >> than pid, that's why I though CAP_FOWNER might be worth to add, no?
> >
> > Hmm, I would say no, but that might be a good question for kees.
> >
> > IMO it's not sensitive information and so no sense requiring privilege
> > (and encouraging handing out of extra privilage to get at the info)
> 
> Nothing jumps out at me about just seeing uid/euid. Everything can be
> construed as an information leak, but this don't seem like something
> that needs special protection.

OK, thanks Kees.

	Cyrill