fcntl: Add F_GETOWNER_UIDS option

fs/fcntl.c +6 -0

+#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;

Quoted 7 lines · Cyrill Gorcunov

fs/fcntl.c +6 -0

+#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;

fs/fcntl.c static int f_getown_ex(struct file *filp · line 359

Quoted 10 lines

fs/fcntl.c +10 -0

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

Quoted 21 lines · Oleg Nesterov

Quoted 7 lines · Cyrill Gorcunov

fs/fcntl.c +6 -0

+#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;

fs/fcntl.c static int f_getown_ex(struct file *filp · line 359

Quoted 10 lines

fs/fcntl.c +10 -0

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

Quoted 2 lines

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?

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.

Quoted 2 lines

Or I miss something
in general?

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

Oleg.

Quoted 6 lines · Oleg Nesterov

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.

Quoted 4 lines

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?

Quoted 12 lines · Cyrill Gorcunov

Quoted 10 lines · Oleg Nesterov

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.

Quoted 3 lines

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.

Quoted 17 lines · Serge E. Hallyn

Quoted 15 lines · Cyrill Gorcunov

Quoted 10 lines · Oleg Nesterov

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?

Quoted 4 lines

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?

Quoted 18 lines · Cyrill Gorcunov

Quoted 17 lines · Serge E. Hallyn

Quoted 15 lines · Cyrill Gorcunov

Quoted 10 lines · Oleg Nesterov

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?

Quoted 7 lines

Quoted 4 lines

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

Quoted 2 lines

Quoted 2 lines

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.

Quoted 4 lines

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.

Quoted 3 lines

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.

Quoted 4 lines

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.

Something like below i think (yet untested).

Cyrill
---
From: Cyrill Gorcunov <[email protected]>
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 <[email protected]>

CC "Eric W. Biederman" <[email protected]>

CC "Serge E. Hallyn" <[email protected]>

CC Oleg Nesterov <[email protected]>

CC Andrew Morton <[email protected]>

CC Pavel Emelyanov <[email protected]>

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

fs/fcntl.c +32 -0

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

include/asm-generic/fcntl.h +4 -0

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

security/selinux/hooks.c +1 -0

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

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

Quoted 5 lines · Serge E. Hallyn

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?

Quoted 8 lines · Oleg Nesterov

Quoted 5 lines · Serge E. Hallyn

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

Quoted 4 lines

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

Quoted 1 line

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.

Quoted 21 lines · Serge Hallyn

Quoted 8 lines · Oleg Nesterov

Quoted 5 lines · Serge E. Hallyn

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

Quoted 5 lines

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

Right?

Quoted 3 lines

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?

Quoted 22 lines · Cyrill Gorcunov

Quoted 21 lines · Serge Hallyn

Quoted 8 lines · Oleg Nesterov

Quoted 5 lines · Serge E. Hallyn

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

Quoted 5 lines

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

Right?

Quoted 8 lines

Quoted 3 lines

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.

Quoted 32 lines · Serge E. Hallyn

Quoted 22 lines · Cyrill Gorcunov

Quoted 21 lines · Serge Hallyn

Quoted 8 lines · Oleg Nesterov

Quoted 5 lines · Serge E. Hallyn

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

Quoted 5 lines

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

Right?

Quoted 8 lines

Quoted 3 lines

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?

Quoted 4 lines

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.

Quoted 2 lines

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

Quoted 3 lines

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?

Quoted 8 lines

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.

Quoted 3 lines

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.

Quoted 5 lines

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.

Quoted 4 lines

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

Quoted 1 line

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.

On Fri, Mar 30, 2012 at 09:12:19AM -0500, Serge Hallyn wrote:
...

Quoted 10 lines

Quoted 9 lines

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?

Quoted 2 lines

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?

Quoted 4 lines

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

...

Quoted 10 lines

Quoted 1 line

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

Quoted 15 lines · Cyrill Gorcunov

On Fri, Mar 30, 2012 at 09:12:19AM -0500, Serge Hallyn wrote:
...

Quoted 12 lines

Quoted 9 lines

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?

Quoted 3 lines

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)

Quoted 5 lines

Quoted 4 lines

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?

Quoted 4 lines

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.

Quoted 7 lines · Oleg Nesterov

Quoted 5 lines · Serge E. Hallyn

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

Quoted 1 line

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

Quoted 5 lines

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.