[PATCH RFC] overlayfs,xattr: allow unprivileged users to whiteout

[PATCH RFC] overlayfs,xattr: allow unprivileged users to whiteout

[Serge Hallyn]

To mark a file which exists in the lower layer as deleted,
it creates a symbolic link to a file called "(overlay-whiteout)"
in the writeable mount, and sets a "trusted.overlay" xattr
on that link.

1. When the create the symbolic link as container root, not
as the global root

2. Allow root in a container to edit "trusted.overlay*"
xattrs.  Generally only global root is allowed to edit
"trusted.*"

With this patch, I'm able to delete files and directories in a
user-namespace-based overlayfs-backed container.  The overlay
writeable layer after "rm ab/ab; rmdir ab; mv xxx yyy;" ends up
looking like:

ls -l .local/share/lxc/u11/delta0/home/ubuntu/
total 0
lrwxrwxrwx 1 150000 150000 18 Feb 13 22:30 ab -> (overlay-whiteout)
lrwxrwxrwx 1 150000 150000 18 Feb 13 22:30 xxx -> (overlay-whiteout)
-rw-rw-r-- 1 151000 151000  0 Feb 13 03:53 yyy

(with 150000 being the mapped container root)

Note - the fs/xattr.c hunk could presumably be dropped if we
switched to using "user.overlay".  This could however cause
problems with pre-existing overlay deltas.  I don't really care
how this is done, but am wondering whether there is any good
reason why making overlay whiteouts should require root on
the host rather than only in the userns.

Signed-off-by: Serge Hallyn <serge.hallyn@ubuntu.com>
---
 fs/overlayfs/dir.c | 9 +++++++--
 fs/xattr.c         | 5 ++++-
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index a209409..3c4657b 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -12,6 +12,7 @@
 #include <linux/xattr.h>
 #include <linux/security.h>
 #include <linux/cred.h>
+#include <linux/sched.h>
 #include "overlayfs.h"
 
 static const char *ovl_whiteout_symlink = "(overlay-whiteout)";
@@ -38,8 +39,12 @@ static int ovl_whiteout(struct dentry *upperdir, struct dentry *dentry)
 	cap_raise(override_cred->cap_effective, CAP_SYS_ADMIN);
 	cap_raise(override_cred->cap_effective, CAP_DAC_OVERRIDE);
 	cap_raise(override_cred->cap_effective, CAP_FOWNER);
-	override_cred->fsuid = GLOBAL_ROOT_UID;
-	override_cred->fsgid = GLOBAL_ROOT_GID;
+	override_cred->fsuid = make_kuid(current_user_ns(), 0);
+	if (!uid_valid(override_cred->fsuid))
+		override_cred->fsuid = GLOBAL_ROOT_UID;
+	override_cred->fsgid = make_kgid(current_user_ns(), 0);
+	if (!gid_valid(override_cred->fsgid))
+		override_cred->fsgid = GLOBAL_ROOT_GID;
 	old_cred = override_creds(override_cred);
 
 	newdentry = lookup_one_len(dentry->d_name.name, upperdir,
diff --git a/fs/xattr.c b/fs/xattr.c
index 3377dff..edd826c 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -52,7 +52,10 @@ xattr_permission(struct inode *inode, const char *name, int mask)
 	 * The trusted.* namespace can only be accessed by privileged users.
 	 */
 	if (!strncmp(name, XATTR_TRUSTED_PREFIX, XATTR_TRUSTED_PREFIX_LEN)) {
-		if (!capable(CAP_SYS_ADMIN))
+		if (strncmp(name, "trusted.overlay", 15) == 0) {
+			if (!inode_capable(inode, CAP_SYS_ADMIN))
+				return (mask & MAY_WRITE) ? -EPERM : -ENODATA;
+		} else if (!capable(CAP_SYS_ADMIN))
 			return (mask & MAY_WRITE) ? -EPERM : -ENODATA;
 		return 0;
 	}
-- 
1.9.rc1

Re: [PATCH RFC] overlayfs,xattr: allow unprivileged users to whiteout

[Miklos Szeredi]

On Tue, Feb 25, 2014 at 6:31 PM, Serge Hallyn <serge.hallyn@ubuntu.com> wrote:
> To mark a file which exists in the lower layer as deleted,
> it creates a symbolic link to a file called "(overlay-whiteout)"
> in the writeable mount, and sets a "trusted.overlay" xattr
> on that link.
>
> 1. When the create the symbolic link as container root, not
> as the global root
>
> 2. Allow root in a container to edit "trusted.overlay*"
> xattrs.  Generally only global root is allowed to edit
> "trusted.*"

Shouldn't overlayfs just skip the permission checks and call
__vfs_setxattr_noperm() instead?

Thanks,
Miklos

Re: [PATCH RFC] overlayfs,xattr: allow unprivileged users to whiteout

[Andy Whitcroft]

On Fri, Feb 28, 2014 at 03:15:14PM +0100, Miklos Szeredi wrote:
> On Tue, Feb 25, 2014 at 6:31 PM, Serge Hallyn <serge.hallyn@ubuntu.com> wrote:
> > To mark a file which exists in the lower layer as deleted,
> > it creates a symbolic link to a file called "(overlay-whiteout)"
> > in the writeable mount, and sets a "trusted.overlay" xattr
> > on that link.
> >
> > 1. When the create the symbolic link as container root, not
> > as the global root
> >
> > 2. Allow root in a container to edit "trusted.overlay*"
> > xattrs.  Generally only global root is allowed to edit
> > "trusted.*"
> 
> Shouldn't overlayfs just skip the permission checks and call
> __vfs_setxattr_noperm() instead?

It does seem we should be avoiding the permissions here, as we have let
the thing be mounted we have done the permissions checks for that and for
the file access itself already.  This operation is something we definatly
want to represent in the filesystem.

-apw

Re: [PATCH RFC] overlayfs,xattr: allow unprivileged users to whiteout

[Serge Hallyn]

Quoting Andy Whitcroft (apw@canonical.com):
> On Fri, Feb 28, 2014 at 03:15:14PM +0100, Miklos Szeredi wrote:
> > On Tue, Feb 25, 2014 at 6:31 PM, Serge Hallyn <serge.hallyn@ubuntu.com> wrote:
> > > To mark a file which exists in the lower layer as deleted,
> > > it creates a symbolic link to a file called "(overlay-whiteout)"
> > > in the writeable mount, and sets a "trusted.overlay" xattr
> > > on that link.
> > >
> > > 1. When the create the symbolic link as container root, not
> > > as the global root
> > >
> > > 2. Allow root in a container to edit "trusted.overlay*"
> > > xattrs.  Generally only global root is allowed to edit
> > > "trusted.*"
> > 
> > Shouldn't overlayfs just skip the permission checks and call
> > __vfs_setxattr_noperm() instead?
> 
> It does seem we should be avoiding the permissions here, as we have let
> the thing be mounted we have done the permissions checks for that and for
> the file access itself already.  This operation is something we definatly
> want to represent in the filesystem.

D'oh.  Yeah, that looks good.   Andy, should I send a new patch, or
can you make those changes inline?

-serge

[RFC] overlayfs priviledge escalation handling under user namespaces

[Andy Whitcroft]

When mounting an overlayfs filesystem in a user namespace we encounter
issues with whiteout handling.  Specifically the local namespace root
user is not able to manipulate the "trusted." hierachy to install the
xattr marking the whiteouts.

In the preceeding discussion it was noted that we should really be avoiding
the permissions checks in these cases, as we have already validated that
the operation is permissible.

It was suggested we could use the __vfs_getxattr_noperm interfaces for
this, and I did spend some time prototyping this.  However it requires
us to expose at least two new interfaces to allow setting and getting
these without checks.  Overall it seemed excessive given that we were
already indicating our willingness to take on additional capabilities to
get these operations done.  That lead to a rethink, and the two alternate
patches which follow:

1) overlayfs: switch to the init user namespace for xattr operations:
This first patch solves the issue by dropping back into the init_user_ns
for these operations, effectivly avoiding the namespace issues.

2) overlayfs: use kernel service credentials for copy up and xattr manipulations:
This second patch takes a more radical approach.  As we are willing to
grab all and any permissions we need to make these operations succeed,
it simply grabs full kernel rights via prepare_kernel_cred(NULL).

I personally favour the latter approach as it allows us to remove a number
of essentially arbitrary capability grabs, though it could be argued that
the former patch better documents what is its we actually need.

We have done some basic testing on both and they seem to work as expected.

Comments, thoughts?

-apw

Andy Whitcroft (1):
  UBUNTU: ubuntu: overlayfs20 -- switch to the init user namespace for
    xattr operations

 fs/overlayfs/copy_up.c |  4 ++++
 fs/overlayfs/dir.c     | 23 +++++++++++++++++++----
 fs/overlayfs/readdir.c |  7 +++++++
 fs/overlayfs/super.c   |  5 ++++-
 4 files changed, 34 insertions(+), 5 deletions(-)

-- 
1.9.0

[RFC PATCH 1/1] overlayfs: switch to the init user namespace for xattr operations

[Andy Whitcroft]

We need to do xattr operations with CAP_SYS_ADMIN and as the real root
user.  Temporarily switch user namespaces to init_user_ns for those
operations.

Signed-off-by: Andy Whitcroft <apw@canonical.com>
---
 fs/overlayfs/copy_up.c |  4 ++++
 fs/overlayfs/dir.c     | 23 +++++++++++++++++++----
 fs/overlayfs/readdir.c |  7 +++++++
 fs/overlayfs/super.c   |  5 ++++-
 4 files changed, 34 insertions(+), 5 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 351c162..698d460 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -15,6 +15,7 @@
 #include <linux/security.h>
 #include <linux/uaccess.h>
 #include <linux/sched.h>
+#include <linux/user_namespace.h>
 #include "overlayfs.h"
 
 #define OVL_COPY_UP_CHUNK_SIZE (1 << 20)
@@ -284,6 +285,7 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
 	override_cred->fsgid = stat->gid;
 	/*
 	 * CAP_SYS_ADMIN for copying up extended attributes
+	 * init_user_ns for copying up extended attributes
 	 * CAP_DAC_OVERRIDE for create
 	 * CAP_FOWNER for chmod, timestamp update
 	 * CAP_FSETID for chmod
@@ -294,6 +296,8 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
 	cap_raise(override_cred->cap_effective, CAP_FOWNER);
 	cap_raise(override_cred->cap_effective, CAP_FSETID);
 	cap_raise(override_cred->cap_effective, CAP_MKNOD);
+	put_user_ns(override_cred->user_ns);
+	override_cred->user_ns = get_user_ns(&init_user_ns);
 	old_cred = override_creds(override_cred);
 
 	mutex_lock_nested(&upperdir->d_inode->i_mutex, I_MUTEX_PARENT);
diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index a209409..96baba9 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -12,6 +12,7 @@
 #include <linux/xattr.h>
 #include <linux/security.h>
 #include <linux/cred.h>
+#include <linux/user_namespace.h>
 #include "overlayfs.h"
 
 static const char *ovl_whiteout_symlink = "(overlay-whiteout)";
@@ -32,14 +33,21 @@ static int ovl_whiteout(struct dentry *upperdir, struct dentry *dentry)
 
 	/*
 	 * CAP_SYS_ADMIN for setxattr
+	 * init_user_ns for setxattr
 	 * CAP_DAC_OVERRIDE for symlink creation
 	 * CAP_FOWNER for unlink in sticky directory
 	 */
 	cap_raise(override_cred->cap_effective, CAP_SYS_ADMIN);
 	cap_raise(override_cred->cap_effective, CAP_DAC_OVERRIDE);
 	cap_raise(override_cred->cap_effective, CAP_FOWNER);
-	override_cred->fsuid = GLOBAL_ROOT_UID;
-	override_cred->fsgid = GLOBAL_ROOT_GID;
+	override_cred->fsuid = make_kuid(current_user_ns(), 0);
+	if (!uid_valid(override_cred->fsuid))
+		override_cred->fsuid = GLOBAL_ROOT_UID;
+	override_cred->fsgid = make_kgid(current_user_ns(), 0);
+	if (!gid_valid(override_cred->fsgid))
+		override_cred->fsgid = GLOBAL_ROOT_GID;
+	put_user_ns(override_cred->user_ns);
+	override_cred->user_ns = get_user_ns(&init_user_ns);
 	old_cred = override_creds(override_cred);
 
 	newdentry = lookup_one_len(dentry->d_name.name, upperdir,
@@ -109,10 +117,13 @@ static struct dentry *ovl_lookup_create(struct dentry *upperdir,
 
 		/*
 		 * CAP_SYS_ADMIN for getxattr
+		 * init_user_ns for getxattr
 		 * CAP_FOWNER for unlink in sticky directory
 		 */
 		cap_raise(override_cred->cap_effective, CAP_SYS_ADMIN);
 		cap_raise(override_cred->cap_effective, CAP_FOWNER);
+		put_user_ns(override_cred->user_ns);
+		override_cred->user_ns = get_user_ns(&init_user_ns);
 		old_cred = override_creds(override_cred);
 
 		err = -EEXIST;
@@ -209,8 +220,10 @@ static int ovl_set_opaque(struct dentry *upperdentry)
 	if (!override_cred)
 		return -ENOMEM;
 
-	/* CAP_SYS_ADMIN for setxattr of "trusted" namespace */
+	/* CAP_SYS_ADMIN, init_user_ns for setxattr of "trusted" namespace */
 	cap_raise(override_cred->cap_effective, CAP_SYS_ADMIN);
+	put_user_ns(override_cred->user_ns);
+	override_cred->user_ns = get_user_ns(&init_user_ns);
 	old_cred = override_creds(override_cred);
 	err = vfs_setxattr(upperdentry, ovl_opaque_xattr, "y", 1, 0);
 	revert_creds(old_cred);
@@ -229,8 +242,10 @@ static int ovl_remove_opaque(struct dentry *upperdentry)
 	if (!override_cred)
 		return -ENOMEM;
 
-	/* CAP_SYS_ADMIN for removexattr of "trusted" namespace */
+	/* CAP_SYS_ADMIN, init_user_ns for removexattr of "trusted" namespace */
 	cap_raise(override_cred->cap_effective, CAP_SYS_ADMIN);
+	put_user_ns(override_cred->user_ns);
+	override_cred->user_ns = get_user_ns(&init_user_ns);
 	old_cred = override_creds(override_cred);
 	err = vfs_removexattr(upperdentry, ovl_opaque_xattr);
 	revert_creds(old_cred);
diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
index 9c6f08f..5419454 100644
--- a/fs/overlayfs/readdir.c
+++ b/fs/overlayfs/readdir.c
@@ -15,6 +15,7 @@
 #include <linux/rbtree.h>
 #include <linux/security.h>
 #include <linux/cred.h>
+#include <linux/user_namespace.h>
 #include "overlayfs.h"
 
 struct ovl_cache_entry {
@@ -226,10 +227,13 @@ static int ovl_dir_mark_whiteouts(struct ovl_readdir_data *rdd)
 
 	/*
 	 * CAP_SYS_ADMIN for getxattr
+	 * init_user_ns for getxattr
 	 * CAP_DAC_OVERRIDE for lookup
 	 */
 	cap_raise(override_cred->cap_effective, CAP_SYS_ADMIN);
 	cap_raise(override_cred->cap_effective, CAP_DAC_OVERRIDE);
+	put_user_ns(override_cred->user_ns);
+	override_cred->user_ns = get_user_ns(&init_user_ns);
 	old_cred = override_creds(override_cred);
 
 	mutex_lock(&rdd->dir->d_inode->i_mutex);
@@ -510,11 +514,14 @@ static int ovl_remove_whiteouts(struct dentry *dir, struct list_head *list)
 	/*
 	 * CAP_DAC_OVERRIDE for lookup and unlink
 	 * CAP_SYS_ADMIN for setxattr of "trusted" namespace
+	 * init_user_ns for setxattr of "trusted" namespace
 	 * CAP_FOWNER for unlink in sticky directory
 	 */
 	cap_raise(override_cred->cap_effective, CAP_DAC_OVERRIDE);
 	cap_raise(override_cred->cap_effective, CAP_SYS_ADMIN);
 	cap_raise(override_cred->cap_effective, CAP_FOWNER);
+	put_user_ns(override_cred->user_ns);
+	override_cred->user_ns = get_user_ns(&init_user_ns);
 	old_cred = override_creds(override_cred);
 
 	err = vfs_setxattr(upperdir, ovl_opaque_xattr, "y", 1, 0);
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 50890c2..2889818 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -19,6 +19,7 @@
 #include <linux/sched.h>
 #include <linux/statfs.h>
 #include <linux/seq_file.h>
+#include <linux/user_namespace.h>
 #include "overlayfs.h"
 
 MODULE_AUTHOR("Miklos Szeredi <miklos@szeredi.hu>");
@@ -308,8 +309,10 @@ static int ovl_do_lookup(struct dentry *dentry)
 			if (!override_cred)
 				goto out_dput_upper;
 
-			/* CAP_SYS_ADMIN needed for getxattr */
+			/* CAP_SYS_ADMIN, init_user_ns needed for getxattr */
 			cap_raise(override_cred->cap_effective, CAP_SYS_ADMIN);
+			put_user_ns(override_cred->user_ns);
+			override_cred->user_ns = get_user_ns(&init_user_ns);
 			old_cred = override_creds(override_cred);
 
 			if (ovl_is_opaquedir(upperdentry)) {
-- 
1.9.0

[RFC PATCH 1/1] overlayfs: use kernel service credentials for copy up and xattr manipulations

[Andy Whitcroft]

We need to be priviledged to perform operations such as copy up and
xattr manipulations on "trusted.".  Use prepare_kernel_cred to obtain
the necessary priviledges; from its documentation:

   "Prepare a set of credentials for a kernel service.  This can then be
    used to override a task's own credentials so that work can be done
    on behalf of that task that requires a different subjective context."

Signed-off-by: Andy Whitcroft <apw@canonical.com>
---
 fs/overlayfs/copy_up.c | 14 +-------------
 fs/overlayfs/dir.c     | 35 +++++++++++------------------------
 fs/overlayfs/readdir.c | 18 ++----------------
 fs/overlayfs/super.c   |  4 +---
 4 files changed, 15 insertions(+), 56 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 351c162..c7894de 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -276,24 +276,12 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
 	}
 
 	err = -ENOMEM;
-	override_cred = prepare_creds();
+	override_cred = prepare_kernel_cred(NULL);
 	if (!override_cred)
 		goto out_free_link;
 
 	override_cred->fsuid = stat->uid;
 	override_cred->fsgid = stat->gid;
-	/*
-	 * CAP_SYS_ADMIN for copying up extended attributes
-	 * CAP_DAC_OVERRIDE for create
-	 * CAP_FOWNER for chmod, timestamp update
-	 * CAP_FSETID for chmod
-	 * CAP_MKNOD for mknod
-	 */
-	cap_raise(override_cred->cap_effective, CAP_SYS_ADMIN);
-	cap_raise(override_cred->cap_effective, CAP_DAC_OVERRIDE);
-	cap_raise(override_cred->cap_effective, CAP_FOWNER);
-	cap_raise(override_cred->cap_effective, CAP_FSETID);
-	cap_raise(override_cred->cap_effective, CAP_MKNOD);
 	old_cred = override_creds(override_cred);
 
 	mutex_lock_nested(&upperdir->d_inode->i_mutex, I_MUTEX_PARENT);
diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index a209409..0a7eb4a 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -11,6 +11,7 @@
 #include <linux/namei.h>
 #include <linux/xattr.h>
 #include <linux/security.h>
+#include <linux/sched.h>
 #include <linux/cred.h>
 #include "overlayfs.h"
 
@@ -26,20 +27,16 @@ static int ovl_whiteout(struct dentry *upperdir, struct dentry *dentry)
 	/* FIXME: recheck lower dentry to see if whiteout is really needed */
 
 	err = -ENOMEM;
-	override_cred = prepare_creds();
+	override_cred = prepare_kernel_cred(NULL);
 	if (!override_cred)
 		goto out;
 
-	/*
-	 * CAP_SYS_ADMIN for setxattr
-	 * CAP_DAC_OVERRIDE for symlink creation
-	 * CAP_FOWNER for unlink in sticky directory
-	 */
-	cap_raise(override_cred->cap_effective, CAP_SYS_ADMIN);
-	cap_raise(override_cred->cap_effective, CAP_DAC_OVERRIDE);
-	cap_raise(override_cred->cap_effective, CAP_FOWNER);
-	override_cred->fsuid = GLOBAL_ROOT_UID;
-	override_cred->fsgid = GLOBAL_ROOT_GID;
+	override_cred->fsuid = make_kuid(current_user_ns(), 0);
+	if (!uid_valid(override_cred->fsuid))
+		override_cred->fsuid = GLOBAL_ROOT_UID;
+	override_cred->fsgid = make_kgid(current_user_ns(), 0);
+	if (!gid_valid(override_cred->fsgid))
+		override_cred->fsgid = GLOBAL_ROOT_GID;
 	old_cred = override_creds(override_cred);
 
 	newdentry = lookup_one_len(dentry->d_name.name, upperdir,
@@ -103,16 +100,10 @@ static struct dentry *ovl_lookup_create(struct dentry *upperdir,
 			goto out_dput;
 
 		err = -ENOMEM;
-		override_cred = prepare_creds();
+		override_cred = prepare_kernel_cred(NULL);
 		if (!override_cred)
 			goto out_dput;
 
-		/*
-		 * CAP_SYS_ADMIN for getxattr
-		 * CAP_FOWNER for unlink in sticky directory
-		 */
-		cap_raise(override_cred->cap_effective, CAP_SYS_ADMIN);
-		cap_raise(override_cred->cap_effective, CAP_FOWNER);
 		old_cred = override_creds(override_cred);
 
 		err = -EEXIST;
@@ -205,12 +196,10 @@ static int ovl_set_opaque(struct dentry *upperdentry)
 	const struct cred *old_cred;
 	struct cred *override_cred;
 
-	override_cred = prepare_creds();
+	override_cred = prepare_kernel_cred(NULL);
 	if (!override_cred)
 		return -ENOMEM;
 
-	/* CAP_SYS_ADMIN for setxattr of "trusted" namespace */
-	cap_raise(override_cred->cap_effective, CAP_SYS_ADMIN);
 	old_cred = override_creds(override_cred);
 	err = vfs_setxattr(upperdentry, ovl_opaque_xattr, "y", 1, 0);
 	revert_creds(old_cred);
@@ -225,12 +214,10 @@ static int ovl_remove_opaque(struct dentry *upperdentry)
 	const struct cred *old_cred;
 	struct cred *override_cred;
 
-	override_cred = prepare_creds();
+	override_cred = prepare_kernel_cred(NULL);
 	if (!override_cred)
 		return -ENOMEM;
 
-	/* CAP_SYS_ADMIN for removexattr of "trusted" namespace */
-	cap_raise(override_cred->cap_effective, CAP_SYS_ADMIN);
 	old_cred = override_creds(override_cred);
 	err = vfs_removexattr(upperdentry, ovl_opaque_xattr);
 	revert_creds(old_cred);
diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
index 9c6f08f..1cc4cbd 100644
--- a/fs/overlayfs/readdir.c
+++ b/fs/overlayfs/readdir.c
@@ -218,18 +218,12 @@ static int ovl_dir_mark_whiteouts(struct ovl_readdir_data *rdd)
 	const struct cred *old_cred;
 	struct cred *override_cred;
 
-	override_cred = prepare_creds();
+	override_cred = prepare_kernel_cred(NULL);
 	if (!override_cred) {
 		ovl_cache_free(rdd->list);
 		return -ENOMEM;
 	}
 
-	/*
-	 * CAP_SYS_ADMIN for getxattr
-	 * CAP_DAC_OVERRIDE for lookup
-	 */
-	cap_raise(override_cred->cap_effective, CAP_SYS_ADMIN);
-	cap_raise(override_cred->cap_effective, CAP_DAC_OVERRIDE);
 	old_cred = override_creds(override_cred);
 
 	mutex_lock(&rdd->dir->d_inode->i_mutex);
@@ -503,18 +497,10 @@ static int ovl_remove_whiteouts(struct dentry *dir, struct list_head *list)
 	ovl_path_upper(dir, &upperpath);
 	upperdir = upperpath.dentry;
 
-	override_cred = prepare_creds();
+	override_cred = prepare_kernel_cred(NULL);
 	if (!override_cred)
 		return -ENOMEM;
 
-	/*
-	 * CAP_DAC_OVERRIDE for lookup and unlink
-	 * CAP_SYS_ADMIN for setxattr of "trusted" namespace
-	 * CAP_FOWNER for unlink in sticky directory
-	 */
-	cap_raise(override_cred->cap_effective, CAP_DAC_OVERRIDE);
-	cap_raise(override_cred->cap_effective, CAP_SYS_ADMIN);
-	cap_raise(override_cred->cap_effective, CAP_FOWNER);
 	old_cred = override_creds(override_cred);
 
 	err = vfs_setxattr(upperdir, ovl_opaque_xattr, "y", 1, 0);
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 50890c2..79288a8 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -304,12 +304,10 @@ static int ovl_do_lookup(struct dentry *dentry)
 			struct cred *override_cred;
 
 			err = -ENOMEM;
-			override_cred = prepare_creds();
+			override_cred = prepare_kernel_cred(NULL);
 			if (!override_cred)
 				goto out_dput_upper;
 
-			/* CAP_SYS_ADMIN needed for getxattr */
-			cap_raise(override_cred->cap_effective, CAP_SYS_ADMIN);
 			old_cred = override_creds(override_cred);
 
 			if (ovl_is_opaquedir(upperdentry)) {
-- 
1.9.0

Re: [RFC PATCH 1/1] overlayfs: use kernel service credentials for copy up and xattr manipulations

[Serge Hallyn]

Quoting Andy Whitcroft (apw@canonical.com):
> We need to be priviledged to perform operations such as copy up and
> xattr manipulations on "trusted.".  Use prepare_kernel_cred to obtain
> the necessary priviledges; from its documentation:
> 
>    "Prepare a set of credentials for a kernel service.  This can then be
>     used to override a task's own credentials so that work can be done
>     on behalf of that task that requires a different subjective context."
> 
> Signed-off-by: Andy Whitcroft <apw@canonical.com>

Thanks, Andy.  Both of these worked, and both look sane.  I prefer
this patch, personally, but my test+ack applies to either.

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

> ---
>  fs/overlayfs/copy_up.c | 14 +-------------
>  fs/overlayfs/dir.c     | 35 +++++++++++------------------------
>  fs/overlayfs/readdir.c | 18 ++----------------
>  fs/overlayfs/super.c   |  4 +---
>  4 files changed, 15 insertions(+), 56 deletions(-)
> 
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index 351c162..c7894de 100644
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -276,24 +276,12 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
>  	}
>  
>  	err = -ENOMEM;
> -	override_cred = prepare_creds();
> +	override_cred = prepare_kernel_cred(NULL);
>  	if (!override_cred)
>  		goto out_free_link;
>  
>  	override_cred->fsuid = stat->uid;
>  	override_cred->fsgid = stat->gid;
> -	/*
> -	 * CAP_SYS_ADMIN for copying up extended attributes
> -	 * CAP_DAC_OVERRIDE for create
> -	 * CAP_FOWNER for chmod, timestamp update
> -	 * CAP_FSETID for chmod
> -	 * CAP_MKNOD for mknod
> -	 */
> -	cap_raise(override_cred->cap_effective, CAP_SYS_ADMIN);
> -	cap_raise(override_cred->cap_effective, CAP_DAC_OVERRIDE);
> -	cap_raise(override_cred->cap_effective, CAP_FOWNER);
> -	cap_raise(override_cred->cap_effective, CAP_FSETID);
> -	cap_raise(override_cred->cap_effective, CAP_MKNOD);
>  	old_cred = override_creds(override_cred);
>  
>  	mutex_lock_nested(&upperdir->d_inode->i_mutex, I_MUTEX_PARENT);
> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> index a209409..0a7eb4a 100644
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -11,6 +11,7 @@
>  #include <linux/namei.h>
>  #include <linux/xattr.h>
>  #include <linux/security.h>
> +#include <linux/sched.h>
>  #include <linux/cred.h>
>  #include "overlayfs.h"
>  
> @@ -26,20 +27,16 @@ static int ovl_whiteout(struct dentry *upperdir, struct dentry *dentry)
>  	/* FIXME: recheck lower dentry to see if whiteout is really needed */
>  
>  	err = -ENOMEM;
> -	override_cred = prepare_creds();
> +	override_cred = prepare_kernel_cred(NULL);
>  	if (!override_cred)
>  		goto out;
>  
> -	/*
> -	 * CAP_SYS_ADMIN for setxattr
> -	 * CAP_DAC_OVERRIDE for symlink creation
> -	 * CAP_FOWNER for unlink in sticky directory
> -	 */
> -	cap_raise(override_cred->cap_effective, CAP_SYS_ADMIN);
> -	cap_raise(override_cred->cap_effective, CAP_DAC_OVERRIDE);
> -	cap_raise(override_cred->cap_effective, CAP_FOWNER);
> -	override_cred->fsuid = GLOBAL_ROOT_UID;
> -	override_cred->fsgid = GLOBAL_ROOT_GID;
> +	override_cred->fsuid = make_kuid(current_user_ns(), 0);
> +	if (!uid_valid(override_cred->fsuid))
> +		override_cred->fsuid = GLOBAL_ROOT_UID;
> +	override_cred->fsgid = make_kgid(current_user_ns(), 0);
> +	if (!gid_valid(override_cred->fsgid))
> +		override_cred->fsgid = GLOBAL_ROOT_GID;
>  	old_cred = override_creds(override_cred);
>  
>  	newdentry = lookup_one_len(dentry->d_name.name, upperdir,
> @@ -103,16 +100,10 @@ static struct dentry *ovl_lookup_create(struct dentry *upperdir,
>  			goto out_dput;
>  
>  		err = -ENOMEM;
> -		override_cred = prepare_creds();
> +		override_cred = prepare_kernel_cred(NULL);
>  		if (!override_cred)
>  			goto out_dput;
>  
> -		/*
> -		 * CAP_SYS_ADMIN for getxattr
> -		 * CAP_FOWNER for unlink in sticky directory
> -		 */
> -		cap_raise(override_cred->cap_effective, CAP_SYS_ADMIN);
> -		cap_raise(override_cred->cap_effective, CAP_FOWNER);
>  		old_cred = override_creds(override_cred);
>  
>  		err = -EEXIST;
> @@ -205,12 +196,10 @@ static int ovl_set_opaque(struct dentry *upperdentry)
>  	const struct cred *old_cred;
>  	struct cred *override_cred;
>  
> -	override_cred = prepare_creds();
> +	override_cred = prepare_kernel_cred(NULL);
>  	if (!override_cred)
>  		return -ENOMEM;
>  
> -	/* CAP_SYS_ADMIN for setxattr of "trusted" namespace */
> -	cap_raise(override_cred->cap_effective, CAP_SYS_ADMIN);
>  	old_cred = override_creds(override_cred);
>  	err = vfs_setxattr(upperdentry, ovl_opaque_xattr, "y", 1, 0);
>  	revert_creds(old_cred);
> @@ -225,12 +214,10 @@ static int ovl_remove_opaque(struct dentry *upperdentry)
>  	const struct cred *old_cred;
>  	struct cred *override_cred;
>  
> -	override_cred = prepare_creds();
> +	override_cred = prepare_kernel_cred(NULL);
>  	if (!override_cred)
>  		return -ENOMEM;
>  
> -	/* CAP_SYS_ADMIN for removexattr of "trusted" namespace */
> -	cap_raise(override_cred->cap_effective, CAP_SYS_ADMIN);
>  	old_cred = override_creds(override_cred);
>  	err = vfs_removexattr(upperdentry, ovl_opaque_xattr);
>  	revert_creds(old_cred);
> diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
> index 9c6f08f..1cc4cbd 100644
> --- a/fs/overlayfs/readdir.c
> +++ b/fs/overlayfs/readdir.c
> @@ -218,18 +218,12 @@ static int ovl_dir_mark_whiteouts(struct ovl_readdir_data *rdd)
>  	const struct cred *old_cred;
>  	struct cred *override_cred;
>  
> -	override_cred = prepare_creds();
> +	override_cred = prepare_kernel_cred(NULL);
>  	if (!override_cred) {
>  		ovl_cache_free(rdd->list);
>  		return -ENOMEM;
>  	}
>  
> -	/*
> -	 * CAP_SYS_ADMIN for getxattr
> -	 * CAP_DAC_OVERRIDE for lookup
> -	 */
> -	cap_raise(override_cred->cap_effective, CAP_SYS_ADMIN);
> -	cap_raise(override_cred->cap_effective, CAP_DAC_OVERRIDE);
>  	old_cred = override_creds(override_cred);
>  
>  	mutex_lock(&rdd->dir->d_inode->i_mutex);
> @@ -503,18 +497,10 @@ static int ovl_remove_whiteouts(struct dentry *dir, struct list_head *list)
>  	ovl_path_upper(dir, &upperpath);
>  	upperdir = upperpath.dentry;
>  
> -	override_cred = prepare_creds();
> +	override_cred = prepare_kernel_cred(NULL);
>  	if (!override_cred)
>  		return -ENOMEM;
>  
> -	/*
> -	 * CAP_DAC_OVERRIDE for lookup and unlink
> -	 * CAP_SYS_ADMIN for setxattr of "trusted" namespace
> -	 * CAP_FOWNER for unlink in sticky directory
> -	 */
> -	cap_raise(override_cred->cap_effective, CAP_DAC_OVERRIDE);
> -	cap_raise(override_cred->cap_effective, CAP_SYS_ADMIN);
> -	cap_raise(override_cred->cap_effective, CAP_FOWNER);
>  	old_cred = override_creds(override_cred);
>  
>  	err = vfs_setxattr(upperdir, ovl_opaque_xattr, "y", 1, 0);
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index 50890c2..79288a8 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -304,12 +304,10 @@ static int ovl_do_lookup(struct dentry *dentry)
>  			struct cred *override_cred;
>  
>  			err = -ENOMEM;
> -			override_cred = prepare_creds();
> +			override_cred = prepare_kernel_cred(NULL);
>  			if (!override_cred)
>  				goto out_dput_upper;
>  
> -			/* CAP_SYS_ADMIN needed for getxattr */
> -			cap_raise(override_cred->cap_effective, CAP_SYS_ADMIN);
>  			old_cred = override_creds(override_cred);
>  
>  			if (ovl_is_opaquedir(upperdentry)) {
> -- 
> 1.9.0