[PATCH 0/8] Security: Provide unioned file support

[PATCH 0/8] Security: Provide unioned file support

[David Howells]

The attached patches provide security support for unioned files where the
security involves an object-label-based LSM (such as SELinux) rather than a
path-based LSM.

The patches can be broken down into a number of sets:

 (1) A small patch to drop a lock earlier in overlayfs.  The main VFS patch
     touches the same code, so I put this first.

 (2) The main VFS patch that makes an open file struct referring to a union
     file have ->f_path point to the union/overlay file whilst ->f_inode and
     ->f_mapping refer to the subordinate file that does the actual work.

 (3) LSM hooks to handle copy up of a file, including label setting and xattr
     filtration and SELinux implementations of these hooks.

 (4) LSM hooks to handle file open and file permission checking for the
     instance where a union/overlay file is opened that actually falls through
     to a subordinate file (ie. as (2) above) and the SELinux implementation.

 (5) An SELinux patch to make a common helper for several functions that need
     to determine the label for an inode.

The first two patches can be found here:

	http://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git/log/?h=for-viro

And all the patches here:

	http://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git/log/?h=overlayfs

Tagged with overlay-pin-20150618.

This is based on part of Al Viro's vfs/for-next branch.  However, the security
bits will need to go through the security tree - but after first two patches
are taken through the VFS tree.

David
---
David Howells (8):
      overlay: Call ovl_drop_write() earlier in ovl_dentry_open()
      overlayfs: Make f_path always point to the overlay and f_inode to the underlay
      Security: Provide copy-up security hooks for unioned files
      Overlayfs: Use copy-up security hooks
      SELinux: Stub in copy-up handling
      SELinux: Handle opening of a unioned file
      SELinux: Create a common helper to determine an inode label
      SELinux: Check against union label for file operations


 fs/dcache.c                       |    5 +
 fs/internal.h                     |    1 
 fs/open.c                         |   49 +++++-----
 fs/overlayfs/copy_up.c            |   12 ++
 fs/overlayfs/inode.c              |   22 +---
 fs/overlayfs/overlayfs.h          |    1 
 fs/overlayfs/super.c              |    1 
 include/linux/dcache.h            |    2 
 include/linux/fs.h                |    2 
 include/linux/security.h          |   36 +++++++
 security/capability.c             |   13 +++
 security/security.c               |   13 +++
 security/selinux/hooks.c          |  185 +++++++++++++++++++++++++++----------
 security/selinux/include/objsec.h |    1 
 14 files changed, 254 insertions(+), 89 deletions(-)

[PATCH 1/8] overlay: Call ovl_drop_write() earlier in ovl_dentry_open()

[David Howells]

Call ovl_drop_write() earlier in ovl_dentry_open() before we call vfs_open()
as we've done the copy up for which we needed the freeze-write lock by that
point.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 fs/overlayfs/inode.c |   14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index 308379b2d0b2..21079d1ca2aa 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -343,31 +343,25 @@ static int ovl_dentry_open(struct dentry *dentry, struct file *file,
 	int err;
 	struct path realpath;
 	enum ovl_path_type type;
-	bool want_write = false;
 
 	type = ovl_path_real(dentry, &realpath);
 	if (ovl_open_need_copy_up(file->f_flags, type, realpath.dentry)) {
-		want_write = true;
 		err = ovl_want_write(dentry);
 		if (err)
-			goto out;
+			return err;
 
 		if (file->f_flags & O_TRUNC)
 			err = ovl_copy_up_last(dentry, NULL, true);
 		else
 			err = ovl_copy_up(dentry);
+		ovl_drop_write(dentry);
 		if (err)
-			goto out_drop_write;
+			return err;
 
 		ovl_path_upper(dentry, &realpath);
 	}
 
-	err = vfs_open(&realpath, file, cred);
-out_drop_write:
-	if (want_write)
-		ovl_drop_write(dentry);
-out:
-	return err;
+	return vfs_open(&realpath, file, cred);
 }
 
 static const struct inode_operations ovl_file_inode_operations = {

[PATCH 2/8] overlayfs: Make f_path always point to the overlay and f_inode to the underlay

[David Howells]

Make file->f_path always point to the overlay dentry so that the path in
/proc/pid/fd is correct and to ensure that label-based LSMs have access to the
overlay as well as the underlay (path-based LSMs probably don't need it).

Using my union testsuite to set things up, before the patch I see:

	[root@andromeda union-testsuite]# bash 5</mnt/a/foo107
	[root@andromeda union-testsuite]# ls -l /proc/$$/fd/
	...
	lr-x------. 1 root root 64 Jun  5 14:38 5 -> /a/foo107
	[root@andromeda union-testsuite]# stat /mnt/a/foo107
	...
	Device: 23h/35d Inode: 13381       Links: 1
	...
	[root@andromeda union-testsuite]# stat -L /proc/$$/fd/5
	...
	Device: 23h/35d Inode: 13381       Links: 1
	...

After the patch:

	[root@andromeda union-testsuite]# bash 5</mnt/a/foo107
	[root@andromeda union-testsuite]# ls -l /proc/$$/fd/
	...
	lr-x------. 1 root root 64 Jun  5 14:22 5 -> /mnt/a/foo107
	[root@andromeda union-testsuite]# stat /mnt/a/foo107
	...
	Device: 23h/35d Inode: 40346       Links: 1
	...
	[root@andromeda union-testsuite]# stat -L /proc/$$/fd/5
	...
	Device: 23h/35d Inode: 40346       Links: 1
	...

Note the change in where /proc/$$/fd/5 points to in the ls command.  It was
pointing to /a/foo107 (which doesn't exist) and now points to /mnt/a/foo107
(which is correct).

The inode accessed, however, is the lower layer.  The union layer is on device
25h/37d and the upper layer on 24h/36d.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 fs/dcache.c              |    5 ++++-
 fs/internal.h            |    1 +
 fs/open.c                |   49 ++++++++++++++++++++++++----------------------
 fs/overlayfs/inode.c     |   14 ++++++-------
 fs/overlayfs/overlayfs.h |    1 +
 fs/overlayfs/super.c     |    1 +
 include/linux/dcache.h   |    2 ++
 include/linux/fs.h       |    2 --
 8 files changed, 41 insertions(+), 34 deletions(-)

diff --git a/fs/dcache.c b/fs/dcache.c
index 37b5afdaf698..c4ce35110704 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -1673,7 +1673,8 @@ void d_set_d_op(struct dentry *dentry, const struct dentry_operations *op)
 				DCACHE_OP_COMPARE	|
 				DCACHE_OP_REVALIDATE	|
 				DCACHE_OP_WEAK_REVALIDATE	|
-				DCACHE_OP_DELETE ));
+				DCACHE_OP_DELETE	|
+				DCACHE_OP_SELECT_INODE));
 	dentry->d_op = op;
 	if (!op)
 		return;
@@ -1689,6 +1690,8 @@ void d_set_d_op(struct dentry *dentry, const struct dentry_operations *op)
 		dentry->d_flags |= DCACHE_OP_DELETE;
 	if (op->d_prune)
 		dentry->d_flags |= DCACHE_OP_PRUNE;
+	if (op->d_select_inode)
+		dentry->d_flags |= DCACHE_OP_SELECT_INODE;
 
 }
 EXPORT_SYMBOL(d_set_d_op);
diff --git a/fs/internal.h b/fs/internal.h
index 01dce1d1476b..4d5af583ab03 100644
--- a/fs/internal.h
+++ b/fs/internal.h
@@ -107,6 +107,7 @@ extern struct file *do_file_open_root(struct dentry *, struct vfsmount *,
 extern long do_handle_open(int mountdirfd,
 			   struct file_handle __user *ufh, int open_flag);
 extern int open_check_o_direct(struct file *f);
+extern int vfs_open(const struct path *, struct file *, const struct cred *);
 
 /*
  * inode.c
diff --git a/fs/open.c b/fs/open.c
index e0250bdcc440..b1c5823b7f11 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -678,18 +678,18 @@ int open_check_o_direct(struct file *f)
 }
 
 static int do_dentry_open(struct file *f,
+			  struct inode *inode,
 			  int (*open)(struct inode *, struct file *),
 			  const struct cred *cred)
 {
 	static const struct file_operations empty_fops = {};
-	struct inode *inode;
 	int error;
 
 	f->f_mode = OPEN_FMODE(f->f_flags) | FMODE_LSEEK |
 				FMODE_PREAD | FMODE_PWRITE;
 
 	path_get(&f->f_path);
-	inode = f->f_inode = f->f_path.dentry->d_inode;
+	f->f_inode = inode;
 	f->f_mapping = inode->i_mapping;
 
 	if (unlikely(f->f_flags & O_PATH)) {
@@ -793,7 +793,8 @@ int finish_open(struct file *file, struct dentry *dentry,
 	BUG_ON(*opened & FILE_OPENED); /* once it's opened, it's opened */
 
 	file->f_path.dentry = dentry;
-	error = do_dentry_open(file, open, current_cred());
+	error = do_dentry_open(file, d_backing_inode(dentry), open,
+			       current_cred());
 	if (!error)
 		*opened |= FILE_OPENED;
 
@@ -822,6 +823,28 @@ int finish_no_open(struct file *file, struct dentry *dentry)
 }
 EXPORT_SYMBOL(finish_no_open);
 
+/**
+ * vfs_open - open the file at the given path
+ * @path: path to open
+ * @file: newly allocated file with f_flag initialized
+ * @cred: credentials to use
+ */
+int vfs_open(const struct path *path, struct file *file,
+	     const struct cred *cred)
+{
+	struct dentry *dentry = path->dentry;
+	struct inode *inode = dentry->d_inode;
+
+	file->f_path = *path;
+	if (dentry->d_flags & DCACHE_OP_SELECT_INODE) {
+		inode = dentry->d_op->d_select_inode(dentry, file->f_flags);
+		if (IS_ERR(inode))
+			return PTR_ERR(inode);
+	}
+
+	return do_dentry_open(file, inode, NULL, cred);
+}
+
 struct file *dentry_open(const struct path *path, int flags,
 			 const struct cred *cred)
 {
@@ -853,26 +876,6 @@ struct file *dentry_open(const struct path *path, int flags,
 }
 EXPORT_SYMBOL(dentry_open);
 
-/**
- * vfs_open - open the file at the given path
- * @path: path to open
- * @filp: newly allocated file with f_flag initialized
- * @cred: credentials to use
- */
-int vfs_open(const struct path *path, struct file *filp,
-	     const struct cred *cred)
-{
-	struct inode *inode = path->dentry->d_inode;
-
-	if (inode->i_op->dentry_open)
-		return inode->i_op->dentry_open(path->dentry, filp, cred);
-	else {
-		filp->f_path = *path;
-		return do_dentry_open(filp, NULL, cred);
-	}
-}
-EXPORT_SYMBOL(vfs_open);
-
 static inline int build_open_flags(int flags, umode_t mode, struct open_flags *op)
 {
 	int lookup_flags = 0;
diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index 21079d1ca2aa..f140e3dbfb7b 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -337,31 +337,30 @@ static bool ovl_open_need_copy_up(int flags, enum ovl_path_type type,
 	return true;
 }
 
-static int ovl_dentry_open(struct dentry *dentry, struct file *file,
-		    const struct cred *cred)
+struct inode *ovl_d_select_inode(struct dentry *dentry, unsigned file_flags)
 {
 	int err;
 	struct path realpath;
 	enum ovl_path_type type;
 
 	type = ovl_path_real(dentry, &realpath);
-	if (ovl_open_need_copy_up(file->f_flags, type, realpath.dentry)) {
+	if (ovl_open_need_copy_up(file_flags, type, realpath.dentry)) {
 		err = ovl_want_write(dentry);
 		if (err)
-			return err;
+			return ERR_PTR(err);
 
-		if (file->f_flags & O_TRUNC)
+		if (file_flags & O_TRUNC)
 			err = ovl_copy_up_last(dentry, NULL, true);
 		else
 			err = ovl_copy_up(dentry);
 		ovl_drop_write(dentry);
 		if (err)
-			return err;
+			return ERR_PTR(err);
 
 		ovl_path_upper(dentry, &realpath);
 	}
 
-	return vfs_open(&realpath, file, cred);
+	return d_backing_inode(realpath.dentry);
 }
 
 static const struct inode_operations ovl_file_inode_operations = {
@@ -372,7 +371,6 @@ static const struct inode_operations ovl_file_inode_operations = {
 	.getxattr	= ovl_getxattr,
 	.listxattr	= ovl_listxattr,
 	.removexattr	= ovl_removexattr,
-	.dentry_open	= ovl_dentry_open,
 };
 
 static const struct inode_operations ovl_symlink_inode_operations = {
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 17ac5afc9ffb..ea5a40b06e3a 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -173,6 +173,7 @@ ssize_t ovl_getxattr(struct dentry *dentry, const char *name,
 		     void *value, size_t size);
 ssize_t ovl_listxattr(struct dentry *dentry, char *list, size_t size);
 int ovl_removexattr(struct dentry *dentry, const char *name);
+struct inode *ovl_d_select_inode(struct dentry *dentry, unsigned file_flags);
 
 struct inode *ovl_new_inode(struct super_block *sb, umode_t mode,
 			    struct ovl_entry *oe);
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 5f0d1993e6e3..84c5e27fbfd9 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -275,6 +275,7 @@ static void ovl_dentry_release(struct dentry *dentry)
 
 static const struct dentry_operations ovl_dentry_operations = {
 	.d_release = ovl_dentry_release,
+	.d_select_inode = ovl_d_select_inode,
 };
 
 static struct ovl_entry *ovl_alloc_entry(unsigned int numlower)
diff --git a/include/linux/dcache.h b/include/linux/dcache.h
index df334cbacc6d..167ec0934049 100644
--- a/include/linux/dcache.h
+++ b/include/linux/dcache.h
@@ -160,6 +160,7 @@ struct dentry_operations {
 	char *(*d_dname)(struct dentry *, char *, int);
 	struct vfsmount *(*d_automount)(struct path *);
 	int (*d_manage)(struct dentry *, bool);
+	struct inode *(*d_select_inode)(struct dentry *, unsigned);
 } ____cacheline_aligned;
 
 /*
@@ -225,6 +226,7 @@ struct dentry_operations {
 
 #define DCACHE_MAY_FREE			0x00800000
 #define DCACHE_FALLTHRU			0x01000000 /* Fall through to lower layer */
+#define DCACHE_OP_SELECT_INODE		0x02000000 /* Unioned entry: dcache op selects inode */
 
 extern seqlock_t rename_lock;
 
diff --git a/include/linux/fs.h b/include/linux/fs.h
index b577e801b4af..2bd77e10e8e5 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1641,7 +1641,6 @@ struct inode_operations {
 	int (*set_acl)(struct inode *, struct posix_acl *, int);
 
 	/* WARNING: probably going away soon, do not use! */
-	int (*dentry_open)(struct dentry *, struct file *, const struct cred *);
 } ____cacheline_aligned;
 
 ssize_t rw_copy_check_uvector(int type, const struct iovec __user * uvector,
@@ -2194,7 +2193,6 @@ extern struct file *file_open_name(struct filename *, int, umode_t);
 extern struct file *filp_open(const char *, int, umode_t);
 extern struct file *file_open_root(struct dentry *, struct vfsmount *,
 				   const char *, int);
-extern int vfs_open(const struct path *, struct file *, const struct cred *);
 extern struct file * dentry_open(const struct path *, int, const struct cred *);
 extern int filp_close(struct file *, fl_owner_t id);
 

Re: [PATCH 2/8] overlayfs: Make f_path always point to the overlay and f_inode to the underlay

[Konstantin Khlebnikov]

On 18.06.2015 16:32, David Howells wrote:
> Make file->f_path always point to the overlay dentry so that the path in
> /proc/pid/fd is correct and to ensure that label-based LSMs have access to the
> overlay as well as the underlay (path-based LSMs probably don't need it).

Cool. Looks like this fixes MNT_NOEXEC,MNT_NOSUID,MNT_NODEV for
overlayfs. But probably breaks MNT_LOCK_NOEXEC/SUID/DEV.

>
> Using my union testsuite to set things up, before the patch I see:
>
> 	[root@andromeda union-testsuite]# bash 5</mnt/a/foo107
> 	[root@andromeda union-testsuite]# ls -l /proc/$$/fd/
> 	...
> 	lr-x------. 1 root root 64 Jun  5 14:38 5 -> /a/foo107
> 	[root@andromeda union-testsuite]# stat /mnt/a/foo107
> 	...
> 	Device: 23h/35d Inode: 13381       Links: 1
> 	...
> 	[root@andromeda union-testsuite]# stat -L /proc/$$/fd/5
> 	...
> 	Device: 23h/35d Inode: 13381       Links: 1
> 	...
>
> After the patch:
>
> 	[root@andromeda union-testsuite]# bash 5</mnt/a/foo107
> 	[root@andromeda union-testsuite]# ls -l /proc/$$/fd/
> 	...
> 	lr-x------. 1 root root 64 Jun  5 14:22 5 -> /mnt/a/foo107
> 	[root@andromeda union-testsuite]# stat /mnt/a/foo107
> 	...
> 	Device: 23h/35d Inode: 40346       Links: 1
> 	...
> 	[root@andromeda union-testsuite]# stat -L /proc/$$/fd/5
> 	...
> 	Device: 23h/35d Inode: 40346       Links: 1
> 	...
>
> Note the change in where /proc/$$/fd/5 points to in the ls command.  It was
> pointing to /a/foo107 (which doesn't exist) and now points to /mnt/a/foo107
> (which is correct).
>
> The inode accessed, however, is the lower layer.  The union layer is on device
> 25h/37d and the upper layer on 24h/36d.
>
> Signed-off-by: David Howells <dhowells@redhat.com>
> ---
>
>   fs/dcache.c              |    5 ++++-
>   fs/internal.h            |    1 +
>   fs/open.c                |   49 ++++++++++++++++++++++++----------------------
>   fs/overlayfs/inode.c     |   14 ++++++-------
>   fs/overlayfs/overlayfs.h |    1 +
>   fs/overlayfs/super.c     |    1 +
>   include/linux/dcache.h   |    2 ++
>   include/linux/fs.h       |    2 --
>   8 files changed, 41 insertions(+), 34 deletions(-)
>
> diff --git a/fs/dcache.c b/fs/dcache.c
> index 37b5afdaf698..c4ce35110704 100644
> --- a/fs/dcache.c
> +++ b/fs/dcache.c
> @@ -1673,7 +1673,8 @@ void d_set_d_op(struct dentry *dentry, const struct dentry_operations *op)
>   				DCACHE_OP_COMPARE	|
>   				DCACHE_OP_REVALIDATE	|
>   				DCACHE_OP_WEAK_REVALIDATE	|
> -				DCACHE_OP_DELETE ));
> +				DCACHE_OP_DELETE	|
> +				DCACHE_OP_SELECT_INODE));
>   	dentry->d_op = op;
>   	if (!op)
>   		return;
> @@ -1689,6 +1690,8 @@ void d_set_d_op(struct dentry *dentry, const struct dentry_operations *op)
>   		dentry->d_flags |= DCACHE_OP_DELETE;
>   	if (op->d_prune)
>   		dentry->d_flags |= DCACHE_OP_PRUNE;
> +	if (op->d_select_inode)
> +		dentry->d_flags |= DCACHE_OP_SELECT_INODE;
>
>   }
>   EXPORT_SYMBOL(d_set_d_op);
> diff --git a/fs/internal.h b/fs/internal.h
> index 01dce1d1476b..4d5af583ab03 100644
> --- a/fs/internal.h
> +++ b/fs/internal.h
> @@ -107,6 +107,7 @@ extern struct file *do_file_open_root(struct dentry *, struct vfsmount *,
>   extern long do_handle_open(int mountdirfd,
>   			   struct file_handle __user *ufh, int open_flag);
>   extern int open_check_o_direct(struct file *f);
> +extern int vfs_open(const struct path *, struct file *, const struct cred *);
>
>   /*
>    * inode.c
> diff --git a/fs/open.c b/fs/open.c
> index e0250bdcc440..b1c5823b7f11 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -678,18 +678,18 @@ int open_check_o_direct(struct file *f)
>   }
>
>   static int do_dentry_open(struct file *f,
> +			  struct inode *inode,
>   			  int (*open)(struct inode *, struct file *),
>   			  const struct cred *cred)
>   {
>   	static const struct file_operations empty_fops = {};
> -	struct inode *inode;
>   	int error;
>
>   	f->f_mode = OPEN_FMODE(f->f_flags) | FMODE_LSEEK |
>   				FMODE_PREAD | FMODE_PWRITE;
>
>   	path_get(&f->f_path);
> -	inode = f->f_inode = f->f_path.dentry->d_inode;
> +	f->f_inode = inode;
>   	f->f_mapping = inode->i_mapping;
>
>   	if (unlikely(f->f_flags & O_PATH)) {
> @@ -793,7 +793,8 @@ int finish_open(struct file *file, struct dentry *dentry,
>   	BUG_ON(*opened & FILE_OPENED); /* once it's opened, it's opened */
>
>   	file->f_path.dentry = dentry;
> -	error = do_dentry_open(file, open, current_cred());
> +	error = do_dentry_open(file, d_backing_inode(dentry), open,
> +			       current_cred());
>   	if (!error)
>   		*opened |= FILE_OPENED;
>
> @@ -822,6 +823,28 @@ int finish_no_open(struct file *file, struct dentry *dentry)
>   }
>   EXPORT_SYMBOL(finish_no_open);
>
> +/**
> + * vfs_open - open the file at the given path
> + * @path: path to open
> + * @file: newly allocated file with f_flag initialized
> + * @cred: credentials to use
> + */
> +int vfs_open(const struct path *path, struct file *file,
> +	     const struct cred *cred)
> +{
> +	struct dentry *dentry = path->dentry;
> +	struct inode *inode = dentry->d_inode;
> +
> +	file->f_path = *path;
> +	if (dentry->d_flags & DCACHE_OP_SELECT_INODE) {
> +		inode = dentry->d_op->d_select_inode(dentry, file->f_flags);
> +		if (IS_ERR(inode))
> +			return PTR_ERR(inode);
> +	}
> +
> +	return do_dentry_open(file, inode, NULL, cred);
> +}
> +
>   struct file *dentry_open(const struct path *path, int flags,
>   			 const struct cred *cred)
>   {
> @@ -853,26 +876,6 @@ struct file *dentry_open(const struct path *path, int flags,
>   }
>   EXPORT_SYMBOL(dentry_open);
>
> -/**
> - * vfs_open - open the file at the given path
> - * @path: path to open
> - * @filp: newly allocated file with f_flag initialized
> - * @cred: credentials to use
> - */
> -int vfs_open(const struct path *path, struct file *filp,
> -	     const struct cred *cred)
> -{
> -	struct inode *inode = path->dentry->d_inode;
> -
> -	if (inode->i_op->dentry_open)
> -		return inode->i_op->dentry_open(path->dentry, filp, cred);
> -	else {
> -		filp->f_path = *path;
> -		return do_dentry_open(filp, NULL, cred);
> -	}
> -}
> -EXPORT_SYMBOL(vfs_open);
> -
>   static inline int build_open_flags(int flags, umode_t mode, struct open_flags *op)
>   {
>   	int lookup_flags = 0;
> diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> index 21079d1ca2aa..f140e3dbfb7b 100644
> --- a/fs/overlayfs/inode.c
> +++ b/fs/overlayfs/inode.c
> @@ -337,31 +337,30 @@ static bool ovl_open_need_copy_up(int flags, enum ovl_path_type type,
>   	return true;
>   }
>
> -static int ovl_dentry_open(struct dentry *dentry, struct file *file,
> -		    const struct cred *cred)
> +struct inode *ovl_d_select_inode(struct dentry *dentry, unsigned file_flags)
>   {
>   	int err;
>   	struct path realpath;
>   	enum ovl_path_type type;
>
>   	type = ovl_path_real(dentry, &realpath);
> -	if (ovl_open_need_copy_up(file->f_flags, type, realpath.dentry)) {
> +	if (ovl_open_need_copy_up(file_flags, type, realpath.dentry)) {
>   		err = ovl_want_write(dentry);
>   		if (err)
> -			return err;
> +			return ERR_PTR(err);
>
> -		if (file->f_flags & O_TRUNC)
> +		if (file_flags & O_TRUNC)
>   			err = ovl_copy_up_last(dentry, NULL, true);
>   		else
>   			err = ovl_copy_up(dentry);
>   		ovl_drop_write(dentry);
>   		if (err)
> -			return err;
> +			return ERR_PTR(err);
>
>   		ovl_path_upper(dentry, &realpath);
>   	}
>
> -	return vfs_open(&realpath, file, cred);
> +	return d_backing_inode(realpath.dentry);
>   }
>
>   static const struct inode_operations ovl_file_inode_operations = {
> @@ -372,7 +371,6 @@ static const struct inode_operations ovl_file_inode_operations = {
>   	.getxattr	= ovl_getxattr,
>   	.listxattr	= ovl_listxattr,
>   	.removexattr	= ovl_removexattr,
> -	.dentry_open	= ovl_dentry_open,
>   };
>
>   static const struct inode_operations ovl_symlink_inode_operations = {
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index 17ac5afc9ffb..ea5a40b06e3a 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -173,6 +173,7 @@ ssize_t ovl_getxattr(struct dentry *dentry, const char *name,
>   		     void *value, size_t size);
>   ssize_t ovl_listxattr(struct dentry *dentry, char *list, size_t size);
>   int ovl_removexattr(struct dentry *dentry, const char *name);
> +struct inode *ovl_d_select_inode(struct dentry *dentry, unsigned file_flags);
>
>   struct inode *ovl_new_inode(struct super_block *sb, umode_t mode,
>   			    struct ovl_entry *oe);
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index 5f0d1993e6e3..84c5e27fbfd9 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -275,6 +275,7 @@ static void ovl_dentry_release(struct dentry *dentry)
>
>   static const struct dentry_operations ovl_dentry_operations = {
>   	.d_release = ovl_dentry_release,
> +	.d_select_inode = ovl_d_select_inode,
>   };
>
>   static struct ovl_entry *ovl_alloc_entry(unsigned int numlower)
> diff --git a/include/linux/dcache.h b/include/linux/dcache.h
> index df334cbacc6d..167ec0934049 100644
> --- a/include/linux/dcache.h
> +++ b/include/linux/dcache.h
> @@ -160,6 +160,7 @@ struct dentry_operations {
>   	char *(*d_dname)(struct dentry *, char *, int);
>   	struct vfsmount *(*d_automount)(struct path *);
>   	int (*d_manage)(struct dentry *, bool);
> +	struct inode *(*d_select_inode)(struct dentry *, unsigned);
>   } ____cacheline_aligned;
>
>   /*
> @@ -225,6 +226,7 @@ struct dentry_operations {
>
>   #define DCACHE_MAY_FREE			0x00800000
>   #define DCACHE_FALLTHRU			0x01000000 /* Fall through to lower layer */
> +#define DCACHE_OP_SELECT_INODE		0x02000000 /* Unioned entry: dcache op selects inode */
>
>   extern seqlock_t rename_lock;
>
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index b577e801b4af..2bd77e10e8e5 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1641,7 +1641,6 @@ struct inode_operations {
>   	int (*set_acl)(struct inode *, struct posix_acl *, int);
>
>   	/* WARNING: probably going away soon, do not use! */
> -	int (*dentry_open)(struct dentry *, struct file *, const struct cred *);
>   } ____cacheline_aligned;
>
>   ssize_t rw_copy_check_uvector(int type, const struct iovec __user * uvector,
> @@ -2194,7 +2193,6 @@ extern struct file *file_open_name(struct filename *, int, umode_t);
>   extern struct file *filp_open(const char *, int, umode_t);
>   extern struct file *file_open_root(struct dentry *, struct vfsmount *,
>   				   const char *, int);
> -extern int vfs_open(const struct path *, struct file *, const struct cred *);
>   extern struct file * dentry_open(const struct path *, int, const struct cred *);
>   extern int filp_close(struct file *, fl_owner_t id);
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 2/8] overlayfs: Make f_path always point to the overlay and f_inode to the underlay

[Konstantin Khlebnikov]

On 20.07.2015 15:42, Konstantin Khlebnikov wrote:
> On 18.06.2015 16:32, David Howells wrote:
>> Make file->f_path always point to the overlay dentry so that the path in
>> /proc/pid/fd is correct and to ensure that label-based LSMs have
>> access to the
>> overlay as well as the underlay (path-based LSMs probably don't need it).
>
> Cool. Looks like this fixes MNT_NOEXEC,MNT_NOSUID,MNT_NODEV for
> overlayfs. But probably breaks MNT_LOCK_NOEXEC/SUID/DEV.

Just checked: mount -o remount,nosuid and noexec now work as expected.
Flag -o nodev already worked before because it's checked before open.
Overlayfs now ignores these restrictions from lower mountpoints.


Looks like all ok but some brave maintainers add FS_USERNS_MOUNT to
overlayfs: this way any user can create user-ns and bypass restrictions
of underlying filesystem like that was in CVE-2015-1328.

Executing suid binaries from noexec/nosuid filesystem shouldn't be a
problem if this happens only inside user-ns and mnt-ns. Right?

>
>>
>> Using my union testsuite to set things up, before the patch I see:
>>
>>     [root@andromeda union-testsuite]# bash 5</mnt/a/foo107
>>     [root@andromeda union-testsuite]# ls -l /proc/$$/fd/
>>     ...
>>     lr-x------. 1 root root 64 Jun  5 14:38 5 -> /a/foo107
>>     [root@andromeda union-testsuite]# stat /mnt/a/foo107
>>     ...
>>     Device: 23h/35d Inode: 13381       Links: 1
>>     ...
>>     [root@andromeda union-testsuite]# stat -L /proc/$$/fd/5
>>     ...
>>     Device: 23h/35d Inode: 13381       Links: 1
>>     ...
>>
>> After the patch:
>>
>>     [root@andromeda union-testsuite]# bash 5</mnt/a/foo107
>>     [root@andromeda union-testsuite]# ls -l /proc/$$/fd/
>>     ...
>>     lr-x------. 1 root root 64 Jun  5 14:22 5 -> /mnt/a/foo107
>>     [root@andromeda union-testsuite]# stat /mnt/a/foo107
>>     ...
>>     Device: 23h/35d Inode: 40346       Links: 1
>>     ...
>>     [root@andromeda union-testsuite]# stat -L /proc/$$/fd/5
>>     ...
>>     Device: 23h/35d Inode: 40346       Links: 1
>>     ...
>>
>> Note the change in where /proc/$$/fd/5 points to in the ls command.
>> It was
>> pointing to /a/foo107 (which doesn't exist) and now points to
>> /mnt/a/foo107
>> (which is correct).
>>
>> The inode accessed, however, is the lower layer.  The union layer is
>> on device
>> 25h/37d and the upper layer on 24h/36d.
>>
>> Signed-off-by: David Howells <dhowells@redhat.com>
>> ---
>>
>>   fs/dcache.c              |    5 ++++-
>>   fs/internal.h            |    1 +
>>   fs/open.c                |   49
>> ++++++++++++++++++++++++----------------------
>>   fs/overlayfs/inode.c     |   14 ++++++-------
>>   fs/overlayfs/overlayfs.h |    1 +
>>   fs/overlayfs/super.c     |    1 +
>>   include/linux/dcache.h   |    2 ++
>>   include/linux/fs.h       |    2 --
>>   8 files changed, 41 insertions(+), 34 deletions(-)
>>
>> diff --git a/fs/dcache.c b/fs/dcache.c
>> index 37b5afdaf698..c4ce35110704 100644
>> --- a/fs/dcache.c
>> +++ b/fs/dcache.c
>> @@ -1673,7 +1673,8 @@ void d_set_d_op(struct dentry *dentry, const
>> struct dentry_operations *op)
>>                   DCACHE_OP_COMPARE    |
>>                   DCACHE_OP_REVALIDATE    |
>>                   DCACHE_OP_WEAK_REVALIDATE    |
>> -                DCACHE_OP_DELETE ));
>> +                DCACHE_OP_DELETE    |
>> +                DCACHE_OP_SELECT_INODE));
>>       dentry->d_op = op;
>>       if (!op)
>>           return;
>> @@ -1689,6 +1690,8 @@ void d_set_d_op(struct dentry *dentry, const
>> struct dentry_operations *op)
>>           dentry->d_flags |= DCACHE_OP_DELETE;
>>       if (op->d_prune)
>>           dentry->d_flags |= DCACHE_OP_PRUNE;
>> +    if (op->d_select_inode)
>> +        dentry->d_flags |= DCACHE_OP_SELECT_INODE;
>>
>>   }
>>   EXPORT_SYMBOL(d_set_d_op);
>> diff --git a/fs/internal.h b/fs/internal.h
>> index 01dce1d1476b..4d5af583ab03 100644
>> --- a/fs/internal.h
>> +++ b/fs/internal.h
>> @@ -107,6 +107,7 @@ extern struct file *do_file_open_root(struct
>> dentry *, struct vfsmount *,
>>   extern long do_handle_open(int mountdirfd,
>>                  struct file_handle __user *ufh, int open_flag);
>>   extern int open_check_o_direct(struct file *f);
>> +extern int vfs_open(const struct path *, struct file *, const struct
>> cred *);
>>
>>   /*
>>    * inode.c
>> diff --git a/fs/open.c b/fs/open.c
>> index e0250bdcc440..b1c5823b7f11 100644
>> --- a/fs/open.c
>> +++ b/fs/open.c
>> @@ -678,18 +678,18 @@ int open_check_o_direct(struct file *f)
>>   }
>>
>>   static int do_dentry_open(struct file *f,
>> +              struct inode *inode,
>>                 int (*open)(struct inode *, struct file *),
>>                 const struct cred *cred)
>>   {
>>       static const struct file_operations empty_fops = {};
>> -    struct inode *inode;
>>       int error;
>>
>>       f->f_mode = OPEN_FMODE(f->f_flags) | FMODE_LSEEK |
>>                   FMODE_PREAD | FMODE_PWRITE;
>>
>>       path_get(&f->f_path);
>> -    inode = f->f_inode = f->f_path.dentry->d_inode;
>> +    f->f_inode = inode;
>>       f->f_mapping = inode->i_mapping;
>>
>>       if (unlikely(f->f_flags & O_PATH)) {
>> @@ -793,7 +793,8 @@ int finish_open(struct file *file, struct dentry
>> *dentry,
>>       BUG_ON(*opened & FILE_OPENED); /* once it's opened, it's opened */
>>
>>       file->f_path.dentry = dentry;
>> -    error = do_dentry_open(file, open, current_cred());
>> +    error = do_dentry_open(file, d_backing_inode(dentry), open,
>> +                   current_cred());
>>       if (!error)
>>           *opened |= FILE_OPENED;
>>
>> @@ -822,6 +823,28 @@ int finish_no_open(struct file *file, struct
>> dentry *dentry)
>>   }
>>   EXPORT_SYMBOL(finish_no_open);
>>
>> +/**
>> + * vfs_open - open the file at the given path
>> + * @path: path to open
>> + * @file: newly allocated file with f_flag initialized
>> + * @cred: credentials to use
>> + */
>> +int vfs_open(const struct path *path, struct file *file,
>> +         const struct cred *cred)
>> +{
>> +    struct dentry *dentry = path->dentry;
>> +    struct inode *inode = dentry->d_inode;
>> +
>> +    file->f_path = *path;
>> +    if (dentry->d_flags & DCACHE_OP_SELECT_INODE) {
>> +        inode = dentry->d_op->d_select_inode(dentry, file->f_flags);
>> +        if (IS_ERR(inode))
>> +            return PTR_ERR(inode);
>> +    }
>> +
>> +    return do_dentry_open(file, inode, NULL, cred);
>> +}
>> +
>>   struct file *dentry_open(const struct path *path, int flags,
>>                const struct cred *cred)
>>   {
>> @@ -853,26 +876,6 @@ struct file *dentry_open(const struct path *path,
>> int flags,
>>   }
>>   EXPORT_SYMBOL(dentry_open);
>>
>> -/**
>> - * vfs_open - open the file at the given path
>> - * @path: path to open
>> - * @filp: newly allocated file with f_flag initialized
>> - * @cred: credentials to use
>> - */
>> -int vfs_open(const struct path *path, struct file *filp,
>> -         const struct cred *cred)
>> -{
>> -    struct inode *inode = path->dentry->d_inode;
>> -
>> -    if (inode->i_op->dentry_open)
>> -        return inode->i_op->dentry_open(path->dentry, filp, cred);
>> -    else {
>> -        filp->f_path = *path;
>> -        return do_dentry_open(filp, NULL, cred);
>> -    }
>> -}
>> -EXPORT_SYMBOL(vfs_open);
>> -
>>   static inline int build_open_flags(int flags, umode_t mode, struct
>> open_flags *op)
>>   {
>>       int lookup_flags = 0;
>> diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
>> index 21079d1ca2aa..f140e3dbfb7b 100644
>> --- a/fs/overlayfs/inode.c
>> +++ b/fs/overlayfs/inode.c
>> @@ -337,31 +337,30 @@ static bool ovl_open_need_copy_up(int flags,
>> enum ovl_path_type type,
>>       return true;
>>   }
>>
>> -static int ovl_dentry_open(struct dentry *dentry, struct file *file,
>> -            const struct cred *cred)
>> +struct inode *ovl_d_select_inode(struct dentry *dentry, unsigned
>> file_flags)
>>   {
>>       int err;
>>       struct path realpath;
>>       enum ovl_path_type type;
>>
>>       type = ovl_path_real(dentry, &realpath);
>> -    if (ovl_open_need_copy_up(file->f_flags, type, realpath.dentry)) {
>> +    if (ovl_open_need_copy_up(file_flags, type, realpath.dentry)) {
>>           err = ovl_want_write(dentry);
>>           if (err)
>> -            return err;
>> +            return ERR_PTR(err);
>>
>> -        if (file->f_flags & O_TRUNC)
>> +        if (file_flags & O_TRUNC)
>>               err = ovl_copy_up_last(dentry, NULL, true);
>>           else
>>               err = ovl_copy_up(dentry);
>>           ovl_drop_write(dentry);
>>           if (err)
>> -            return err;
>> +            return ERR_PTR(err);
>>
>>           ovl_path_upper(dentry, &realpath);
>>       }
>>
>> -    return vfs_open(&realpath, file, cred);
>> +    return d_backing_inode(realpath.dentry);
>>   }
>>
>>   static const struct inode_operations ovl_file_inode_operations = {
>> @@ -372,7 +371,6 @@ static const struct inode_operations
>> ovl_file_inode_operations = {
>>       .getxattr    = ovl_getxattr,
>>       .listxattr    = ovl_listxattr,
>>       .removexattr    = ovl_removexattr,
>> -    .dentry_open    = ovl_dentry_open,
>>   };
>>
>>   static const struct inode_operations ovl_symlink_inode_operations = {
>> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
>> index 17ac5afc9ffb..ea5a40b06e3a 100644
>> --- a/fs/overlayfs/overlayfs.h
>> +++ b/fs/overlayfs/overlayfs.h
>> @@ -173,6 +173,7 @@ ssize_t ovl_getxattr(struct dentry *dentry, const
>> char *name,
>>                void *value, size_t size);
>>   ssize_t ovl_listxattr(struct dentry *dentry, char *list, size_t size);
>>   int ovl_removexattr(struct dentry *dentry, const char *name);
>> +struct inode *ovl_d_select_inode(struct dentry *dentry, unsigned
>> file_flags);
>>
>>   struct inode *ovl_new_inode(struct super_block *sb, umode_t mode,
>>                   struct ovl_entry *oe);
>> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
>> index 5f0d1993e6e3..84c5e27fbfd9 100644
>> --- a/fs/overlayfs/super.c
>> +++ b/fs/overlayfs/super.c
>> @@ -275,6 +275,7 @@ static void ovl_dentry_release(struct dentry *dentry)
>>
>>   static const struct dentry_operations ovl_dentry_operations = {
>>       .d_release = ovl_dentry_release,
>> +    .d_select_inode = ovl_d_select_inode,
>>   };
>>
>>   static struct ovl_entry *ovl_alloc_entry(unsigned int numlower)
>> diff --git a/include/linux/dcache.h b/include/linux/dcache.h
>> index df334cbacc6d..167ec0934049 100644
>> --- a/include/linux/dcache.h
>> +++ b/include/linux/dcache.h
>> @@ -160,6 +160,7 @@ struct dentry_operations {
>>       char *(*d_dname)(struct dentry *, char *, int);
>>       struct vfsmount *(*d_automount)(struct path *);
>>       int (*d_manage)(struct dentry *, bool);
>> +    struct inode *(*d_select_inode)(struct dentry *, unsigned);
>>   } ____cacheline_aligned;
>>
>>   /*
>> @@ -225,6 +226,7 @@ struct dentry_operations {
>>
>>   #define DCACHE_MAY_FREE            0x00800000
>>   #define DCACHE_FALLTHRU            0x01000000 /* Fall through to
>> lower layer */
>> +#define DCACHE_OP_SELECT_INODE        0x02000000 /* Unioned entry:
>> dcache op selects inode */
>>
>>   extern seqlock_t rename_lock;
>>
>> diff --git a/include/linux/fs.h b/include/linux/fs.h
>> index b577e801b4af..2bd77e10e8e5 100644
>> --- a/include/linux/fs.h
>> +++ b/include/linux/fs.h
>> @@ -1641,7 +1641,6 @@ struct inode_operations {
>>       int (*set_acl)(struct inode *, struct posix_acl *, int);
>>
>>       /* WARNING: probably going away soon, do not use! */
>> -    int (*dentry_open)(struct dentry *, struct file *, const struct
>> cred *);
>>   } ____cacheline_aligned;
>>
>>   ssize_t rw_copy_check_uvector(int type, const struct iovec __user *
>> uvector,
>> @@ -2194,7 +2193,6 @@ extern struct file *file_open_name(struct
>> filename *, int, umode_t);
>>   extern struct file *filp_open(const char *, int, umode_t);
>>   extern struct file *file_open_root(struct dentry *, struct vfsmount *,
>>                      const char *, int);
>> -extern int vfs_open(const struct path *, struct file *, const struct
>> cred *);
>>   extern struct file * dentry_open(const struct path *, int, const
>> struct cred *);
>>   extern int filp_close(struct file *, fl_owner_t id);
>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe
>> linux-fsdevel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>

[PATCH 3/8] Security: Provide copy-up security hooks for unioned files

[David Howells]

Provide two new security hooks for use with security files that are used when
a file is copied up between layers:

 (1) security_inode_copy_up().  This is called so that the security label on
     the destination file can be set appropriately.

 (2) security_inode_copy_up_xattr().  This is called so that each xattr being
     copied up can be vetted - including modification and discard.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 include/linux/security.h |   36 ++++++++++++++++++++++++++++++++++++
 security/capability.c    |   13 +++++++++++++
 security/security.c      |   13 +++++++++++++
 3 files changed, 62 insertions(+)

diff --git a/include/linux/security.h b/include/linux/security.h
index 52febde52479..537c77b4cd5e 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -562,6 +562,25 @@ static inline void security_free_mnt_opts(struct security_mnt_opts *opts)
  *	@inode contains a pointer to the inode.
  *	@secid contains a pointer to the location where result will be saved.
  *	In case of failure, @secid will be set to zero.
+ * @inode_copy_up:
+ *	Appropriately label the destination inode when a unioned file is copied
+ *	up from a lower layer to the union/overlay layer.
+ *	@src indicates the file that is being copied up.
+ *	@dst indicates the file that has being created by the copy up.
+ *	Returns 0 on success or a negative error code on error.
+ * @inode_copy_up_xattr:
+ *	Filter/modify the xattrs being copied up when a unioned file is copied
+ *	up from a lower layer to the union/overlay layer.
+ *	@src indicates the file that is being copied up.
+ *	@dst indicates the file that has being created by the copy up.
+ *	@name indicates the name of the xattr.
+ *	@value, *@size indicate the payload of the xattr.
+ *	Returns 0 to accept the xattr, 1 to discard the xattr or a negative
+ *	error code to abort the copy up.  The xattr buffer must be at least
+ *	XATTR_SIZE_MAX in capacity and the contents may be modified and *@size
+ *	changed appropriately.  Note that the caller is responsible for reading
+ *	and writing the xattrs as this hook is merely a filter.
+ *
  *
  * Security hooks for file operations
  *
@@ -1571,6 +1590,9 @@ struct security_operations {
 	int (*inode_setsecurity) (struct inode *inode, const char *name, const void *value, size_t size, int flags);
 	int (*inode_listsecurity) (struct inode *inode, char *buffer, size_t buffer_size);
 	void (*inode_getsecid) (const struct inode *inode, u32 *secid);
+	int (*inode_copy_up) (struct dentry *src, struct dentry *dst);
+	int (*inode_copy_up_xattr) (struct dentry *src, struct dentry *dst,
+				    const char *name, void *value, size_t *size);
 
 	int (*file_permission) (struct file *file, int mask);
 	int (*file_alloc_security) (struct file *file);
@@ -1858,6 +1880,10 @@ int security_inode_getsecurity(const struct inode *inode, const char *name, void
 int security_inode_setsecurity(struct inode *inode, const char *name, const void *value, size_t size, int flags);
 int security_inode_listsecurity(struct inode *inode, char *buffer, size_t buffer_size);
 void security_inode_getsecid(const struct inode *inode, u32 *secid);
+int security_inode_copy_up(struct dentry *src, struct dentry *dst);
+int security_inode_copy_up_xattr(struct dentry *src, struct dentry *dst,
+				 const char *name, void *value, size_t *size);
+
 int security_file_permission(struct file *file, int mask);
 int security_file_alloc(struct file *file);
 void security_file_free(struct file *file);
@@ -2323,6 +2349,16 @@ static inline void security_inode_getsecid(const struct inode *inode, u32 *secid
 	*secid = 0;
 }
 
+static inline int security_inode_copy_up(struct dentry *src, struct dentry *dst)
+{
+	return 0;
+}
+static inline int security_inode_copy_up_xattr(struct dentry *src, struct dentry *dst,
+					       const char *name, const void *value, size_t *size)
+{
+	return 0;
+}
+
 static inline int security_file_permission(struct file *file, int mask)
 {
 	return 0;
diff --git a/security/capability.c b/security/capability.c
index 7d3f38fe02ba..c59f7c548c7a 100644
--- a/security/capability.c
+++ b/security/capability.c
@@ -268,6 +268,17 @@ static void cap_inode_getsecid(const struct inode *inode, u32 *secid)
 	*secid = 0;
 }
 
+static int cap_inode_copy_up(struct dentry *src, struct dentry *dst)
+{
+	return 0;
+}
+
+static int cap_inode_copy_up_xattr(struct dentry *src, struct dentry *dst,
+				   const char *name, void *value, size_t *size)
+{
+	return 0;
+}
+
 #ifdef CONFIG_SECURITY_PATH
 static int cap_path_mknod(struct path *dir, struct dentry *dentry, umode_t mode,
 			  unsigned int dev)
@@ -1008,6 +1019,8 @@ void __init security_fixup_ops(struct security_operations *ops)
 	set_to_cap_if_null(ops, inode_setsecurity);
 	set_to_cap_if_null(ops, inode_listsecurity);
 	set_to_cap_if_null(ops, inode_getsecid);
+	set_to_cap_if_null(ops, inode_copy_up);
+	set_to_cap_if_null(ops, inode_copy_up_xattr);
 #ifdef CONFIG_SECURITY_PATH
 	set_to_cap_if_null(ops, path_mknod);
 	set_to_cap_if_null(ops, path_mkdir);
diff --git a/security/security.c b/security/security.c
index 04c8feca081a..cb3563013ef5 100644
--- a/security/security.c
+++ b/security/security.c
@@ -707,6 +707,19 @@ void security_inode_getsecid(const struct inode *inode, u32 *secid)
 	security_ops->inode_getsecid(inode, secid);
 }
 
+int security_inode_copy_up(struct dentry *src, struct dentry *dst)
+{
+	return security_ops->inode_copy_up(src, dst);
+}
+EXPORT_SYMBOL(security_inode_copy_up);
+
+int security_inode_copy_up_xattr(struct dentry *src, struct dentry *dst,
+				 const char *name, void *value, size_t *size)
+{
+	return security_ops->inode_copy_up_xattr(src, dst, name, value, size);
+}
+EXPORT_SYMBOL(security_inode_copy_up_xattr);
+
 int security_file_permission(struct file *file, int mask)
 {
 	int ret;

[PATCH 4/8] Overlayfs: Use copy-up security hooks

[David Howells]

Use the copy-up security hooks previously provided to allow an LSM to adjust
the security on a newly created copy and to filter the xattrs copied to that
file copy.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 fs/overlayfs/copy_up.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 24f640441bd9..a350f2314c56 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -58,6 +58,14 @@ int ovl_copy_xattr(struct dentry *old, struct dentry *new)
 			error = size;
 			goto out_free_value;
 		}
+		error = security_inode_copy_up_xattr(old, new,
+						     name, value, &size);
+		if (error < 0)
+			goto out_free_value;
+		if (error == 1) {
+			error = 0;
+			continue; /* Discard */
+		}
 		error = vfs_setxattr(new, name, value, size, 0);
 		if (error)
 			goto out_free_value;
@@ -223,6 +231,10 @@ static int ovl_copy_up_locked(struct dentry *workdir, struct dentry *upperdir,
 	if (err)
 		goto out2;
 
+	err = security_inode_copy_up(lowerpath->dentry, newdentry);
+	if (err < 0)
+		goto out_cleanup;
+
 	if (S_ISREG(stat->mode)) {
 		struct path upperpath;
 		ovl_path_upper(dentry, &upperpath);

[PATCH 5/8] SELinux: Stub in copy-up handling

[David Howells]

Provide stubs for union/overlay copy-up handling.  The xattr copy up stub
discards lower SELinux xattrs rather than letting them be copied up so that
the security label on the copy doesn't get corrupted.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 security/selinux/hooks.c |   16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index ffa5a642629a..c5d893e2ff23 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3200,6 +3200,20 @@ static void selinux_inode_getsecid(const struct inode *inode, u32 *secid)
 	*secid = isec->sid;
 }
 
+static int selinux_inode_copy_up(struct dentry *src, struct dentry *dst)
+{
+	return 0;
+}
+
+static int selinux_inode_copy_up_xattr(struct dentry *src, struct dentry *dst,
+				       const char *name, void *value,
+				       size_t *size)
+{
+	if (strcmp(name, XATTR_NAME_SELINUX) == 0)
+		return 1; /* Discard */
+	return 0;
+}
+
 /* file security operations */
 
 static int selinux_revalidate_file_permission(struct file *file, int mask)
@@ -5917,6 +5931,8 @@ static struct security_operations selinux_ops = {
 	.inode_setsecurity =		selinux_inode_setsecurity,
 	.inode_listsecurity =		selinux_inode_listsecurity,
 	.inode_getsecid =		selinux_inode_getsecid,
+	.inode_copy_up =		selinux_inode_copy_up,
+	.inode_copy_up_xattr =		selinux_inode_copy_up_xattr,
 
 	.file_permission =		selinux_file_permission,
 	.file_alloc_security =		selinux_file_alloc_security,

Re: [PATCH 5/8] SELinux: Stub in copy-up handling

[Stephen Smalley]

On 06/18/2015 09:32 AM, David Howells wrote:
> Provide stubs for union/overlay copy-up handling.  The xattr copy up stub
> discards lower SELinux xattrs rather than letting them be copied up so that
> the security label on the copy doesn't get corrupted.
> 
> Signed-off-by: David Howells <dhowells@redhat.com>
> ---
> 
>  security/selinux/hooks.c |   16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index ffa5a642629a..c5d893e2ff23 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3200,6 +3200,20 @@ static void selinux_inode_getsecid(const struct inode *inode, u32 *secid)
>  	*secid = isec->sid;
>  }
>  
> +static int selinux_inode_copy_up(struct dentry *src, struct dentry *dst)
> +{
> +	return 0;
> +}
> +
> +static int selinux_inode_copy_up_xattr(struct dentry *src, struct dentry *dst,
> +				       const char *name, void *value,
> +				       size_t *size)
> +{
> +	if (strcmp(name, XATTR_NAME_SELINUX) == 0)
> +		return 1; /* Discard */
> +	return 0;
> +}
> +

(expanded cc list)

I'm not sure we never want to copy up the SELinux attribute.  See my
other email about ecryptfs and supporting per-file labeling and
consistent access control over the upper and lower inodes.

>  /* file security operations */
>  
>  static int selinux_revalidate_file_permission(struct file *file, int mask)
> @@ -5917,6 +5931,8 @@ static struct security_operations selinux_ops = {
>  	.inode_setsecurity =		selinux_inode_setsecurity,
>  	.inode_listsecurity =		selinux_inode_listsecurity,
>  	.inode_getsecid =		selinux_inode_getsecid,
> +	.inode_copy_up =		selinux_inode_copy_up,
> +	.inode_copy_up_xattr =		selinux_inode_copy_up_xattr,
>  
>  	.file_permission =		selinux_file_permission,
>  	.file_alloc_security =		selinux_file_alloc_security,
> 
> 

Re: [PATCH 5/8] SELinux: Stub in copy-up handling

[Stephen Smalley]

On 06/18/2015 09:32 AM, David Howells wrote:
> Provide stubs for union/overlay copy-up handling.  The xattr copy up stub
> discards lower SELinux xattrs rather than letting them be copied up so that
> the security label on the copy doesn't get corrupted.
> 
> Signed-off-by: David Howells <dhowells@redhat.com>
> ---
> 
>  security/selinux/hooks.c |   16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index ffa5a642629a..c5d893e2ff23 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3200,6 +3200,20 @@ static void selinux_inode_getsecid(const struct inode *inode, u32 *secid)
>  	*secid = isec->sid;
>  }
>  
> +static int selinux_inode_copy_up(struct dentry *src, struct dentry *dst)
> +{
> +	return 0;
> +}
> +
> +static int selinux_inode_copy_up_xattr(struct dentry *src, struct dentry *dst,
> +				       const char *name, void *value,
> +				       size_t *size)
> +{
> +	if (strcmp(name, XATTR_NAME_SELINUX) == 0)
> +		return 1; /* Discard */
> +	return 0;
> +}
> +

(expanded cc list)

I'm not sure we never want to copy up the SELinux attribute.  See my
other email about ecryptfs and supporting per-file labeling and
consistent access control over the upper and lower inodes.

>  /* file security operations */
>  
>  static int selinux_revalidate_file_permission(struct file *file, int mask)
> @@ -5917,6 +5931,8 @@ static struct security_operations selinux_ops = {
>  	.inode_setsecurity =		selinux_inode_setsecurity,
>  	.inode_listsecurity =		selinux_inode_listsecurity,
>  	.inode_getsecid =		selinux_inode_getsecid,
> +	.inode_copy_up =		selinux_inode_copy_up,
> +	.inode_copy_up_xattr =		selinux_inode_copy_up_xattr,
>  
>  	.file_permission =		selinux_file_permission,
>  	.file_alloc_security =		selinux_file_alloc_security,
> 
> 

Re: [PATCH 5/8] SELinux: Stub in copy-up handling

[Casey Schaufler]

On 6/18/2015 6:32 AM, David Howells wrote:
> Provide stubs for union/overlay copy-up handling.  The xattr copy up stub
> discards lower SELinux xattrs rather than letting them be copied up so that
> the security label on the copy doesn't get corrupted.

Are you planning to do this for Smack, too?

>
> Signed-off-by: David Howells <dhowells@redhat.com>
> ---
>
>  security/selinux/hooks.c |   16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index ffa5a642629a..c5d893e2ff23 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3200,6 +3200,20 @@ static void selinux_inode_getsecid(const struct inode *inode, u32 *secid)
>  	*secid = isec->sid;
>  }
>  
> +static int selinux_inode_copy_up(struct dentry *src, struct dentry *dst)
> +{
> +	return 0;
> +}
> +
> +static int selinux_inode_copy_up_xattr(struct dentry *src, struct dentry *dst,
> +				       const char *name, void *value,
> +				       size_t *size)
> +{
> +	if (strcmp(name, XATTR_NAME_SELINUX) == 0)
> +		return 1; /* Discard */
> +	return 0;
> +}
> +
>  /* file security operations */
>  
>  static int selinux_revalidate_file_permission(struct file *file, int mask)
> @@ -5917,6 +5931,8 @@ static struct security_operations selinux_ops = {
>  	.inode_setsecurity =		selinux_inode_setsecurity,
>  	.inode_listsecurity =		selinux_inode_listsecurity,
>  	.inode_getsecid =		selinux_inode_getsecid,
> +	.inode_copy_up =		selinux_inode_copy_up,
> +	.inode_copy_up_xattr =		selinux_inode_copy_up_xattr,
>  
>  	.file_permission =		selinux_file_permission,
>  	.file_alloc_security =		selinux_file_alloc_security,
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 5/8] SELinux: Stub in copy-up handling

[David Howells]

Stephen Smalley <sds@tycho.nsa.gov> wrote:

> I'm not sure we never want to copy up the SELinux attribute.  See my
> other email about ecryptfs and supporting per-file labeling and
> consistent access control over the upper and lower inodes.

Yes.  That's why there are two operations. inode_copy_up() should set the
label appropriately and inode_copy_up_xattr() should prevent the label set by
inode_copy_up() from being clobbered.

Note that inode_copy_up() has access to the lower file label and can perform
some sort of incantation to transmute it before applying it.

David

Re: [PATCH 5/8] SELinux: Stub in copy-up handling

[David Howells]

Stephen Smalley <sds@tycho.nsa.gov> wrote:

> I'm not sure we never want to copy up the SELinux attribute.  See my
> other email about ecryptfs and supporting per-file labeling and
> consistent access control over the upper and lower inodes.

Yes.  That's why there are two operations. inode_copy_up() should set the
label appropriately and inode_copy_up_xattr() should prevent the label set by
inode_copy_up() from being clobbered.

Note that inode_copy_up() has access to the lower file label and can perform
some sort of incantation to transmute it before applying it.

David

[PATCH 6/8] SELinux: Handle opening of a unioned file

[David Howells]

Handle the opening of a unioned file by trying to derive the label that would
be attached to the union-layer inode if it doesn't exist.

If the union-layer inode does exist (as it necessarily does in overlayfs, but
not in unionmount), we assume that it has the right label and use that.
Otherwise we try to get it from the superblock.

If the superblock has a globally-applied label, we use that, otherwise we try
to transition to an appropriate label.  This union label is then stored in the
file_security_struct.

We then perform an additional check to make sure that the calling task is
granted permission by the union-layer inode label to open the file in addition
to a check to make sure that the task is granted permission to open the lower
file with the lower inode label.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 security/selinux/hooks.c          |   69 +++++++++++++++++++++++++++++++++++++
 security/selinux/include/objsec.h |    1 +
 2 files changed, 70 insertions(+)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index c5d893e2ff23..c4495a797eb1 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3490,10 +3490,72 @@ static int selinux_file_receive(struct file *file)
 	return file_has_perm(cred, file, file_to_av(file));
 }
 
+/*
+ * We have a file opened on a unioned file system that falls through to a file
+ * on a lower layer.  If there is a union inode, we try to get the label from
+ * that, otherwise we need to get it from the superblock.
+ *
+ * file->f_path points to the union layer and file->f_inode points to the lower
+ * layer.
+ */
+static int selinux_file_open_union(struct file *file,
+				   struct file_security_struct *fsec,
+				   const struct cred *cred)
+{
+	const struct superblock_security_struct *sbsec;
+	const struct inode_security_struct *isec, *dsec, *fisec;
+	const struct task_security_struct *tsec = current_security();
+	struct common_audit_data ad;
+	struct dentry *union_dentry = file->f_path.dentry;
+	const struct inode *union_inode = d_inode(union_dentry);
+	const struct inode *lower_inode = file_inode(file);
+	struct dentry *dir;
+	int rc;
+
+	sbsec = union_dentry->d_sb->s_security;
+
+	if (union_inode) {
+		isec = union_inode->i_security;
+		fsec->union_isid = isec->sid;
+	} else if ((sbsec->flags & SE_SBINITIALIZED) &&
+		   (sbsec->behavior == SECURITY_FS_USE_MNTPOINT)) {
+		fsec->union_isid = sbsec->mntpoint_sid;
+	} else {
+		dir = dget_parent(union_dentry);
+		dsec = d_inode(dir)->i_security;
+
+		rc = security_transition_sid(
+			tsec->sid, dsec->sid,
+			inode_mode_to_security_class(lower_inode->i_mode),
+			&union_dentry->d_name,
+			&fsec->union_isid);
+		dput(dir);
+		if (rc) {
+			pr_warn("%s:  security_transition_sid failed, rc=%d (name=%pD)\n",
+				__func__, -rc, file);
+			return rc;
+		}
+	}
+
+	/* We need to check that the union file is allowed to be opened as well
+	 * as checking that the lower file is allowed to be opened.
+	 */
+	if (unlikely(IS_PRIVATE(lower_inode)))
+		return 0;
+
+	ad.type = LSM_AUDIT_DATA_PATH;
+	ad.u.path = file->f_path;
+
+	fisec = lower_inode->i_security;
+	return avc_has_perm(cred_sid(cred), fsec->union_isid, fisec->sclass,
+			    open_file_to_av(file), &ad);
+}
+
 static int selinux_file_open(struct file *file, const struct cred *cred)
 {
 	struct file_security_struct *fsec;
 	struct inode_security_struct *isec;
+	int rc;
 
 	fsec = file->f_security;
 	isec = file_inode(file)->i_security;
@@ -3514,6 +3576,13 @@ static int selinux_file_open(struct file *file, const struct cred *cred)
 	 * new inode label or new policy.
 	 * This check is not redundant - do not remove.
 	 */
+
+	if (d_inode(file->f_path.dentry) != file->f_inode) {
+		rc = selinux_file_open_union(file, fsec, cred);
+		if (rc < 0)
+			return rc;
+	}
+
 	return file_path_has_perm(cred, file, open_file_to_av(file));
 }
 
diff --git a/security/selinux/include/objsec.h b/security/selinux/include/objsec.h
index 81fa718d5cb3..f088c080aa9e 100644
--- a/security/selinux/include/objsec.h
+++ b/security/selinux/include/objsec.h
@@ -54,6 +54,7 @@ struct file_security_struct {
 	u32 sid;		/* SID of open file description */
 	u32 fown_sid;		/* SID of file owner (for SIGIO) */
 	u32 isid;		/* SID of inode at the time of file open */
+	u32 union_isid;		/* SID of would-be inodes in union top (or 0) */
 	u32 pseqno;		/* Policy seqno at the time of file open */
 };
 

Re: [PATCH 6/8] SELinux: Handle opening of a unioned file

[Stephen Smalley]

On 06/18/2015 09:33 AM, David Howells wrote:
> Handle the opening of a unioned file by trying to derive the label that would
> be attached to the union-layer inode if it doesn't exist.
> 
> If the union-layer inode does exist (as it necessarily does in overlayfs, but
> not in unionmount), we assume that it has the right label and use that.
> Otherwise we try to get it from the superblock.
> 
> If the superblock has a globally-applied label, we use that, otherwise we try
> to transition to an appropriate label.  This union label is then stored in the
> file_security_struct.
> 
> We then perform an additional check to make sure that the calling task is
> granted permission by the union-layer inode label to open the file in addition
> to a check to make sure that the task is granted permission to open the lower
> file with the lower inode label.
> 
> Signed-off-by: David Howells <dhowells@redhat.com>
> ---
> 
>  security/selinux/hooks.c          |   69 +++++++++++++++++++++++++++++++++++++
>  security/selinux/include/objsec.h |    1 +
>  2 files changed, 70 insertions(+)
> 
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index c5d893e2ff23..c4495a797eb1 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3490,10 +3490,72 @@ static int selinux_file_receive(struct file *file)
>  	return file_has_perm(cred, file, file_to_av(file));
>  }
>  
> +/*
> + * We have a file opened on a unioned file system that falls through to a file
> + * on a lower layer.  If there is a union inode, we try to get the label from
> + * that, otherwise we need to get it from the superblock.
> + *
> + * file->f_path points to the union layer and file->f_inode points to the lower
> + * layer.
> + */
> +static int selinux_file_open_union(struct file *file,
> +				   struct file_security_struct *fsec,
> +				   const struct cred *cred)
> +{
> +	const struct superblock_security_struct *sbsec;
> +	const struct inode_security_struct *isec, *dsec, *fisec;
> +	const struct task_security_struct *tsec = current_security();
> +	struct common_audit_data ad;
> +	struct dentry *union_dentry = file->f_path.dentry;
> +	const struct inode *union_inode = d_inode(union_dentry);
> +	const struct inode *lower_inode = file_inode(file);
> +	struct dentry *dir;
> +	int rc;
> +
> +	sbsec = union_dentry->d_sb->s_security;
> +
> +	if (union_inode) {
> +		isec = union_inode->i_security;
> +		fsec->union_isid = isec->sid;
> +	} else if ((sbsec->flags & SE_SBINITIALIZED) &&
> +		   (sbsec->behavior == SECURITY_FS_USE_MNTPOINT)) {
> +		fsec->union_isid = sbsec->mntpoint_sid;
> +	} else {
> +		dir = dget_parent(union_dentry);
> +		dsec = d_inode(dir)->i_security;
> +
> +		rc = security_transition_sid(
> +			tsec->sid, dsec->sid,
> +			inode_mode_to_security_class(lower_inode->i_mode),
> +			&union_dentry->d_name,
> +			&fsec->union_isid);
> +		dput(dir);
> +		if (rc) {
> +			pr_warn("%s:  security_transition_sid failed, rc=%d (name=%pD)\n",
> +				__func__, -rc, file);

I would drop this pr_warn altogether (and ultimately the printk from
inode_init_security).  Not necessary.

> +			return rc;
> +		}
> +	}
> +
> +	/* We need to check that the union file is allowed to be opened as well
> +	 * as checking that the lower file is allowed to be opened.

Hmm...so if I try to open a file for write access, then we are going to
require that the process be allowed to write to both the union/overlay
inode and to the lower inode?  That seems problematic for the containers
use case where no write access will be granted to the lower files.

> +	 */
> +	if (unlikely(IS_PRIVATE(lower_inode)))
> +		return 0;
> +
> +	ad.type = LSM_AUDIT_DATA_PATH;
> +	ad.u.path = file->f_path;
> +
> +	fisec = lower_inode->i_security;
> +	return avc_has_perm(cred_sid(cred), fsec->union_isid, fisec->sclass,
> +			    open_file_to_av(file), &ad);
> +}
> +
>  static int selinux_file_open(struct file *file, const struct cred *cred)
>  {
>  	struct file_security_struct *fsec;
>  	struct inode_security_struct *isec;
> +	int rc;
>  
>  	fsec = file->f_security;
>  	isec = file_inode(file)->i_security;
> @@ -3514,6 +3576,13 @@ static int selinux_file_open(struct file *file, const struct cred *cred)
>  	 * new inode label or new policy.
>  	 * This check is not redundant - do not remove.
>  	 */
> +
> +	if (d_inode(file->f_path.dentry) != file->f_inode) {
> +		rc = selinux_file_open_union(file, fsec, cred);
> +		if (rc < 0)
> +			return rc;
> +	}
> +
>  	return file_path_has_perm(cred, file, open_file_to_av(file));
>  }
>  
> diff --git a/security/selinux/include/objsec.h b/security/selinux/include/objsec.h
> index 81fa718d5cb3..f088c080aa9e 100644
> --- a/security/selinux/include/objsec.h
> +++ b/security/selinux/include/objsec.h
> @@ -54,6 +54,7 @@ struct file_security_struct {
>  	u32 sid;		/* SID of open file description */
>  	u32 fown_sid;		/* SID of file owner (for SIGIO) */
>  	u32 isid;		/* SID of inode at the time of file open */
> +	u32 union_isid;		/* SID of would-be inodes in union top (or 0) */
>  	u32 pseqno;		/* Policy seqno at the time of file open */
>  };
>  
> 
> 

Re: [PATCH 6/8] SELinux: Handle opening of a unioned file

[Stephen Smalley]

On 06/18/2015 09:33 AM, David Howells wrote:
> Handle the opening of a unioned file by trying to derive the label that would
> be attached to the union-layer inode if it doesn't exist.
> 
> If the union-layer inode does exist (as it necessarily does in overlayfs, but
> not in unionmount), we assume that it has the right label and use that.
> Otherwise we try to get it from the superblock.
> 
> If the superblock has a globally-applied label, we use that, otherwise we try
> to transition to an appropriate label.  This union label is then stored in the
> file_security_struct.
> 
> We then perform an additional check to make sure that the calling task is
> granted permission by the union-layer inode label to open the file in addition
> to a check to make sure that the task is granted permission to open the lower
> file with the lower inode label.
> 
> Signed-off-by: David Howells <dhowells@redhat.com>
> ---
> 
>  security/selinux/hooks.c          |   69 +++++++++++++++++++++++++++++++++++++
>  security/selinux/include/objsec.h |    1 +
>  2 files changed, 70 insertions(+)
> 
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index c5d893e2ff23..c4495a797eb1 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3490,10 +3490,72 @@ static int selinux_file_receive(struct file *file)
>  	return file_has_perm(cred, file, file_to_av(file));
>  }
>  
> +/*
> + * We have a file opened on a unioned file system that falls through to a file
> + * on a lower layer.  If there is a union inode, we try to get the label from
> + * that, otherwise we need to get it from the superblock.
> + *
> + * file->f_path points to the union layer and file->f_inode points to the lower
> + * layer.
> + */
> +static int selinux_file_open_union(struct file *file,
> +				   struct file_security_struct *fsec,
> +				   const struct cred *cred)
> +{
> +	const struct superblock_security_struct *sbsec;
> +	const struct inode_security_struct *isec, *dsec, *fisec;
> +	const struct task_security_struct *tsec = current_security();
> +	struct common_audit_data ad;
> +	struct dentry *union_dentry = file->f_path.dentry;
> +	const struct inode *union_inode = d_inode(union_dentry);
> +	const struct inode *lower_inode = file_inode(file);
> +	struct dentry *dir;
> +	int rc;
> +
> +	sbsec = union_dentry->d_sb->s_security;
> +
> +	if (union_inode) {
> +		isec = union_inode->i_security;
> +		fsec->union_isid = isec->sid;
> +	} else if ((sbsec->flags & SE_SBINITIALIZED) &&
> +		   (sbsec->behavior == SECURITY_FS_USE_MNTPOINT)) {
> +		fsec->union_isid = sbsec->mntpoint_sid;
> +	} else {
> +		dir = dget_parent(union_dentry);
> +		dsec = d_inode(dir)->i_security;
> +
> +		rc = security_transition_sid(
> +			tsec->sid, dsec->sid,
> +			inode_mode_to_security_class(lower_inode->i_mode),
> +			&union_dentry->d_name,
> +			&fsec->union_isid);
> +		dput(dir);
> +		if (rc) {
> +			pr_warn("%s:  security_transition_sid failed, rc=%d (name=%pD)\n",
> +				__func__, -rc, file);

I would drop this pr_warn altogether (and ultimately the printk from
inode_init_security).  Not necessary.

> +			return rc;
> +		}
> +	}
> +
> +	/* We need to check that the union file is allowed to be opened as well
> +	 * as checking that the lower file is allowed to be opened.

Hmm...so if I try to open a file for write access, then we are going to
require that the process be allowed to write to both the union/overlay
inode and to the lower inode?  That seems problematic for the containers
use case where no write access will be granted to the lower files.

> +	 */
> +	if (unlikely(IS_PRIVATE(lower_inode)))
> +		return 0;
> +
> +	ad.type = LSM_AUDIT_DATA_PATH;
> +	ad.u.path = file->f_path;
> +
> +	fisec = lower_inode->i_security;
> +	return avc_has_perm(cred_sid(cred), fsec->union_isid, fisec->sclass,
> +			    open_file_to_av(file), &ad);
> +}
> +
>  static int selinux_file_open(struct file *file, const struct cred *cred)
>  {
>  	struct file_security_struct *fsec;
>  	struct inode_security_struct *isec;
> +	int rc;
>  
>  	fsec = file->f_security;
>  	isec = file_inode(file)->i_security;
> @@ -3514,6 +3576,13 @@ static int selinux_file_open(struct file *file, const struct cred *cred)
>  	 * new inode label or new policy.
>  	 * This check is not redundant - do not remove.
>  	 */
> +
> +	if (d_inode(file->f_path.dentry) != file->f_inode) {
> +		rc = selinux_file_open_union(file, fsec, cred);
> +		if (rc < 0)
> +			return rc;
> +	}
> +
>  	return file_path_has_perm(cred, file, open_file_to_av(file));
>  }
>  
> diff --git a/security/selinux/include/objsec.h b/security/selinux/include/objsec.h
> index 81fa718d5cb3..f088c080aa9e 100644
> --- a/security/selinux/include/objsec.h
> +++ b/security/selinux/include/objsec.h
> @@ -54,6 +54,7 @@ struct file_security_struct {
>  	u32 sid;		/* SID of open file description */
>  	u32 fown_sid;		/* SID of file owner (for SIGIO) */
>  	u32 isid;		/* SID of inode at the time of file open */
> +	u32 union_isid;		/* SID of would-be inodes in union top (or 0) */
>  	u32 pseqno;		/* Policy seqno at the time of file open */
>  };
>  
> 
> 

Re: [PATCH 6/8] SELinux: Handle opening of a unioned file

[David Howells]

Stephen Smalley <sds@tycho.nsa.gov> wrote:

> > +	/* We need to check that the union file is allowed to be opened as well
> > +	 * as checking that the lower file is allowed to be opened.
> 
> Hmm...so if I try to open a file for write access, then we are going to
> require that the process be allowed to write to both the union/overlay
> inode and to the lower inode?  That seems problematic for the containers
> use case where no write access will be granted to the lower files.

Actually, this comment should probably be deleted.  I am currently thinking
that access through the overlay fs should only be mediated by the label on the
overlay inode and should not involve the lower inode.

Possibly, then the lower file label should be reckoned against the label of
whoever created the *mount*.

David

Re: [PATCH 6/8] SELinux: Handle opening of a unioned file

[David Howells]

Stephen Smalley <sds@tycho.nsa.gov> wrote:

> > +	/* We need to check that the union file is allowed to be opened as well
> > +	 * as checking that the lower file is allowed to be opened.
> 
> Hmm...so if I try to open a file for write access, then we are going to
> require that the process be allowed to write to both the union/overlay
> inode and to the lower inode?  That seems problematic for the containers
> use case where no write access will be granted to the lower files.

Actually, this comment should probably be deleted.  I am currently thinking
that access through the overlay fs should only be mediated by the label on the
overlay inode and should not involve the lower inode.

Possibly, then the lower file label should be reckoned against the label of
whoever created the *mount*.

David

[PATCH 7/8] SELinux: Create a common helper to determine an inode label

[David Howells]

Create a common helper function to determine the label for a new inode.  This
is then used by:

	- may_create()
	- selinux_dentry_init_security()
	- selinux_inode_init_security()
	- selinux_file_open_union()

This will change the behaviour of the first two functions slightly, bringing
them into line with the third.  The fourth function is newly created in a
preceding patch and applied only in the case of a filesystem union or overlay.

Suggested-by: Stephen Smalley <sds@tycho.nsa.gov>
Signed-off-by: David Howells <dhowells@redhat.com>
---

 security/selinux/hooks.c |  124 ++++++++++++++++++++++------------------------
 1 file changed, 60 insertions(+), 64 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index c4495a797eb1..7eef1032c11a 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -1684,6 +1684,42 @@ out:
 	return rc;
 }
 
+/*
+ * Determine the label for an inode that might be unioned.
+ */
+static int selinux_determine_inode_label(const struct inode *dir,
+					 const struct qstr *name,
+					 const char *caller,
+					 u16 tclass,
+					 u32 *_new_isid)
+{
+	const struct superblock_security_struct *sbsec;
+	const struct inode_security_struct *dsec;
+	const struct task_security_struct *tsec = current_security();
+	int rc;
+
+	sbsec = dir->i_sb->s_security;
+
+	if ((sbsec->flags & SE_SBINITIALIZED) &&
+		   (sbsec->behavior == SECURITY_FS_USE_MNTPOINT)) {
+		*_new_isid = sbsec->mntpoint_sid;
+	} else if (tsec->create_sid) {
+		*_new_isid = tsec->create_sid;
+	} else {
+		dsec = dir->i_security;
+
+		rc = security_transition_sid(tsec->sid, dsec->sid, tclass,
+					     name, _new_isid);
+		if (rc) {
+			pr_warn("%s:  security_transition_sid failed, rc=%d (name=%*.*s)\n",
+				caller, -rc, name->len, name->len, name->name);
+			return rc;
+		}
+	}
+
+	return 0;
+}
+
 /* Check whether a task can create a file. */
 static int may_create(struct inode *dir,
 		      struct dentry *dentry,
@@ -1700,7 +1736,6 @@ static int may_create(struct inode *dir,
 	sbsec = dir->i_sb->s_security;
 
 	sid = tsec->sid;
-	newsid = tsec->create_sid;
 
 	ad.type = LSM_AUDIT_DATA_DENTRY;
 	ad.u.dentry = dentry;
@@ -1711,12 +1746,10 @@ static int may_create(struct inode *dir,
 	if (rc)
 		return rc;
 
-	if (!newsid || !(sbsec->flags & SBLABEL_MNT)) {
-		rc = security_transition_sid(sid, dsec->sid, tclass,
-					     &dentry->d_name, &newsid);
-		if (rc)
-			return rc;
-	}
+	rc = selinux_determine_inode_label(dir, &dentry->d_name, __func__,
+					   tclass, &newsid);
+	if (rc)
+		return rc;
 
 	rc = avc_has_perm(sid, newsid, tclass, FILE__CREATE, &ad);
 	if (rc)
@@ -2723,32 +2756,14 @@ static int selinux_dentry_init_security(struct dentry *dentry, int mode,
 					struct qstr *name, void **ctx,
 					u32 *ctxlen)
 {
-	const struct cred *cred = current_cred();
-	struct task_security_struct *tsec;
-	struct inode_security_struct *dsec;
-	struct superblock_security_struct *sbsec;
-	struct inode *dir = d_backing_inode(dentry->d_parent);
 	u32 newsid;
 	int rc;
 
-	tsec = cred->security;
-	dsec = dir->i_security;
-	sbsec = dir->i_sb->s_security;
-
-	if (tsec->create_sid && sbsec->behavior != SECURITY_FS_USE_MNTPOINT) {
-		newsid = tsec->create_sid;
-	} else {
-		rc = security_transition_sid(tsec->sid, dsec->sid,
-					     inode_mode_to_security_class(mode),
-					     name,
-					     &newsid);
-		if (rc) {
-			printk(KERN_WARNING
-				"%s: security_transition_sid failed, rc=%d\n",
-			       __func__, -rc);
-			return rc;
-		}
-	}
+	rc = selinux_determine_inode_label(d_inode(dentry), name, __func__,
+					   inode_mode_to_security_class(mode),
+					   &newsid);
+	if (rc)
+		return rc;
 
 	return security_sid_to_context(newsid, (char **)ctx, ctxlen);
 }
@@ -2771,22 +2786,12 @@ static int selinux_inode_init_security(struct inode *inode, struct inode *dir,
 	sid = tsec->sid;
 	newsid = tsec->create_sid;
 
-	if ((sbsec->flags & SE_SBINITIALIZED) &&
-	    (sbsec->behavior == SECURITY_FS_USE_MNTPOINT))
-		newsid = sbsec->mntpoint_sid;
-	else if (!newsid || !(sbsec->flags & SBLABEL_MNT)) {
-		rc = security_transition_sid(sid, dsec->sid,
-					     inode_mode_to_security_class(inode->i_mode),
-					     qstr, &newsid);
-		if (rc) {
-			printk(KERN_WARNING "%s:  "
-			       "security_transition_sid failed, rc=%d (dev=%s "
-			       "ino=%ld)\n",
-			       __func__,
-			       -rc, inode->i_sb->s_id, inode->i_ino);
-			return rc;
-		}
-	}
+	rc = selinux_determine_inode_label(
+		dir, qstr, __func__,
+		inode_mode_to_security_class(inode->i_mode),
+		&newsid);
+	if (rc)
+		return rc;
 
 	/* Possibly defer initialization to selinux_complete_init. */
 	if (sbsec->flags & SE_SBINITIALIZED) {
@@ -3502,9 +3507,7 @@ static int selinux_file_open_union(struct file *file,
 				   struct file_security_struct *fsec,
 				   const struct cred *cred)
 {
-	const struct superblock_security_struct *sbsec;
-	const struct inode_security_struct *isec, *dsec, *fisec;
-	const struct task_security_struct *tsec = current_security();
+	const struct inode_security_struct *isec, *fisec;
 	struct common_audit_data ad;
 	struct dentry *union_dentry = file->f_path.dentry;
 	const struct inode *union_inode = d_inode(union_dentry);
@@ -3512,29 +3515,22 @@ static int selinux_file_open_union(struct file *file,
 	struct dentry *dir;
 	int rc;
 
-	sbsec = union_dentry->d_sb->s_security;
-
 	if (union_inode) {
+		/* If we're opening an overlay inode, use the label from that
+		 * in preference to the label on a lower inode which we might
+		 * actually be opening.
+		 */
 		isec = union_inode->i_security;
 		fsec->union_isid = isec->sid;
-	} else if ((sbsec->flags & SE_SBINITIALIZED) &&
-		   (sbsec->behavior == SECURITY_FS_USE_MNTPOINT)) {
-		fsec->union_isid = sbsec->mntpoint_sid;
 	} else {
 		dir = dget_parent(union_dentry);
-		dsec = d_inode(dir)->i_security;
-
-		rc = security_transition_sid(
-			tsec->sid, dsec->sid,
-			inode_mode_to_security_class(lower_inode->i_mode),
+		rc = selinux_determine_inode_label(
+			d_inode(dir),
 			&union_dentry->d_name,
+			__func__,
+			inode_mode_to_security_class(lower_inode->i_mode),
 			&fsec->union_isid);
 		dput(dir);
-		if (rc) {
-			pr_warn("%s:  security_transition_sid failed, rc=%d (name=%pD)\n",
-				__func__, -rc, file);
-			return rc;
-		}
 	}
 
 	/* We need to check that the union file is allowed to be opened as well

Re: [PATCH 7/8] SELinux: Create a common helper to determine an inode label

[Stephen Smalley]

On 06/18/2015 09:33 AM, David Howells wrote:
> Create a common helper function to determine the label for a new inode.  This
> is then used by:
> 
> 	- may_create()
> 	- selinux_dentry_init_security()
> 	- selinux_inode_init_security()
> 	- selinux_file_open_union()
> 
> This will change the behaviour of the first two functions slightly, bringing
> them into line with the third.  The fourth function is newly created in a
> preceding patch and applied only in the case of a filesystem union or overlay.
> 
> Suggested-by: Stephen Smalley <sds@tycho.nsa.gov>
> Signed-off-by: David Howells <dhowells@redhat.com>
> ---
> 
>  security/selinux/hooks.c |  124 ++++++++++++++++++++++------------------------
>  1 file changed, 60 insertions(+), 64 deletions(-)
> 
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index c4495a797eb1..7eef1032c11a 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -1684,6 +1684,42 @@ out:
>  	return rc;
>  }
>  
> +/*
> + * Determine the label for an inode that might be unioned.
> + */
> +static int selinux_determine_inode_label(const struct inode *dir,
> +					 const struct qstr *name,
> +					 const char *caller,
> +					 u16 tclass,
> +					 u32 *_new_isid)
> +{
> +	const struct superblock_security_struct *sbsec;
> +	const struct inode_security_struct *dsec;
> +	const struct task_security_struct *tsec = current_security();
> +	int rc;
> +
> +	sbsec = dir->i_sb->s_security;
> +
> +	if ((sbsec->flags & SE_SBINITIALIZED) &&
> +		   (sbsec->behavior == SECURITY_FS_USE_MNTPOINT)) {
> +		*_new_isid = sbsec->mntpoint_sid;
> +	} else if (tsec->create_sid) {

This doesn't quite match the logic in inode_init_security today, see its
checking of SBLABEL_MNT.

> +		*_new_isid = tsec->create_sid;
> +	} else {
> +		dsec = dir->i_security;
> +
> +		rc = security_transition_sid(tsec->sid, dsec->sid, tclass,
> +					     name, _new_isid);
> +		if (rc) {
> +			pr_warn("%s:  security_transition_sid failed, rc=%d (name=%*.*s)\n",
> +				caller, -rc, name->len, name->len, name->name);

Let's drop this pr_warn call.

> +			return rc;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  /* Check whether a task can create a file. */
>  static int may_create(struct inode *dir,
>  		      struct dentry *dentry,
> @@ -1700,7 +1736,6 @@ static int may_create(struct inode *dir,
>  	sbsec = dir->i_sb->s_security;
>  
>  	sid = tsec->sid;
> -	newsid = tsec->create_sid;
>  
>  	ad.type = LSM_AUDIT_DATA_DENTRY;
>  	ad.u.dentry = dentry;
> @@ -1711,12 +1746,10 @@ static int may_create(struct inode *dir,
>  	if (rc)
>  		return rc;
>  
> -	if (!newsid || !(sbsec->flags & SBLABEL_MNT)) {
> -		rc = security_transition_sid(sid, dsec->sid, tclass,
> -					     &dentry->d_name, &newsid);
> -		if (rc)
> -			return rc;
> -	}
> +	rc = selinux_determine_inode_label(dir, &dentry->d_name, __func__,
> +					   tclass, &newsid);
> +	if (rc)
> +		return rc;
>  
>  	rc = avc_has_perm(sid, newsid, tclass, FILE__CREATE, &ad);
>  	if (rc)
> @@ -2723,32 +2756,14 @@ static int selinux_dentry_init_security(struct dentry *dentry, int mode,
>  					struct qstr *name, void **ctx,
>  					u32 *ctxlen)
>  {
> -	const struct cred *cred = current_cred();
> -	struct task_security_struct *tsec;
> -	struct inode_security_struct *dsec;
> -	struct superblock_security_struct *sbsec;
> -	struct inode *dir = d_backing_inode(dentry->d_parent);
>  	u32 newsid;
>  	int rc;
>  
> -	tsec = cred->security;
> -	dsec = dir->i_security;
> -	sbsec = dir->i_sb->s_security;
> -
> -	if (tsec->create_sid && sbsec->behavior != SECURITY_FS_USE_MNTPOINT) {
> -		newsid = tsec->create_sid;
> -	} else {
> -		rc = security_transition_sid(tsec->sid, dsec->sid,
> -					     inode_mode_to_security_class(mode),
> -					     name,
> -					     &newsid);
> -		if (rc) {
> -			printk(KERN_WARNING
> -				"%s: security_transition_sid failed, rc=%d\n",
> -			       __func__, -rc);
> -			return rc;
> -		}
> -	}
> +	rc = selinux_determine_inode_label(d_inode(dentry), name, __func__,
> +					   inode_mode_to_security_class(mode),
> +					   &newsid);
> +	if (rc)
> +		return rc;
>  
>  	return security_sid_to_context(newsid, (char **)ctx, ctxlen);
>  }
> @@ -2771,22 +2786,12 @@ static int selinux_inode_init_security(struct inode *inode, struct inode *dir,
>  	sid = tsec->sid;
>  	newsid = tsec->create_sid;
>  
> -	if ((sbsec->flags & SE_SBINITIALIZED) &&
> -	    (sbsec->behavior == SECURITY_FS_USE_MNTPOINT))
> -		newsid = sbsec->mntpoint_sid;
> -	else if (!newsid || !(sbsec->flags & SBLABEL_MNT)) {
> -		rc = security_transition_sid(sid, dsec->sid,
> -					     inode_mode_to_security_class(inode->i_mode),
> -					     qstr, &newsid);
> -		if (rc) {
> -			printk(KERN_WARNING "%s:  "
> -			       "security_transition_sid failed, rc=%d (dev=%s "
> -			       "ino=%ld)\n",
> -			       __func__,
> -			       -rc, inode->i_sb->s_id, inode->i_ino);
> -			return rc;
> -		}
> -	}
> +	rc = selinux_determine_inode_label(
> +		dir, qstr, __func__,
> +		inode_mode_to_security_class(inode->i_mode),
> +		&newsid);
> +	if (rc)
> +		return rc;
>  
>  	/* Possibly defer initialization to selinux_complete_init. */
>  	if (sbsec->flags & SE_SBINITIALIZED) {
> @@ -3502,9 +3507,7 @@ static int selinux_file_open_union(struct file *file,
>  				   struct file_security_struct *fsec,
>  				   const struct cred *cred)
>  {
> -	const struct superblock_security_struct *sbsec;
> -	const struct inode_security_struct *isec, *dsec, *fisec;
> -	const struct task_security_struct *tsec = current_security();
> +	const struct inode_security_struct *isec, *fisec;
>  	struct common_audit_data ad;
>  	struct dentry *union_dentry = file->f_path.dentry;
>  	const struct inode *union_inode = d_inode(union_dentry);
> @@ -3512,29 +3515,22 @@ static int selinux_file_open_union(struct file *file,
>  	struct dentry *dir;
>  	int rc;
>  
> -	sbsec = union_dentry->d_sb->s_security;
> -
>  	if (union_inode) {
> +		/* If we're opening an overlay inode, use the label from that
> +		 * in preference to the label on a lower inode which we might
> +		 * actually be opening.
> +		 */
>  		isec = union_inode->i_security;
>  		fsec->union_isid = isec->sid;
> -	} else if ((sbsec->flags & SE_SBINITIALIZED) &&
> -		   (sbsec->behavior == SECURITY_FS_USE_MNTPOINT)) {
> -		fsec->union_isid = sbsec->mntpoint_sid;
>  	} else {
>  		dir = dget_parent(union_dentry);
> -		dsec = d_inode(dir)->i_security;
> -
> -		rc = security_transition_sid(
> -			tsec->sid, dsec->sid,
> -			inode_mode_to_security_class(lower_inode->i_mode),
> +		rc = selinux_determine_inode_label(
> +			d_inode(dir),
>  			&union_dentry->d_name,
> +			__func__,
> +			inode_mode_to_security_class(lower_inode->i_mode),
>  			&fsec->union_isid);
>  		dput(dir);
> -		if (rc) {
> -			pr_warn("%s:  security_transition_sid failed, rc=%d (name=%pD)\n",
> -				__func__, -rc, file);
> -			return rc;
> -		}
>  	}
>  
>  	/* We need to check that the union file is allowed to be opened as well
> 
> 

Re: [PATCH 7/8] SELinux: Create a common helper to determine an inode label

[Stephen Smalley]

On 06/18/2015 09:33 AM, David Howells wrote:
> Create a common helper function to determine the label for a new inode.  This
> is then used by:
> 
> 	- may_create()
> 	- selinux_dentry_init_security()
> 	- selinux_inode_init_security()
> 	- selinux_file_open_union()
> 
> This will change the behaviour of the first two functions slightly, bringing
> them into line with the third.  The fourth function is newly created in a
> preceding patch and applied only in the case of a filesystem union or overlay.
> 
> Suggested-by: Stephen Smalley <sds@tycho.nsa.gov>
> Signed-off-by: David Howells <dhowells@redhat.com>
> ---
> 
>  security/selinux/hooks.c |  124 ++++++++++++++++++++++------------------------
>  1 file changed, 60 insertions(+), 64 deletions(-)
> 
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index c4495a797eb1..7eef1032c11a 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -1684,6 +1684,42 @@ out:
>  	return rc;
>  }
>  
> +/*
> + * Determine the label for an inode that might be unioned.
> + */
> +static int selinux_determine_inode_label(const struct inode *dir,
> +					 const struct qstr *name,
> +					 const char *caller,
> +					 u16 tclass,
> +					 u32 *_new_isid)
> +{
> +	const struct superblock_security_struct *sbsec;
> +	const struct inode_security_struct *dsec;
> +	const struct task_security_struct *tsec = current_security();
> +	int rc;
> +
> +	sbsec = dir->i_sb->s_security;
> +
> +	if ((sbsec->flags & SE_SBINITIALIZED) &&
> +		   (sbsec->behavior == SECURITY_FS_USE_MNTPOINT)) {
> +		*_new_isid = sbsec->mntpoint_sid;
> +	} else if (tsec->create_sid) {

This doesn't quite match the logic in inode_init_security today, see its
checking of SBLABEL_MNT.

> +		*_new_isid = tsec->create_sid;
> +	} else {
> +		dsec = dir->i_security;
> +
> +		rc = security_transition_sid(tsec->sid, dsec->sid, tclass,
> +					     name, _new_isid);
> +		if (rc) {
> +			pr_warn("%s:  security_transition_sid failed, rc=%d (name=%*.*s)\n",
> +				caller, -rc, name->len, name->len, name->name);

Let's drop this pr_warn call.

> +			return rc;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  /* Check whether a task can create a file. */
>  static int may_create(struct inode *dir,
>  		      struct dentry *dentry,
> @@ -1700,7 +1736,6 @@ static int may_create(struct inode *dir,
>  	sbsec = dir->i_sb->s_security;
>  
>  	sid = tsec->sid;
> -	newsid = tsec->create_sid;
>  
>  	ad.type = LSM_AUDIT_DATA_DENTRY;
>  	ad.u.dentry = dentry;
> @@ -1711,12 +1746,10 @@ static int may_create(struct inode *dir,
>  	if (rc)
>  		return rc;
>  
> -	if (!newsid || !(sbsec->flags & SBLABEL_MNT)) {
> -		rc = security_transition_sid(sid, dsec->sid, tclass,
> -					     &dentry->d_name, &newsid);
> -		if (rc)
> -			return rc;
> -	}
> +	rc = selinux_determine_inode_label(dir, &dentry->d_name, __func__,
> +					   tclass, &newsid);
> +	if (rc)
> +		return rc;
>  
>  	rc = avc_has_perm(sid, newsid, tclass, FILE__CREATE, &ad);
>  	if (rc)
> @@ -2723,32 +2756,14 @@ static int selinux_dentry_init_security(struct dentry *dentry, int mode,
>  					struct qstr *name, void **ctx,
>  					u32 *ctxlen)
>  {
> -	const struct cred *cred = current_cred();
> -	struct task_security_struct *tsec;
> -	struct inode_security_struct *dsec;
> -	struct superblock_security_struct *sbsec;
> -	struct inode *dir = d_backing_inode(dentry->d_parent);
>  	u32 newsid;
>  	int rc;
>  
> -	tsec = cred->security;
> -	dsec = dir->i_security;
> -	sbsec = dir->i_sb->s_security;
> -
> -	if (tsec->create_sid && sbsec->behavior != SECURITY_FS_USE_MNTPOINT) {
> -		newsid = tsec->create_sid;
> -	} else {
> -		rc = security_transition_sid(tsec->sid, dsec->sid,
> -					     inode_mode_to_security_class(mode),
> -					     name,
> -					     &newsid);
> -		if (rc) {
> -			printk(KERN_WARNING
> -				"%s: security_transition_sid failed, rc=%d\n",
> -			       __func__, -rc);
> -			return rc;
> -		}
> -	}
> +	rc = selinux_determine_inode_label(d_inode(dentry), name, __func__,
> +					   inode_mode_to_security_class(mode),
> +					   &newsid);
> +	if (rc)
> +		return rc;
>  
>  	return security_sid_to_context(newsid, (char **)ctx, ctxlen);
>  }
> @@ -2771,22 +2786,12 @@ static int selinux_inode_init_security(struct inode *inode, struct inode *dir,
>  	sid = tsec->sid;
>  	newsid = tsec->create_sid;
>  
> -	if ((sbsec->flags & SE_SBINITIALIZED) &&
> -	    (sbsec->behavior == SECURITY_FS_USE_MNTPOINT))
> -		newsid = sbsec->mntpoint_sid;
> -	else if (!newsid || !(sbsec->flags & SBLABEL_MNT)) {
> -		rc = security_transition_sid(sid, dsec->sid,
> -					     inode_mode_to_security_class(inode->i_mode),
> -					     qstr, &newsid);
> -		if (rc) {
> -			printk(KERN_WARNING "%s:  "
> -			       "security_transition_sid failed, rc=%d (dev=%s "
> -			       "ino=%ld)\n",
> -			       __func__,
> -			       -rc, inode->i_sb->s_id, inode->i_ino);
> -			return rc;
> -		}
> -	}
> +	rc = selinux_determine_inode_label(
> +		dir, qstr, __func__,
> +		inode_mode_to_security_class(inode->i_mode),
> +		&newsid);
> +	if (rc)
> +		return rc;
>  
>  	/* Possibly defer initialization to selinux_complete_init. */
>  	if (sbsec->flags & SE_SBINITIALIZED) {
> @@ -3502,9 +3507,7 @@ static int selinux_file_open_union(struct file *file,
>  				   struct file_security_struct *fsec,
>  				   const struct cred *cred)
>  {
> -	const struct superblock_security_struct *sbsec;
> -	const struct inode_security_struct *isec, *dsec, *fisec;
> -	const struct task_security_struct *tsec = current_security();
> +	const struct inode_security_struct *isec, *fisec;
>  	struct common_audit_data ad;
>  	struct dentry *union_dentry = file->f_path.dentry;
>  	const struct inode *union_inode = d_inode(union_dentry);
> @@ -3512,29 +3515,22 @@ static int selinux_file_open_union(struct file *file,
>  	struct dentry *dir;
>  	int rc;
>  
> -	sbsec = union_dentry->d_sb->s_security;
> -
>  	if (union_inode) {
> +		/* If we're opening an overlay inode, use the label from that
> +		 * in preference to the label on a lower inode which we might
> +		 * actually be opening.
> +		 */
>  		isec = union_inode->i_security;
>  		fsec->union_isid = isec->sid;
> -	} else if ((sbsec->flags & SE_SBINITIALIZED) &&
> -		   (sbsec->behavior == SECURITY_FS_USE_MNTPOINT)) {
> -		fsec->union_isid = sbsec->mntpoint_sid;
>  	} else {
>  		dir = dget_parent(union_dentry);
> -		dsec = d_inode(dir)->i_security;
> -
> -		rc = security_transition_sid(
> -			tsec->sid, dsec->sid,
> -			inode_mode_to_security_class(lower_inode->i_mode),
> +		rc = selinux_determine_inode_label(
> +			d_inode(dir),
>  			&union_dentry->d_name,
> +			__func__,
> +			inode_mode_to_security_class(lower_inode->i_mode),
>  			&fsec->union_isid);
>  		dput(dir);
> -		if (rc) {
> -			pr_warn("%s:  security_transition_sid failed, rc=%d (name=%pD)\n",
> -				__func__, -rc, file);
> -			return rc;
> -		}
>  	}
>  
>  	/* We need to check that the union file is allowed to be opened as well
> 
> 

Re: [PATCH 7/8] SELinux: Create a common helper to determine an inode label

[David Howells]

Stephen Smalley <sds@tycho.nsa.gov> wrote:

> > +	if ((sbsec->flags & SE_SBINITIALIZED) &&
> > +		   (sbsec->behavior == SECURITY_FS_USE_MNTPOINT)) {
> > +		*_new_isid = sbsec->mntpoint_sid;
> > +	} else if (tsec->create_sid) {
> 
> This doesn't quite match the logic in inode_init_security today, see its
> checking of SBLABEL_MNT.

Fair point.  What does SBLABEL_MNT mean precisely?  It seems to indicate one
of an odd mix of behaviours.  I presume it means that we *have* to calculate a
label and can't get one from the underlying fs if it is not set.

Also, in:

	sbsec->flags |= SE_SBINITIALIZED;
	if (selinux_is_sblabel_mnt(sb))
		sbsec->flags |= SBLABEL_MNT;

should SE_SBINITIALIZED be set after SBLABEL_MNT?  And should there be a
memory barrier in here somewhere before the setting of SE_SBINITIALIZED?

David

Re: [PATCH 7/8] SELinux: Create a common helper to determine an inode label

[David Howells]

Stephen Smalley <sds@tycho.nsa.gov> wrote:

> > +	if ((sbsec->flags & SE_SBINITIALIZED) &&
> > +		   (sbsec->behavior == SECURITY_FS_USE_MNTPOINT)) {
> > +		*_new_isid = sbsec->mntpoint_sid;
> > +	} else if (tsec->create_sid) {
> 
> This doesn't quite match the logic in inode_init_security today, see its
> checking of SBLABEL_MNT.

Fair point.  What does SBLABEL_MNT mean precisely?  It seems to indicate one
of an odd mix of behaviours.  I presume it means that we *have* to calculate a
label and can't get one from the underlying fs if it is not set.

Also, in:

	sbsec->flags |= SE_SBINITIALIZED;
	if (selinux_is_sblabel_mnt(sb))
		sbsec->flags |= SBLABEL_MNT;

should SE_SBINITIALIZED be set after SBLABEL_MNT?  And should there be a
memory barrier in here somewhere before the setting of SE_SBINITIALIZED?

David

Re: [PATCH 7/8] SELinux: Create a common helper to determine an inode label

[Stephen Smalley]

On 06/18/2015 11:13 AM, David Howells wrote:
> Stephen Smalley <sds@tycho.nsa.gov> wrote:
> 
>>> +	if ((sbsec->flags & SE_SBINITIALIZED) &&
>>> +		   (sbsec->behavior == SECURITY_FS_USE_MNTPOINT)) {
>>> +		*_new_isid = sbsec->mntpoint_sid;
>>> +	} else if (tsec->create_sid) {
>>
>> This doesn't quite match the logic in inode_init_security today, see its
>> checking of SBLABEL_MNT.
> 
> Fair point.  What does SBLABEL_MNT mean precisely?  It seems to indicate one
> of an odd mix of behaviours.  I presume it means that we *have* to calculate a
> label and can't get one from the underlying fs if it is not set.

It means the filesystem supports per-file labeling and you can use
setxattr(..."security.selinux") and setfscreatecon() for files on it.
You can see whether it is set on a filesystem by looking for the
seclabel option in cat /proc/mounts.  If it is not set, then we ignore
tsec->create_sid.  It is arguable as to whether it is correct to always
call security_transition_sid() there either, but that's another topic.

> 
> Also, in:
> 
> 	sbsec->flags |= SE_SBINITIALIZED;
> 	if (selinux_is_sblabel_mnt(sb))
> 		sbsec->flags |= SBLABEL_MNT;
> 
> should SE_SBINITIALIZED be set after SBLABEL_MNT?  And should there be a
> memory barrier in here somewhere before the setting of SE_SBINITIALIZED?

I believe that's all under sbsec->lock held by the caller, but that code
has changed a lot and been refactored significantly by others.

Re: [PATCH 7/8] SELinux: Create a common helper to determine an inode label

[Stephen Smalley]

On 06/18/2015 11:13 AM, David Howells wrote:
> Stephen Smalley <sds@tycho.nsa.gov> wrote:
> 
>>> +	if ((sbsec->flags & SE_SBINITIALIZED) &&
>>> +		   (sbsec->behavior == SECURITY_FS_USE_MNTPOINT)) {
>>> +		*_new_isid = sbsec->mntpoint_sid;
>>> +	} else if (tsec->create_sid) {
>>
>> This doesn't quite match the logic in inode_init_security today, see its
>> checking of SBLABEL_MNT.
> 
> Fair point.  What does SBLABEL_MNT mean precisely?  It seems to indicate one
> of an odd mix of behaviours.  I presume it means that we *have* to calculate a
> label and can't get one from the underlying fs if it is not set.

It means the filesystem supports per-file labeling and you can use
setxattr(..."security.selinux") and setfscreatecon() for files on it.
You can see whether it is set on a filesystem by looking for the
seclabel option in cat /proc/mounts.  If it is not set, then we ignore
tsec->create_sid.  It is arguable as to whether it is correct to always
call security_transition_sid() there either, but that's another topic.

> 
> Also, in:
> 
> 	sbsec->flags |= SE_SBINITIALIZED;
> 	if (selinux_is_sblabel_mnt(sb))
> 		sbsec->flags |= SBLABEL_MNT;
> 
> should SE_SBINITIALIZED be set after SBLABEL_MNT?  And should there be a
> memory barrier in here somewhere before the setting of SE_SBINITIALIZED?

I believe that's all under sbsec->lock held by the caller, but that code
has changed a lot and been refactored significantly by others.

Re: [PATCH 7/8] SELinux: Create a common helper to determine an inode label

[David Howells]

Stephen Smalley <sds@tycho.nsa.gov> wrote:

> > Fair point.  What does SBLABEL_MNT mean precisely?  It seems to indicate one
> > of an odd mix of behaviours.  I presume it means that we *have* to calculate a
> > label and can't get one from the underlying fs if it is not set.
> 
> It means the filesystem supports per-file labeling and you can use
> setxattr(..."security.selinux") and setfscreatecon() for files on it.
> You can see whether it is set on a filesystem by looking for the
> seclabel option in cat /proc/mounts.  If it is not set, then we ignore
> tsec->create_sid.  It is arguable as to whether it is correct to always
> call security_transition_sid() there either, but that's another topic.

Okay, so how about the attached?

David
---
static int selinux_determine_inode_label(const struct inode *dir,
					 const struct qstr *name,
					 const char *caller,
					 u16 tclass,
					 u32 *_new_isid)
{
	const struct superblock_security_struct *sbsec = dir->i_sb->s_security;
	const struct inode_security_struct *dsec = dir->i_security;
	const struct task_security_struct *tsec = current_security();

	if ((sbsec->flags & SE_SBINITIALIZED) &&
	    (sbsec->behavior == SECURITY_FS_USE_MNTPOINT)) {
		*_new_isid = sbsec->mntpoint_sid;
	} else if ((sbsec->flags & SBLABEL_MNT) &&
		   tsec->create_sid) {
		*_new_isid = tsec->create_sid;
	} else {
		return security_transition_sid(tsec->sid, dsec->sid, tclass,
					       name, _new_isid);
	}

	return 0;
}

Re: [PATCH 7/8] SELinux: Create a common helper to determine an inode label

[David Howells]

Stephen Smalley <sds@tycho.nsa.gov> wrote:

> > Fair point.  What does SBLABEL_MNT mean precisely?  It seems to indicate one
> > of an odd mix of behaviours.  I presume it means that we *have* to calculate a
> > label and can't get one from the underlying fs if it is not set.
> 
> It means the filesystem supports per-file labeling and you can use
> setxattr(..."security.selinux") and setfscreatecon() for files on it.
> You can see whether it is set on a filesystem by looking for the
> seclabel option in cat /proc/mounts.  If it is not set, then we ignore
> tsec->create_sid.  It is arguable as to whether it is correct to always
> call security_transition_sid() there either, but that's another topic.

Okay, so how about the attached?

David
---
static int selinux_determine_inode_label(const struct inode *dir,
					 const struct qstr *name,
					 const char *caller,
					 u16 tclass,
					 u32 *_new_isid)
{
	const struct superblock_security_struct *sbsec = dir->i_sb->s_security;
	const struct inode_security_struct *dsec = dir->i_security;
	const struct task_security_struct *tsec = current_security();

	if ((sbsec->flags & SE_SBINITIALIZED) &&
	    (sbsec->behavior == SECURITY_FS_USE_MNTPOINT)) {
		*_new_isid = sbsec->mntpoint_sid;
	} else if ((sbsec->flags & SBLABEL_MNT) &&
		   tsec->create_sid) {
		*_new_isid = tsec->create_sid;
	} else {
		return security_transition_sid(tsec->sid, dsec->sid, tclass,
					       name, _new_isid);
	}

	return 0;
}

Re: [PATCH 7/8] SELinux: Create a common helper to determine an inode label

[Stephen Smalley]

On 06/18/2015 11:32 AM, David Howells wrote:
> Stephen Smalley <sds@tycho.nsa.gov> wrote:
> 
>>> Fair point.  What does SBLABEL_MNT mean precisely?  It seems to indicate one
>>> of an odd mix of behaviours.  I presume it means that we *have* to calculate a
>>> label and can't get one from the underlying fs if it is not set.
>>
>> It means the filesystem supports per-file labeling and you can use
>> setxattr(..."security.selinux") and setfscreatecon() for files on it.
>> You can see whether it is set on a filesystem by looking for the
>> seclabel option in cat /proc/mounts.  If it is not set, then we ignore
>> tsec->create_sid.  It is arguable as to whether it is correct to always
>> call security_transition_sid() there either, but that's another topic.
> 
> Okay, so how about the attached?
> 
> David
> ---
> static int selinux_determine_inode_label(const struct inode *dir,
> 					 const struct qstr *name,
> 					 const char *caller,
> 					 u16 tclass,
> 					 u32 *_new_isid)
> {
> 	const struct superblock_security_struct *sbsec = dir->i_sb->s_security;
> 	const struct inode_security_struct *dsec = dir->i_security;
> 	const struct task_security_struct *tsec = current_security();
> 
> 	if ((sbsec->flags & SE_SBINITIALIZED) &&
> 	    (sbsec->behavior == SECURITY_FS_USE_MNTPOINT)) {
> 		*_new_isid = sbsec->mntpoint_sid;
> 	} else if ((sbsec->flags & SBLABEL_MNT) &&
> 		   tsec->create_sid) {
> 		*_new_isid = tsec->create_sid;
> 	} else {
> 		return security_transition_sid(tsec->sid, dsec->sid, tclass,
> 					       name, _new_isid);
> 	}
> 
> 	return 0;
> }

That looks good to me.  In fact, I'd take a patch that defines that
function and rewrites may_create(), inode_init_security(), and
dentry_init_security() to use it even before (or independent of) the
union support patches.

Re: [PATCH 7/8] SELinux: Create a common helper to determine an inode label

[Stephen Smalley]

On 06/18/2015 11:32 AM, David Howells wrote:
> Stephen Smalley <sds@tycho.nsa.gov> wrote:
> 
>>> Fair point.  What does SBLABEL_MNT mean precisely?  It seems to indicate one
>>> of an odd mix of behaviours.  I presume it means that we *have* to calculate a
>>> label and can't get one from the underlying fs if it is not set.
>>
>> It means the filesystem supports per-file labeling and you can use
>> setxattr(..."security.selinux") and setfscreatecon() for files on it.
>> You can see whether it is set on a filesystem by looking for the
>> seclabel option in cat /proc/mounts.  If it is not set, then we ignore
>> tsec->create_sid.  It is arguable as to whether it is correct to always
>> call security_transition_sid() there either, but that's another topic.
> 
> Okay, so how about the attached?
> 
> David
> ---
> static int selinux_determine_inode_label(const struct inode *dir,
> 					 const struct qstr *name,
> 					 const char *caller,
> 					 u16 tclass,
> 					 u32 *_new_isid)
> {
> 	const struct superblock_security_struct *sbsec = dir->i_sb->s_security;
> 	const struct inode_security_struct *dsec = dir->i_security;
> 	const struct task_security_struct *tsec = current_security();
> 
> 	if ((sbsec->flags & SE_SBINITIALIZED) &&
> 	    (sbsec->behavior == SECURITY_FS_USE_MNTPOINT)) {
> 		*_new_isid = sbsec->mntpoint_sid;
> 	} else if ((sbsec->flags & SBLABEL_MNT) &&
> 		   tsec->create_sid) {
> 		*_new_isid = tsec->create_sid;
> 	} else {
> 		return security_transition_sid(tsec->sid, dsec->sid, tclass,
> 					       name, _new_isid);
> 	}
> 
> 	return 0;
> }

That looks good to me.  In fact, I'd take a patch that defines that
function and rewrites may_create(), inode_init_security(), and
dentry_init_security() to use it even before (or independent of) the
union support patches.

Re: [PATCH 7/8] SELinux: Create a common helper to determine an inode label

[Stephen Smalley]

On 06/18/2015 11:32 AM, David Howells wrote:
> Stephen Smalley <sds@tycho.nsa.gov> wrote:
> 
>>> Fair point.  What does SBLABEL_MNT mean precisely?  It seems to indicate one
>>> of an odd mix of behaviours.  I presume it means that we *have* to calculate a
>>> label and can't get one from the underlying fs if it is not set.
>>
>> It means the filesystem supports per-file labeling and you can use
>> setxattr(..."security.selinux") and setfscreatecon() for files on it.
>> You can see whether it is set on a filesystem by looking for the
>> seclabel option in cat /proc/mounts.  If it is not set, then we ignore
>> tsec->create_sid.  It is arguable as to whether it is correct to always
>> call security_transition_sid() there either, but that's another topic.
> 
> Okay, so how about the attached?
> 
> David
> ---
> static int selinux_determine_inode_label(const struct inode *dir,
> 					 const struct qstr *name,
> 					 const char *caller,
> 					 u16 tclass,
> 					 u32 *_new_isid)
> {
> 	const struct superblock_security_struct *sbsec = dir->i_sb->s_security;
> 	const struct inode_security_struct *dsec = dir->i_security;
> 	const struct task_security_struct *tsec = current_security();
> 
> 	if ((sbsec->flags & SE_SBINITIALIZED) &&
> 	    (sbsec->behavior == SECURITY_FS_USE_MNTPOINT)) {
> 		*_new_isid = sbsec->mntpoint_sid;
> 	} else if ((sbsec->flags & SBLABEL_MNT) &&
> 		   tsec->create_sid) {
> 		*_new_isid = tsec->create_sid;
> 	} else {
> 		return security_transition_sid(tsec->sid, dsec->sid, tclass,
> 					       name, _new_isid);
> 	}
> 
> 	return 0;
> }

Except you can drop the caller argument now.

Re: [PATCH 7/8] SELinux: Create a common helper to determine an inode label

[Stephen Smalley]

On 06/18/2015 11:32 AM, David Howells wrote:
> Stephen Smalley <sds@tycho.nsa.gov> wrote:
> 
>>> Fair point.  What does SBLABEL_MNT mean precisely?  It seems to indicate one
>>> of an odd mix of behaviours.  I presume it means that we *have* to calculate a
>>> label and can't get one from the underlying fs if it is not set.
>>
>> It means the filesystem supports per-file labeling and you can use
>> setxattr(..."security.selinux") and setfscreatecon() for files on it.
>> You can see whether it is set on a filesystem by looking for the
>> seclabel option in cat /proc/mounts.  If it is not set, then we ignore
>> tsec->create_sid.  It is arguable as to whether it is correct to always
>> call security_transition_sid() there either, but that's another topic.
> 
> Okay, so how about the attached?
> 
> David
> ---
> static int selinux_determine_inode_label(const struct inode *dir,
> 					 const struct qstr *name,
> 					 const char *caller,
> 					 u16 tclass,
> 					 u32 *_new_isid)
> {
> 	const struct superblock_security_struct *sbsec = dir->i_sb->s_security;
> 	const struct inode_security_struct *dsec = dir->i_security;
> 	const struct task_security_struct *tsec = current_security();
> 
> 	if ((sbsec->flags & SE_SBINITIALIZED) &&
> 	    (sbsec->behavior == SECURITY_FS_USE_MNTPOINT)) {
> 		*_new_isid = sbsec->mntpoint_sid;
> 	} else if ((sbsec->flags & SBLABEL_MNT) &&
> 		   tsec->create_sid) {
> 		*_new_isid = tsec->create_sid;
> 	} else {
> 		return security_transition_sid(tsec->sid, dsec->sid, tclass,
> 					       name, _new_isid);
> 	}
> 
> 	return 0;
> }

Except you can drop the caller argument now.

[PATCH 8/8] SELinux: Check against union label for file operations

[David Howells]

File operations (eg. read, write) issued against a file that is attached to
the lower layer of a union file needs to be checked against the union-layer
label not the lower layer label.

The union label is stored in the file_security_struct rather than being
retrieved from one of the inodes.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 security/selinux/hooks.c |   12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 7eef1032c11a..9d954ad8b510 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -1657,6 +1657,7 @@ static int file_has_perm(const struct cred *cred,
 			 struct file *file,
 			 u32 av)
 {
+	struct inode_security_struct *isec;
 	struct file_security_struct *fsec = file->f_security;
 	struct inode *inode = file_inode(file);
 	struct common_audit_data ad;
@@ -1677,8 +1678,15 @@ static int file_has_perm(const struct cred *cred,
 
 	/* av is zero if only checking access to the descriptor. */
 	rc = 0;
-	if (av)
-		rc = inode_has_perm(cred, inode, av, &ad);
+	if (av && likely(!IS_PRIVATE(inode))) {
+		if (fsec->union_isid) {
+			isec = inode->i_security;
+			rc = avc_has_perm(sid, fsec->union_isid, isec->sclass,
+					  av, &ad);
+		}
+		if (!rc)
+			rc = inode_has_perm(cred, inode, av, &ad);
+	}
 
 out:
 	return rc;

Re: [PATCH 0/8] Security: Provide unioned file support

[Al Viro]

On Thu, Jun 18, 2015 at 02:32:15PM +0100, David Howells wrote:
> 
> The attached patches provide security support for unioned files where the
> security involves an object-label-based LSM (such as SELinux) rather than a
> path-based LSM.
> 
> The patches can be broken down into a number of sets:
> 
>  (1) A small patch to drop a lock earlier in overlayfs.  The main VFS patch
>      touches the same code, so I put this first.
> 
>  (2) The main VFS patch that makes an open file struct referring to a union
>      file have ->f_path point to the union/overlay file whilst ->f_inode and
>      ->f_mapping refer to the subordinate file that does the actual work.

#1 and #2 applied, will be in tomorrow vfs.git#for-next

Re: [PATCH 0/8] Security: Provide unioned file support

[Miklos Szeredi]

On Fri, Jun 19, 2015 at 9:20 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Thu, Jun 18, 2015 at 02:32:15PM +0100, David Howells wrote:
>>
>> The attached patches provide security support for unioned files where the
>> security involves an object-label-based LSM (such as SELinux) rather than a
>> path-based LSM.
>>
>> The patches can be broken down into a number of sets:
>>
>>  (1) A small patch to drop a lock earlier in overlayfs.  The main VFS patch
>>      touches the same code, so I put this first.
>>
>>  (2) The main VFS patch that makes an open file struct referring to a union
>>      file have ->f_path point to the union/overlay file whilst ->f_inode and
>>      ->f_mapping refer to the subordinate file that does the actual work.
>
> #1 and #2 applied, will be in tomorrow vfs.git#for-next

Brave.

What's going to happen to all those f_path.dentry uses where the
filesystem thinks it's getting its own dentry?

> git grep f_path.dentry | wc -l
171

Thanks,
Miklos

Re: [PATCH 0/8] Security: Provide unioned file support

[Al Viro]

On Fri, Jun 19, 2015 at 09:52:55AM +0200, Miklos Szeredi wrote:
> Brave.
> 
> What's going to happen to all those f_path.dentry uses where the
> filesystem thinks it's getting its own dentry?
> 
> > git grep f_path.dentry | wc -l
> 171

How many of those are not for directories *and* not in something like
CIFS or debugfs?

Re: [PATCH 0/8] Security: Provide unioned file support

[Miklos Szeredi]

On Fri, Jun 19, 2015 at 08:59:03AM +0100, Al Viro wrote:
> On Fri, Jun 19, 2015 at 09:52:55AM +0200, Miklos Szeredi wrote:
> > Brave.
> > 
> > What's going to happen to all those f_path.dentry uses where the
> > filesystem thinks it's getting its own dentry?
> > 
> > > git grep f_path.dentry | wc -l
> > 171
> 
> How many of those are not for directories *and* not in something like
> CIFS or debugfs?

A not insignificant number I think.

The problem is nothing will warn about these but obscure crashes.  These will not be well exercised paths, and I think we need more than handwaving to fix them.

I started a patch converting guilty f_path.dentry to file_dentry(), but it's far
from complete.

Thanks,
Miklos

---
 drivers/staging/lustre/lustre/llite/file.c |   12 ++++++------
 fs/9p/vfs_file.c                           |    6 +++---
 fs/btrfs/file.c                            |    2 +-
 fs/btrfs/ioctl.c                           |   13 +++++++------
 fs/ceph/dir.c                              |    6 +++---
 fs/ceph/file.c                             |    2 +-
 fs/cifs/file.c                             |    4 ++--
 fs/cifs/readdir.c                          |    4 ++--
 fs/configfs/dir.c                          |    8 ++++----
 fs/configfs/file.c                         |   15 +++++++++------
 fs/fat/file.c                              |    7 ++++---
 fs/fuse/dir.c                              |    2 +-
 fs/hfsplus/ioctl.c                         |    2 +-
 fs/hostfs/hostfs_kern.c                    |    4 ++--
 fs/hppfs/hppfs.c                           |    4 ++--
 fs/kernfs/dir.c                            |    2 +-
 fs/kernfs/file.c                           |    6 +++---
 fs/libfs.c                                 |    6 +++---
 fs/ncpfs/dir.c                             |    4 ++--
 fs/nfs/dir.c                               |    6 +++---
 fs/nfs/inode.c                             |    2 +-
 fs/nfs/nfs4file.c                          |    4 ++--
 fs/overlayfs/readdir.c                     |   10 +++++-----
 fs/proc/base.c                             |    6 +++---
 fs/proc/proc_sysctl.c                      |    2 +-
 include/linux/fs.h                         |    5 +++++
 26 files changed, 77 insertions(+), 67 deletions(-)

--- a/fs/9p/vfs_file.c
+++ b/fs/9p/vfs_file.c
@@ -74,7 +74,7 @@ int v9fs_file_open(struct inode *inode,
 					v9fs_proto_dotu(v9ses));
 	fid = file->private_data;
 	if (!fid) {
-		fid = v9fs_fid_clone(file->f_path.dentry);
+		fid = v9fs_fid_clone(file_dentry(file));
 		if (IS_ERR(fid))
 			return PTR_ERR(fid);
 
@@ -100,7 +100,7 @@ int v9fs_file_open(struct inode *inode,
 		 * because we want write after unlink usecase
 		 * to work.
 		 */
-		fid = v9fs_writeback_fid(file->f_path.dentry);
+		fid = v9fs_writeback_fid(file_dentry(file));
 		if (IS_ERR(fid)) {
 			err = PTR_ERR(fid);
 			mutex_unlock(&v9inode->v_mutex);
@@ -515,7 +515,7 @@ v9fs_mmap_file_mmap(struct file *filp, s
 		 * because we want write after unlink usecase
 		 * to work.
 		 */
-		fid = v9fs_writeback_fid(filp->f_path.dentry);
+		fid = v9fs_writeback_fid(file_dentry(filp));
 		if (IS_ERR(fid)) {
 			retval = PTR_ERR(fid);
 			mutex_unlock(&v9inode->v_mutex);
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -1861,7 +1861,7 @@ static int start_ordered_ops(struct inod
  */
 int btrfs_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
 {
-	struct dentry *dentry = file->f_path.dentry;
+	struct dentry *dentry = file_dentry(file);
 	struct inode *inode = d_inode(dentry);
 	struct btrfs_root *root = BTRFS_I(inode)->root;
 	struct btrfs_trans_handle *trans;
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -811,13 +811,14 @@ static inline int btrfs_may_create(struc
  * sys_mkdirat and vfs_mkdir, but we only do a single component lookup
  * inside this filesystem so it's quite a bit simpler.
  */
-static noinline int btrfs_mksubvol(struct path *parent,
+static noinline int btrfs_mksubvol(struct file *file,
 				   char *name, int namelen,
 				   struct btrfs_root *snap_src,
 				   u64 *async_transid, bool readonly,
 				   struct btrfs_qgroup_inherit *inherit)
 {
-	struct inode *dir  = d_inode(parent->dentry);
+	struct dentry *parent = file_dentry(file);
+	struct inode *dir  = d_inode(parent);
 	struct dentry *dentry;
 	int error;
 
@@ -825,7 +826,7 @@ static noinline int btrfs_mksubvol(struc
 	if (error == -EINTR)
 		return error;
 
-	dentry = lookup_one_len(name, parent->dentry, namelen);
+	dentry = lookup_one_len(name, parent, namelen);
 	error = PTR_ERR(dentry);
 	if (IS_ERR(dentry))
 		goto out_unlock;
@@ -1623,7 +1624,7 @@ static noinline int btrfs_ioctl_snap_cre
 	}
 
 	if (subvol) {
-		ret = btrfs_mksubvol(&file->f_path, name, namelen,
+		ret = btrfs_mksubvol(file, name, namelen,
 				     NULL, transid, readonly, inherit);
 	} else {
 		struct fd src = fdget(fd);
@@ -1645,7 +1646,7 @@ static noinline int btrfs_ioctl_snap_cre
 			 */
 			ret = -EPERM;
 		} else {
-			ret = btrfs_mksubvol(&file->f_path, name, namelen,
+			ret = btrfs_mksubvol(file, name, namelen,
 					     BTRFS_I(src_inode)->root,
 					     transid, readonly, inherit);
 		}
@@ -2299,7 +2300,7 @@ static noinline int btrfs_ioctl_ino_look
 static noinline int btrfs_ioctl_snap_destroy(struct file *file,
 					     void __user *arg)
 {
-	struct dentry *parent = file->f_path.dentry;
+	struct dentry *parent = file_dentry(file);
 	struct dentry *dentry;
 	struct inode *dir = d_inode(parent);
 	struct inode *inode;
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1997,6 +1997,11 @@ static inline struct inode *file_inode(c
 	return f->f_inode;
 }
 
+static inline struct dentry *file_dentry(const struct file *f)
+{
+	return f->f_path.dentry;
+}
+
 /* /sys/fs */
 extern struct kobject *fs_kobj;
 
--- a/fs/ceph/dir.c
+++ b/fs/ceph/dir.c
@@ -121,7 +121,7 @@ static int __dcache_readdir(struct file
 			    u32 shared_gen)
 {
 	struct ceph_file_info *fi = file->private_data;
-	struct dentry *parent = file->f_path.dentry;
+	struct dentry *parent = file_dentry(file);
 	struct inode *dir = d_inode(parent);
 	struct list_head *p;
 	struct dentry *dentry, *last;
@@ -268,7 +268,7 @@ static int ceph_readdir(struct file *fil
 		off = 1;
 	}
 	if (ctx->pos == 1) {
-		ino_t ino = parent_ino(file->f_path.dentry);
+		ino_t ino = parent_ino(file_dentry(file));
 		dout("readdir off 1 -> '..'\n");
 		if (!dir_emit(ctx, "..", 2,
 			    ceph_translate_ino(inode->i_sb, ino),
@@ -353,7 +353,7 @@ static int ceph_readdir(struct file *fil
 
 		req->r_inode = inode;
 		ihold(inode);
-		req->r_dentry = dget(file->f_path.dentry);
+		req->r_dentry = dget(file_dentry(file));
 		err = ceph_mdsc_do_request(mdsc, NULL, req);
 		if (err < 0) {
 			ceph_mdsc_put_request(req);
--- a/fs/ceph/file.c
+++ b/fs/ceph/file.c
@@ -210,7 +210,7 @@ int ceph_open(struct inode *inode, struc
 
 	req->r_num_caps = 1;
 	if (flags & O_CREAT)
-		parent_inode = ceph_get_dentry_parent_inode(file->f_path.dentry);
+		parent_inode = ceph_get_dentry_parent_inode(file_dentry(file));
 	err = ceph_mdsc_do_request(mdsc, parent_inode, req);
 	iput(parent_inode);
 	if (!err)
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -272,7 +272,7 @@ struct cifsFileInfo *
 cifs_new_fileinfo(struct cifs_fid *fid, struct file *file,
 		  struct tcon_link *tlink, __u32 oplock)
 {
-	struct dentry *dentry = file->f_path.dentry;
+	struct dentry *dentry = file_dentry(file);
 	struct inode *inode = d_inode(dentry);
 	struct cifsInodeInfo *cinode = CIFS_I(inode);
 	struct cifsFileInfo *cfile;
@@ -462,7 +462,7 @@ int cifs_open(struct inode *inode, struc
 	tcon = tlink_tcon(tlink);
 	server = tcon->ses->server;
 
-	full_path = build_path_from_dentry(file->f_path.dentry);
+	full_path = build_path_from_dentry(file_dentry(file));
 	if (full_path == NULL) {
 		rc = -ENOMEM;
 		goto out;
--- a/fs/cifs/readdir.c
+++ b/fs/cifs/readdir.c
@@ -298,7 +298,7 @@ initiate_cifs_search(const unsigned int
 	cifsFile->invalidHandle = true;
 	cifsFile->srch_inf.endOfSearch = false;
 
-	full_path = build_path_from_dentry(file->f_path.dentry);
+	full_path = build_path_from_dentry(file_dentry(file));
 	if (full_path == NULL) {
 		rc = -ENOMEM;
 		goto error_exit;
@@ -757,7 +757,7 @@ static int cifs_filldir(char *find_entry
 		 */
 		fattr.cf_flags |= CIFS_FATTR_NEED_REVAL;
 
-	cifs_prime_dcache(file->f_path.dentry, &name, &fattr);
+	cifs_prime_dcache(file_dentry(file), &name, &fattr);
 
 	ino = cifs_uniqueid_to_ino_t(fattr.cf_uniqueid);
 	return !dir_emit(ctx, name.name, name.len, ino, fattr.cf_dtype);
--- a/fs/configfs/dir.c
+++ b/fs/configfs/dir.c
@@ -1478,7 +1478,7 @@ int configfs_rename_dir(struct config_it
 
 static int configfs_dir_open(struct inode *inode, struct file *file)
 {
-	struct dentry * dentry = file->f_path.dentry;
+	struct dentry *dentry = file_dentry(file);
 	struct configfs_dirent * parent_sd = dentry->d_fsdata;
 	int err;
 
@@ -1502,7 +1502,7 @@ static int configfs_dir_open(struct inod
 
 static int configfs_dir_close(struct inode *inode, struct file *file)
 {
-	struct dentry * dentry = file->f_path.dentry;
+	struct dentry *dentry = file_dentry(file);
 	struct configfs_dirent * cursor = file->private_data;
 
 	mutex_lock(&d_inode(dentry)->i_mutex);
@@ -1524,7 +1524,7 @@ static inline unsigned char dt_type(stru
 
 static int configfs_readdir(struct file *file, struct dir_context *ctx)
 {
-	struct dentry *dentry = file->f_path.dentry;
+	struct dentry *dentry = file_dentry(file);
 	struct super_block *sb = dentry->d_sb;
 	struct configfs_dirent * parent_sd = dentry->d_fsdata;
 	struct configfs_dirent *cursor = file->private_data;
@@ -1588,7 +1588,7 @@ static int configfs_readdir(struct file
 
 static loff_t configfs_dir_lseek(struct file *file, loff_t offset, int whence)
 {
-	struct dentry * dentry = file->f_path.dentry;
+	struct dentry *dentry = file_dentry(file);
 
 	mutex_lock(&d_inode(dentry)->i_mutex);
 	switch (whence) {
--- a/fs/configfs/file.c
+++ b/fs/configfs/file.c
@@ -111,7 +111,8 @@ configfs_read_file(struct file *file, ch
 
 	mutex_lock(&buffer->mutex);
 	if (buffer->needs_read_fill) {
-		if ((retval = fill_read_buffer(file->f_path.dentry,buffer)))
+		retval = fill_read_buffer(file_dentry(file), buffer);
+		if (retval)
 			goto out;
 	}
 	pr_debug("%s: count = %zd, ppos = %lld, buf = %s\n",
@@ -203,7 +204,7 @@ configfs_write_file(struct file *file, c
 	mutex_lock(&buffer->mutex);
 	len = fill_write_buffer(buffer, buf, count);
 	if (len > 0)
-		len = flush_write_buffer(file->f_path.dentry, buffer, len);
+		len = flush_write_buffer(file_dentry(file), buffer, len);
 	if (len > 0)
 		*ppos += len;
 	mutex_unlock(&buffer->mutex);
@@ -212,8 +213,9 @@ configfs_write_file(struct file *file, c
 
 static int check_perm(struct inode * inode, struct file * file)
 {
-	struct config_item *item = configfs_get_config_item(file->f_path.dentry->d_parent);
-	struct configfs_attribute * attr = to_attr(file->f_path.dentry);
+	struct dentry *dentry = file_dentry(file);
+	struct config_item *item = configfs_get_config_item(dentry->d_parent);
+	struct configfs_attribute *attr = to_attr(dentry);
 	struct configfs_buffer * buffer;
 	struct configfs_item_operations * ops = NULL;
 	int error = 0;
@@ -286,8 +288,9 @@ static int configfs_open_file(struct ino
 
 static int configfs_release(struct inode * inode, struct file * filp)
 {
-	struct config_item * item = to_item(filp->f_path.dentry->d_parent);
-	struct configfs_attribute * attr = to_attr(filp->f_path.dentry);
+	struct dentry *dentry = file_dentry(filp);
+	struct config_item *item = to_item(dentry->d_parent);
+	struct configfs_attribute *attr = to_attr(dentry);
 	struct module * owner = attr->ca_owner;
 	struct configfs_buffer * buffer = filp->private_data;
 
--- a/fs/fat/file.c
+++ b/fs/fat/file.c
@@ -28,6 +28,7 @@ static int fat_ioctl_get_attributes(stru
 
 static int fat_ioctl_set_attributes(struct file *file, u32 __user *user_attr)
 {
+	struct dentry *dentry = file_dentry(file);
 	struct inode *inode = file_inode(file);
 	struct msdos_sb_info *sbi = MSDOS_SB(inode->i_sb);
 	int is_dir = S_ISDIR(inode->i_mode);
@@ -84,16 +85,16 @@ static int fat_ioctl_set_attributes(stru
 	 * out the RO attribute for checking by the security
 	 * module, just because it maps to a file mode.
 	 */
-	err = security_inode_setattr(file->f_path.dentry, &ia);
+	err = security_inode_setattr(dentry, &ia);
 	if (err)
 		goto out_unlock_inode;
 
 	/* This MUST be done before doing anything irreversible... */
-	err = fat_setattr(file->f_path.dentry, &ia);
+	err = fat_setattr(dentry, &ia);
 	if (err)
 		goto out_unlock_inode;
 
-	fsnotify_change(file->f_path.dentry, ia.ia_valid);
+	fsnotify_change(dentry, ia.ia_valid);
 	if (sbi->options.sys_immutable) {
 		if (attr & ATTR_SYS)
 			inode->i_flags |= S_IMMUTABLE;
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -1165,7 +1165,7 @@ static int fuse_direntplus_link(struct f
 	int err;
 	struct fuse_entry_out *o = &direntplus->entry_out;
 	struct fuse_dirent *dirent = &direntplus->dirent;
-	struct dentry *parent = file->f_path.dentry;
+	struct dentry *parent = file_dentry(file);
 	struct qstr name = QSTR_INIT(dirent->name, dirent->namelen);
 	struct dentry *dentry;
 	struct dentry *alias;
--- a/fs/hfsplus/ioctl.c
+++ b/fs/hfsplus/ioctl.c
@@ -25,7 +25,7 @@
  */
 static int hfsplus_ioctl_bless(struct file *file, int __user *user_flags)
 {
-	struct dentry *dentry = file->f_path.dentry;
+	struct dentry *dentry = file_dentry(file);
 	struct inode *inode = d_inode(dentry);
 	struct hfsplus_sb_info *sbi = HFSPLUS_SB(inode->i_sb);
 	struct hfsplus_vh *vh = sbi->s_vhdr;
--- a/fs/hostfs/hostfs_kern.c
+++ b/fs/hostfs/hostfs_kern.c
@@ -284,7 +284,7 @@ static int hostfs_readdir(struct file *f
 	int error, len;
 	unsigned int type;
 
-	name = dentry_name(file->f_path.dentry);
+	name = dentry_name(file_dentry(file));
 	if (name == NULL)
 		return -ENOMEM;
 	dir = open_dir(name, &error);
@@ -323,7 +323,7 @@ static int hostfs_open(struct inode *ino
 	if (mode & FMODE_WRITE)
 		r = w = 1;
 
-	name = dentry_name(file->f_path.dentry);
+	name = dentry_name(file_dentry(file));
 	if (name == NULL)
 		return -ENOMEM;
 
--- a/fs/hppfs/hppfs.c
+++ b/fs/hppfs/hppfs.c
@@ -430,7 +430,7 @@ static int hppfs_open(struct inode *inod
 	if (data == NULL)
 		goto out;
 
-	host_file = dentry_name(file->f_path.dentry, strlen("/rw"));
+	host_file = dentry_name(file_dentry(file), strlen("/rw"));
 	if (host_file == NULL)
 		goto out_free2;
 
@@ -568,7 +568,7 @@ static int hppfs_readdir(struct file *fi
 	struct hppfs_dirent d = {
 		.ctx.actor	= hppfs_filldir,
 		.caller		= ctx,
-		.dentry  	= file->f_path.dentry
+		.dentry  	= file_dentry(file)
 	};
 	int err;
 	proc_file->f_pos = ctx->pos;
--- a/fs/kernfs/dir.c
+++ b/fs/kernfs/dir.c
@@ -1365,7 +1365,7 @@ static struct kernfs_node *kernfs_dir_ne
 
 static int kernfs_fop_readdir(struct file *file, struct dir_context *ctx)
 {
-	struct dentry *dentry = file->f_path.dentry;
+	struct dentry *dentry = file_dentry(file);
 	struct kernfs_node *parent = dentry->d_fsdata;
 	struct kernfs_node *pos = file->private_data;
 	const void *ns = NULL;
--- a/fs/kernfs/file.c
+++ b/fs/kernfs/file.c
@@ -611,7 +611,7 @@ static void kernfs_put_open_node(struct
 
 static int kernfs_fop_open(struct inode *inode, struct file *file)
 {
-	struct kernfs_node *kn = file->f_path.dentry->d_fsdata;
+	struct kernfs_node *kn = file_dentry(file)->d_fsdata;
 	struct kernfs_root *root = kernfs_root(kn);
 	const struct kernfs_ops *ops;
 	struct kernfs_open_file *of;
@@ -728,7 +728,7 @@ static int kernfs_fop_open(struct inode
 
 static int kernfs_fop_release(struct inode *inode, struct file *filp)
 {
-	struct kernfs_node *kn = filp->f_path.dentry->d_fsdata;
+	struct kernfs_node *kn = file_dentry(filp)->d_fsdata;
 	struct kernfs_open_file *of = kernfs_of(filp);
 
 	kernfs_put_open_node(kn, of);
@@ -782,7 +782,7 @@ void kernfs_unmap_bin_file(struct kernfs
 static unsigned int kernfs_fop_poll(struct file *filp, poll_table *wait)
 {
 	struct kernfs_open_file *of = kernfs_of(filp);
-	struct kernfs_node *kn = filp->f_path.dentry->d_fsdata;
+	struct kernfs_node *kn = file_dentry(filp)->d_fsdata;
 	struct kernfs_open_node *on = kn->attr.open;
 
 	/* need parent for the kobj, grab both */
--- a/fs/ncpfs/dir.c
+++ b/fs/ncpfs/dir.c
@@ -417,7 +417,7 @@ ncp_invalidate_dircache_entries(struct d
 
 static int ncp_readdir(struct file *file, struct dir_context *ctx)
 {
-	struct dentry *dentry = file->f_path.dentry;
+	struct dentry *dentry = file_dentry(file);
 	struct inode *inode = d_inode(dentry);
 	struct page *page = NULL;
 	struct ncp_server *server = NCP_SERVER(inode);
@@ -579,7 +579,7 @@ ncp_fill_cache(struct file *file, struct
 		struct ncp_cache_control *ctrl, struct ncp_entry_info *entry,
 		int inval_childs)
 {
-	struct dentry *newdent, *dentry = file->f_path.dentry;
+	struct dentry *newdent, *dentry = file_dentry(file);
 	struct inode *dir = d_inode(dentry);
 	struct ncp_cache_control ctl = *ctrl;
 	struct qstr qname;
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -78,7 +78,7 @@ int dcache_dir_open(struct inode *inode,
 {
 	static struct qstr cursor_name = QSTR_INIT(".", 1);
 
-	file->private_data = d_alloc(file->f_path.dentry, &cursor_name);
+	file->private_data = d_alloc(file_dentry(file), &cursor_name);
 
 	return file->private_data ? 0 : -ENOMEM;
 }
@@ -93,7 +93,7 @@ EXPORT_SYMBOL(dcache_dir_close);
 
 loff_t dcache_dir_lseek(struct file *file, loff_t offset, int whence)
 {
-	struct dentry *dentry = file->f_path.dentry;
+	struct dentry *dentry = file_dentry(file);
 	mutex_lock(&d_inode(dentry)->i_mutex);
 	switch (whence) {
 		case 1:
@@ -148,7 +148,7 @@ static inline unsigned char dt_type(stru
 
 int dcache_readdir(struct file *file, struct dir_context *ctx)
 {
-	struct dentry *dentry = file->f_path.dentry;
+	struct dentry *dentry = file_dentry(file);
 	struct dentry *cursor = file->private_data;
 	struct list_head *p, *q = &cursor->d_child;
 
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -377,7 +377,7 @@ int nfs_readdir_xdr_filler(struct page *
  again:
 	timestamp = jiffies;
 	gencount = nfs_inc_attr_generation_counter();
-	error = NFS_PROTO(inode)->readdir(file->f_path.dentry, cred, entry->cookie, pages,
+	error = NFS_PROTO(inode)->readdir(file_dentry(file), cred, entry->cookie, pages,
 					  NFS_SERVER(inode)->dtsize, desc->plus);
 	if (error < 0) {
 		/* We requested READDIRPLUS, but the server doesn't grok it */
@@ -560,7 +560,7 @@ int nfs_readdir_page_filler(nfs_readdir_
 		count++;
 
 		if (desc->plus != 0)
-			nfs_prime_dcache(desc->file->f_path.dentry, entry);
+			nfs_prime_dcache(file_dentry(desc->file), entry);
 
 		status = nfs_readdir_add_to_array(entry, page);
 		if (status != 0)
@@ -872,7 +872,7 @@ static bool nfs_dir_mapping_need_revalid
  */
 static int nfs_readdir(struct file *file, struct dir_context *ctx)
 {
-	struct dentry	*dentry = file->f_path.dentry;
+	struct dentry	*dentry = file_dentry(file);
 	struct inode	*inode = d_inode(dentry);
 	nfs_readdir_descriptor_t my_desc,
 			*desc = &my_desc;
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -907,7 +907,7 @@ int nfs_open(struct inode *inode, struct
 {
 	struct nfs_open_context *ctx;
 
-	ctx = alloc_nfs_open_context(filp->f_path.dentry, filp->f_mode);
+	ctx = alloc_nfs_open_context(file_dentry(filp), filp->f_mode);
 	if (IS_ERR(ctx))
 		return PTR_ERR(ctx);
 	nfs_file_set_open_context(filp, ctx);
--- a/fs/nfs/nfs4file.c
+++ b/fs/nfs/nfs4file.c
@@ -22,7 +22,7 @@ static int
 nfs4_file_open(struct inode *inode, struct file *filp)
 {
 	struct nfs_open_context *ctx;
-	struct dentry *dentry = filp->f_path.dentry;
+	struct dentry *dentry = file_dentry(filp);
 	struct dentry *parent = NULL;
 	struct inode *dir;
 	unsigned openflags = filp->f_flags;
@@ -50,7 +50,7 @@ nfs4_file_open(struct inode *inode, stru
 	parent = dget_parent(dentry);
 	dir = d_inode(parent);
 
-	ctx = alloc_nfs_open_context(filp->f_path.dentry, filp->f_mode);
+	ctx = alloc_nfs_open_context(file_dentry(filp), filp->f_mode);
 	err = PTR_ERR(ctx);
 	if (IS_ERR(ctx))
 		goto out;
--- a/drivers/staging/lustre/lustre/llite/file.c
+++ b/drivers/staging/lustre/lustre/llite/file.c
@@ -640,7 +640,7 @@ int ll_file_open(struct inode *inode, st
 			   result in a deadlock */
 			mutex_unlock(&lli->lli_och_mutex);
 			it->it_create_mode |= M_CHECK_STALE;
-			rc = ll_intent_file_open(file->f_path.dentry, NULL, 0, it);
+			rc = ll_intent_file_open(file_dentry(file), NULL, 0, it);
 			it->it_create_mode &= ~M_CHECK_STALE;
 			if (rc)
 				goto out_openerr;
@@ -1486,7 +1486,7 @@ static int ll_lov_setea(struct inode *in
 		return -EFAULT;
 	}
 
-	rc = ll_lov_setstripe_ea_info(inode, file->f_path.dentry, flags, lump,
+	rc = ll_lov_setstripe_ea_info(inode, file_dentry(file), flags, lump,
 				     lum_size);
 	cl_lov_delay_create_clear(&file->f_flags);
 
@@ -1515,7 +1515,7 @@ static int ll_lov_setstripe(struct inode
 			return -EFAULT;
 	}
 
-	rc = ll_lov_setstripe_ea_info(inode, file->f_path.dentry, flags, lumv1,
+	rc = ll_lov_setstripe_ea_info(inode, file_dentry(file), flags, lumv1,
 				      lum_size);
 	cl_lov_delay_create_clear(&file->f_flags);
 	if (rc == 0) {
@@ -2094,7 +2094,7 @@ static int ll_swap_layouts(struct file *
 	rc = 0;
 	if (llss->ia2.ia_valid != 0) {
 		mutex_lock(&llss->inode1->i_mutex);
-		rc = ll_setattr(file1->f_path.dentry, &llss->ia2);
+		rc = ll_setattr(file_dentry(file1), &llss->ia2);
 		mutex_unlock(&llss->inode1->i_mutex);
 	}
 
@@ -2102,7 +2102,7 @@ static int ll_swap_layouts(struct file *
 		int rc1;
 
 		mutex_lock(&llss->inode2->i_mutex);
-		rc1 = ll_setattr(file2->f_path.dentry, &llss->ia1);
+		rc1 = ll_setattr(file_dentry(file2), &llss->ia1);
 		mutex_unlock(&llss->inode2->i_mutex);
 		if (rc == 0)
 			rc = rc1;
@@ -2187,7 +2187,7 @@ static int ll_hsm_import(struct inode *i
 
 	mutex_lock(&inode->i_mutex);
 
-	rc = ll_setattr_raw(file->f_path.dentry, attr, true);
+	rc = ll_setattr_raw(file_dentry(file), attr, true);
 	if (rc == -ENODATA)
 		rc = 0;
 
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -1615,7 +1615,7 @@ bool proc_fill_cache(struct file *file,
 	const char *name, int len,
 	instantiate_t instantiate, struct task_struct *task, const void *ptr)
 {
-	struct dentry *child, *dir = file->f_path.dentry;
+	struct dentry *child, *dir = file_dentry(file);
 	struct qstr qname = QSTR_INIT(name, len);
 	struct inode *inode;
 	unsigned type;
@@ -2155,7 +2155,7 @@ static ssize_t proc_pid_attr_read(struct
 		return -ESRCH;
 
 	length = security_getprocattr(task,
-				      (char*)file->f_path.dentry->d_name.name,
+				      (char*)file_dentry(file)->d_name.name,
 				      &p);
 	put_task_struct(task);
 	if (length > 0)
@@ -2198,7 +2198,7 @@ static ssize_t proc_pid_attr_write(struc
 		goto out_free;
 
 	length = security_setprocattr(task,
-				      (char*)file->f_path.dentry->d_name.name,
+				      (char*)file_dentry(file)->d_name.name,
 				      (void*)page, count);
 	mutex_unlock(&task->signal->cred_guard_mutex);
 out_free:
--- a/fs/proc/proc_sysctl.c
+++ b/fs/proc/proc_sysctl.c
@@ -578,7 +578,7 @@ static bool proc_sys_fill_cache(struct f
 				struct ctl_table_header *head,
 				struct ctl_table *table)
 {
-	struct dentry *child, *dir = file->f_path.dentry;
+	struct dentry *child, *dir = file_dentry(file);
 	struct inode *inode;
 	struct qstr qname;
 	ino_t ino = 0;
--- a/fs/overlayfs/readdir.c
+++ b/fs/overlayfs/readdir.c
@@ -247,7 +247,7 @@ static void ovl_dir_reset(struct file *f
 {
 	struct ovl_dir_file *od = file->private_data;
 	struct ovl_dir_cache *cache = od->cache;
-	struct dentry *dentry = file->f_path.dentry;
+	struct dentry *dentry = file_dentry(file);
 	enum ovl_path_type type = ovl_path_type(dentry);
 
 	if (cache && ovl_dentry_version_get(dentry) != cache->version) {
@@ -342,7 +342,7 @@ static struct ovl_dir_cache *ovl_cache_g
 static int ovl_iterate(struct file *file, struct dir_context *ctx)
 {
 	struct ovl_dir_file *od = file->private_data;
-	struct dentry *dentry = file->f_path.dentry;
+	struct dentry *dentry = file_dentry(file);
 	struct ovl_cache_entry *p;
 
 	if (!ctx->pos)
@@ -417,7 +417,7 @@ static int ovl_dir_fsync(struct file *fi
 			 int datasync)
 {
 	struct ovl_dir_file *od = file->private_data;
-	struct dentry *dentry = file->f_path.dentry;
+	struct dentry *dentry = file_dentry(file);
 	struct file *realfile = od->realfile;
 
 	/*
@@ -459,7 +459,7 @@ static int ovl_dir_release(struct inode
 
 	if (od->cache) {
 		mutex_lock(&inode->i_mutex);
-		ovl_cache_put(od, file->f_path.dentry);
+		ovl_cache_put(od, file_dentry(file));
 		mutex_unlock(&inode->i_mutex);
 	}
 	fput(od->realfile);
@@ -481,7 +481,7 @@ static int ovl_dir_open(struct inode *in
 	if (!od)
 		return -ENOMEM;
 
-	type = ovl_path_real(file->f_path.dentry, &realpath);
+	type = ovl_path_real(file_dentry(file), &realpath);
 	realfile = ovl_path_open(&realpath, file->f_flags);
 	if (IS_ERR(realfile)) {
 		kfree(od);

Re: [PATCH 0/8] Security: Provide unioned file support

[Al Viro]

On Fri, Jun 19, 2015 at 10:11:28AM +0200, Miklos Szeredi wrote:
>  drivers/staging/lustre/lustre/llite/file.c |   12 ++++++------
>  fs/9p/vfs_file.c                           |    6 +++---
>  fs/btrfs/file.c                            |    2 +-
>  fs/btrfs/ioctl.c                           |   13 +++++++------
>  fs/ceph/dir.c                              |    6 +++---
>  fs/ceph/file.c                             |    2 +-
>  fs/cifs/file.c                             |    4 ++--
>  fs/cifs/readdir.c                          |    4 ++--
>  fs/configfs/dir.c                          |    8 ++++----
>  fs/configfs/file.c                         |   15 +++++++++------
>  fs/fat/file.c                              |    7 ++++---
>  fs/fuse/dir.c                              |    2 +-
>  fs/hfsplus/ioctl.c                         |    2 +-
>  fs/hostfs/hostfs_kern.c                    |    4 ++--
>  fs/hppfs/hppfs.c                           |    4 ++--
>  fs/kernfs/dir.c                            |    2 +-
>  fs/kernfs/file.c                           |    6 +++---
>  fs/libfs.c                                 |    6 +++---
>  fs/ncpfs/dir.c                             |    4 ++--
>  fs/nfs/dir.c                               |    6 +++---
>  fs/nfs/inode.c                             |    2 +-
>  fs/nfs/nfs4file.c                          |    4 ++--
>  fs/overlayfs/readdir.c                     |   10 +++++-----
>  fs/proc/base.c                             |    6 +++---
>  fs/proc/proc_sysctl.c                      |    2 +-
>  include/linux/fs.h                         |    5 +++++
>  26 files changed, 77 insertions(+), 67 deletions(-)
> 
> --- a/fs/9p/vfs_file.c
> +++ b/fs/9p/vfs_file.c
> @@ -74,7 +74,7 @@ int v9fs_file_open(struct inode *inode,
>  					v9fs_proto_dotu(v9ses));
>  	fid = file->private_data;
>  	if (!fid) {
> -		fid = v9fs_fid_clone(file->f_path.dentry);
> +		fid = v9fs_fid_clone(file_dentry(file));
>  		if (IS_ERR(fid))
>  			return PTR_ERR(fid);
>  
> @@ -100,7 +100,7 @@ int v9fs_file_open(struct inode *inode,
>  		 * because we want write after unlink usecase
>  		 * to work.
>  		 */
> -		fid = v9fs_writeback_fid(file->f_path.dentry);
> +		fid = v9fs_writeback_fid(file_dentry(file));
>  		if (IS_ERR(fid)) {
>  			err = PTR_ERR(fid);
>  			mutex_unlock(&v9inode->v_mutex);
> @@ -515,7 +515,7 @@ v9fs_mmap_file_mmap(struct file *filp, s
>  		 * because we want write after unlink usecase
>  		 * to work.
>  		 */
> -		fid = v9fs_writeback_fid(filp->f_path.dentry);
> +		fid = v9fs_writeback_fid(file_dentry(filp));
>  		if (IS_ERR(fid)) {
>  			retval = PTR_ERR(fid);
>  			mutex_unlock(&v9inode->v_mutex);
> --- a/fs/btrfs/file.c
> +++ b/fs/btrfs/file.c
> @@ -1861,7 +1861,7 @@ static int start_ordered_ops(struct inod
>   */
>  int btrfs_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
>  {
> -	struct dentry *dentry = file->f_path.dentry;
> +	struct dentry *dentry = file_dentry(file);
>  	struct inode *inode = d_inode(dentry);

	file_inode(file), please.  And looking at the only other use
of dentry in there...  I don't see anything that would guarantee that
dentry will remain the child of its parent, which btrfs_log_dentry_safe()
seems to assume.

> +static noinline int btrfs_mksubvol(struct file *file,
>  				   char *name, int namelen,
>  				   struct btrfs_root *snap_src,
>  				   u64 *async_transid, bool readonly,
>  				   struct btrfs_qgroup_inherit *inherit)
>  {
> -	struct inode *dir  = d_inode(parent->dentry);
> +	struct dentry *parent = file_dentry(file);
> +	struct inode *dir  = d_inode(parent);

Directory, probably?

> -	dentry = lookup_one_len(name, parent->dentry, namelen);
> +	dentry = lookup_one_len(name, parent, namelen);

... it would better be one.

> @@ -121,7 +121,7 @@ static int __dcache_readdir(struct file
>  			    u32 shared_gen)
>  {
>  	struct ceph_file_info *fi = file->private_data;
> -	struct dentry *parent = file->f_path.dentry;
> +	struct dentry *parent = file_dentry(file);

Directory.  And I would be very surprised if ceph + overlayfs wouldn't
step into some rather nasty things...

>  static int configfs_dir_open(struct inode *inode, struct file *file)
>  {
> -	struct dentry * dentry = file->f_path.dentry;
> +	struct dentry *dentry = file_dentry(file);

Directory, and combination of configfs with overlayfs is *definitely*
a bad idea.

> --- a/fs/fat/file.c
> +++ b/fs/fat/file.c

Unusable with overlayfs due to ->d_hash() issues.

> --- a/fs/fuse/dir.c
> +++ b/fs/fuse/dir.c
> @@ -1165,7 +1165,7 @@ static int fuse_direntplus_link(struct f
>  	int err;
>  	struct fuse_entry_out *o = &direntplus->entry_out;
>  	struct fuse_dirent *dirent = &direntplus->dirent;
> -	struct dentry *parent = file->f_path.dentry;
> +	struct dentry *parent = file_dentry(file);

Directory.

> --- a/fs/hfsplus/ioctl.c
> +++ b/fs/hfsplus/ioctl.c

->d_hash()

> --- a/fs/hostfs/hostfs_kern.c
> +++ b/fs/hostfs/hostfs_kern.c
> @@ -284,7 +284,7 @@ static int hostfs_readdir(struct file *f
>  	int error, len;
>  	unsigned int type;
>  
> -	name = dentry_name(file->f_path.dentry);
> +	name = dentry_name(file_dentry(file));

Directory

> --- a/fs/hppfs/hppfs.c
> +++ b/fs/hppfs/hppfs.c

git rm fodder.

> --- a/fs/kernfs/dir.c
> +++ b/fs/kernfs/dir.c

Are you seriously going to allow that as overlayfs layer?

> --- a/fs/ncpfs/dir.c
> +++ b/fs/ncpfs/dir.c
> @@ -417,7 +417,7 @@ ncp_invalidate_dircache_entries(struct d
>  
>  static int ncp_readdir(struct file *file, struct dir_context *ctx)
>  {

Directory (and case sensitivity issues on top of that).

> --- a/fs/libfs.c
> +++ b/fs/libfs.c
> @@ -78,7 +78,7 @@ int dcache_dir_open(struct inode *inode,
>  {
>  	static struct qstr cursor_name = QSTR_INIT(".", 1);
>  
> -	file->private_data = d_alloc(file->f_path.dentry, &cursor_name);
> +	file->private_data = d_alloc(file_dentry(file), &cursor_name);

Directory.

... and so on

Re: [PATCH 0/8] Security: Provide unioned file support

[Miklos Szeredi]

On Fri, Jun 19, 2015 at 10:29 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> Directory, probably?
>
>> -     dentry = lookup_one_len(name, parent->dentry, namelen);
>> +     dentry = lookup_one_len(name, parent, namelen);
>
> ... it would better be one.

My point is: without a bloody accessor, there's no way to warn if the
use is illegal (i.e. nondirectory).

Maybe we won't need file->f_dentry, but we do need a way to WARN about
abuse (which there IS probably a significant number already and
nothing preventing more).

Thanks,
Miklos

Re: [PATCH 0/8] Security: Provide unioned file support

[David Howells]

Miklos Szeredi <miklos@szeredi.hu> wrote:

> What's going to happen to all those f_path.dentry uses where the
> filesystem thinks it's getting its own dentry?

Hmmm...  I guess that breaks CIFS.

David

Re: [PATCH 0/8] Security: Provide unioned file support

[David Howells]

Miklos Szeredi <miklos@szeredi.hu> wrote:

> What's going to happen to all those f_path.dentry uses where the
> filesystem thinks it's getting its own dentry?

Hmmm...  I guess that breaks CIFS.

David
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/