linux-kernel - Re: [RFC 1/1] shiftfs: uid/gid shifting bind mount

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <1463655234.2825.3.camel@HansenPartnership.com>
Date:	Thu, 19 May 2016 06:53:54 -0400
From:	James Bottomley <James.Bottomley@...senPartnership.com>
To:	"Serge E. Hallyn" <serge@...lyn.com>
Cc:	Serge Hallyn <serge.hallyn@...ntu.com>,
	Djalal Harouni <tixxdz@...il.com>, Chris Mason <clm@...com>,
	tytso@....edu, Serge Hallyn <serge.hallyn@...onical.com>,
	Josh Triplett <josh@...htriplett.org>,
	"Eric W. Biederman" <ebiederm@...ssion.com>,
	Andy Lutomirski <luto@...nel.org>,
	Seth Forshee <seth.forshee@...onical.com>,
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-security-module@...r.kernel.org,
	Dongsu Park <dongsu@...ocode.com>,
	David Herrmann <dh.herrmann@...glemail.com>,
	Miklos Szeredi <mszeredi@...hat.com>,
	Alban Crequy <alban.crequy@...il.com>,
	Al Viro <viro@...IV.linux.org.uk>
Subject: Re: [RFC 1/1] shiftfs: uid/gid shifting bind mount

On Wed, 2016-05-18 at 21:28 -0500, Serge E. Hallyn wrote:
> Hey James,
> 
> yeah that's a lot better.  I do still get some syslog messages,
> but i was trivially able to bind a shiftfs into a container and
> use it the way I'd want.
> 
> [  209.452274] ------------[ cut here ]------------
> [  209.452296] WARNING: CPU: 0 PID: 3072 at fs/ext4/inode.c:3977 
> ext4_truncate+0x3f5/0x5b0

Heh, I really need to test with ext4; it seems much more careful.  XFS
doesn't warn on any of this.  These are both inode locking problems
with setattr.  It also looks like I'd have the same problem with
setxattr and removexattr.  Does this additional patch allow you to
operate without any warnings?

There's also something else you'll be running into soon: the xattr
calls aren't uid shifted.  I was a bit worried about how to do this
without leaking root attribute setting capability, but I'll think a bit
more carefully about how to do it.

Thanks,

James

---

diff --git a/fs/shiftfs.c b/fs/shiftfs.c
index d352377..29f343f 100644
--- a/fs/shiftfs.c
+++ b/fs/shiftfs.c
@@ -240,14 +240,17 @@ static int shiftfs_setxattr(struct dentry *dentry, const char *name,
 			    const void *value, size_t size, int flags)
 {
 	struct dentry *real = dentry->d_fsdata;
-	const struct inode_operations *iop = real->d_inode->i_op;
+	struct inode *reali = real->d_inode;
+	const struct inode_operations *iop = reali->i_op;
 	int err = -EOPNOTSUPP;
 
 	if (iop->setxattr) {
 		const struct cred *oldcred, *newcred;
 
 		oldcred = shiftfs_new_creds(&newcred, dentry->d_sb);
+		inode_lock(reali);
 		err = iop->setxattr(real, name, value, size, flags);
+		inode_unlock(reali);
 		shiftfs_old_creds(oldcred, &newcred);
 	}
 
@@ -287,12 +290,17 @@ static ssize_t shiftfs_listxattr(struct dentry *dentry, char *list,
 static int shiftfs_removexattr(struct dentry *dentry, const char *name)
 {
 	struct dentry *real = dentry->d_fsdata;
-	const struct inode_operations *iop = real->d_inode->i_op;
+	struct inode *reali = real->d_inode;
+	const struct inode_operations *iop = reali->i_op;
+	int err = -EINVAL;
 
-	if (iop->removexattr)
-		return iop->removexattr(real, name);
+	if (iop->removexattr) {
+		inode_lock(reali);
+		err = iop->removexattr(real, name);
+		inode_unlock(reali);
+	}
 
-	return -EINVAL;
+	return err;
 }
 
 static void shiftfs_fill_inode(struct inode *inode, struct dentry *dentry)
@@ -548,11 +556,13 @@ static int shiftfs_setattr(struct dentry *dentry, struct iattr *attr)
 	newattr.ia_uid = KUIDT_INIT(map_id_up(&ssi->uid_map, __kuid_val(attr->ia_uid)));
 	newattr.ia_gid = KGIDT_INIT(map_id_up(&ssi->gid_map, __kgid_val(attr->ia_gid)));
 
+	inode_lock(reali);
 	oldcred = shiftfs_new_creds(&newcred, dentry->d_sb);
 	if (iop->setattr)
 		err = iop->setattr(real, &newattr);
 	else
 		err = simple_setattr(real, &newattr);
+	inode_unlock(reali);
 	shiftfs_old_creds(oldcred, &newcred);
 
 	return err;