[PATCH] ovl: fix sgid inhertance over whiteout

[PATCH] ovl: fix sgid inhertance over whiteout

[Eryu Guan]

In commit bb0d2b8ad296 ("ovl: fix sgid on directory"), mode of newly
created files & dirs over whiteout was updated to pickup sgid bit. But
it used stat->mode directly, which could not be the correct mode,
because stat->mode could be tailored against umask.

This can be demonstrated by the following script:

umask 022
mkdir -p lower/dir upper work mnt
chown test:test lower/dir
chmod 2775 lower/dir
touch lower/dir/testdir
touch lower/dir/testfile

mount -t overlay -olowerdir=lower,upperdir=upper,workdir=work none mnt
rm -f mnt/dir/testdir
mkdir mnt/dir/testdir
rm -f mnt/dir/testfile
touch mnt/dir/testfile
touch mnt/dir/newfile
ls -l mnt/dir

-rw-r--r--. 1 root test 0 Aug 25 15:45 newfile
drwxrwsrwx. 2 root test 6 Aug 25 15:45 testdir
-rw-rw-rw-. 1 root test 0 Aug 25 15:45 testfile

testdir and testfile are created over whiteout, the modes contain write
permission for group and other, but they shouldn't, like 'newfile'.

Fix it by resetting mode against upperdir inode using
inode_init_owner().

Fixes: bb0d2b8ad296 ("ovl: fix sgid on directory")
Signed-off-by: Eryu Guan <guaneryu@gmail.com>
---

A new fstests test case followed to fstests list.

 fs/overlayfs/dir.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index 12bcd07..9a5e716 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -370,17 +370,19 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
 		goto out_dput2;
 
 	/*
-	 * mode could have been mutilated due to umask (e.g. sgid directory)
+	 * mode could lose sgid bit if upperdir has it set, because workdir has
+	 * no sgid. Reset mode against upperdir.
 	 */
-	if (!hardlink &&
-	    !S_ISLNK(stat->mode) && newdentry->d_inode->i_mode != stat->mode) {
+	if (!hardlink && !S_ISLNK(stat->mode)) {
+		struct inode *newinode = newdentry->d_inode;
 		struct iattr attr = {
 			.ia_valid = ATTR_MODE,
-			.ia_mode = stat->mode,
 		};
-		inode_lock(newdentry->d_inode);
+		inode_init_owner(newinode, udir, newinode->i_mode);
+		attr.ia_mode = newinode->i_mode;
+		inode_lock(newinode);
 		err = notify_change(newdentry, &attr, NULL);
-		inode_unlock(newdentry->d_inode);
+		inode_unlock(newinode);
 		if (err)
 			goto out_cleanup;
 	}
-- 
2.7.4

Re: [PATCH] ovl: fix sgid inhertance over whiteout

[Miklos Szeredi]

On Thu, Aug 25, 2016 at 04:15:02PM +0800, Eryu Guan wrote:
> In commit bb0d2b8ad296 ("ovl: fix sgid on directory"), mode of newly
> created files & dirs over whiteout was updated to pickup sgid bit. But
> it used stat->mode directly, which could not be the correct mode,
> because stat->mode could be tailored against umask.
> 
> This can be demonstrated by the following script:
> 
> umask 022
> mkdir -p lower/dir upper work mnt
> chown test:test lower/dir
> chmod 2775 lower/dir
> touch lower/dir/testdir
> touch lower/dir/testfile
> 
> mount -t overlay -olowerdir=lower,upperdir=upper,workdir=work none mnt
> rm -f mnt/dir/testdir
> mkdir mnt/dir/testdir
> rm -f mnt/dir/testfile
> touch mnt/dir/testfile
> touch mnt/dir/newfile
> ls -l mnt/dir
> 
> -rw-r--r--. 1 root test 0 Aug 25 15:45 newfile
> drwxrwsrwx. 2 root test 6 Aug 25 15:45 testdir
> -rw-rw-rw-. 1 root test 0 Aug 25 15:45 testfile
> 
> testdir and testfile are created over whiteout, the modes contain write
> permission for group and other, but they shouldn't, like 'newfile'.

Hi Eryu,

Yeah, the reson is that we fail to take umask into account.  And that's due to

+       sb->s_flags |= MS_POSIXACL;

in 39a25b2b3762 ("ovl: define ->get_acl() for overlay inodes").

Your patch works around this by relying on the mode of the file created in
workdir, but this only solves part of the problem, and is also not very clean,
since inode_init_owner() should only be called inode of the filesystem itself,
not for foreign inodes.

The below patch should fix this together with the default posix acl inheritance,
which also has an effect on mode of created inode.

It would be nice to get complete test coverage for file creation which is quite
complicated and apparently bug prone.  Variables to test for:

1) create over whiteout or not

   - behavior shouldn't change but different codepaths are taken

2) creating directory, nodirectory, symlink or hard link

   - hard link: no new inode is created, attributes, ACL, etc. of old inode are
     not changed

   - symlink has fix mode

   - if neither sgid or default ACL on parent then create mode calculated based
     on supplied mode and umask

   - otherwise see below

3) parent has sgid or not

   - if sgid is set AND creating directory then inherit sgid

   - if sgid is set then inherit group

4) parent has default ACL or not

   - if parent has default ACL and creating a directory then inherit default ACL

   - if parent has default ACL then set mode and access ACL based on default ACL
     of parent

5) underlying filsystem sets MS_POSIXACL or not

   - codepaths might differ; shouldn't change behavior

   - unfrortunately I don't think there's a filesystem that supports overlayfs
     upperdir but not POSIX ACL, so this needs separate kernel configs to test.

So that's quite a few independent parameters, resulting in lots of cases.  I
guess it's easier to write a generic test case which can test all combinations
above, than to write individual test cases by hand.

Thanks,
Miklos
----

From: Miklos Szeredi <mszeredi@redhat.com>
Subject: ovl: handle umask and posix_acl_default correctly on creation

Setting MS_POSIXACL in sb->s_flags has the side effect of passing mode to
create functions without masking against umask.

Another problem when creating over a whiteout is that the default posix acl
is not inherited from the parent dir (because the real parent dir at the
time of creation is the work directory).

Fix these problems by:

 a) If upper fs does not have MS_POSIXACL, then mask mode with umask.

 b) If creating over a whiteout, call posix_acl_create() to get the
 inherited acls.  After creation (but before moving to the final
 destination) set these acls on the created file.  posix_acl_create() also
 updates the file creation mode as appropriate.

Fixes: 39a25b2b3762 ("ovl: define ->get_acl() for overlay inodes")
Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/overlayfs/dir.c |   56 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 56 insertions(+)

--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -12,6 +12,8 @@
 #include <linux/xattr.h>
 #include <linux/security.h>
 #include <linux/cred.h>
+#include <linux/posix_acl.h>
+#include <linux/posix_acl_xattr.h>
 #include "overlayfs.h"
 
 void ovl_cleanup(struct inode *wdir, struct dentry *wdentry)
@@ -186,6 +188,9 @@ static int ovl_create_upper(struct dentr
 	struct dentry *newdentry;
 	int err;
 
+	if (!hardlink && !IS_POSIXACL(udir))
+		stat->mode &= ~current_umask();
+
 	inode_lock_nested(udir, I_MUTEX_PARENT);
 	newdentry = lookup_one_len(dentry->d_name.name, upperdir,
 				   dentry->d_name.len);
@@ -335,6 +340,32 @@ static struct dentry *ovl_check_empty_an
 	return ret;
 }
 
+static int ovl_set_upper_acl(struct dentry *upperdentry, const char *name,
+			     const struct posix_acl *acl)
+{
+	void *buffer;
+	size_t size;
+	int err;
+
+	if (!IS_ENABLED(CONFIG_FS_POSIX_ACL) || !acl)
+		return 0;
+
+	size = posix_acl_to_xattr(NULL, acl, NULL, 0);
+	buffer = kmalloc(size, GFP_KERNEL);
+	if (!buffer)
+		return -ENOMEM;
+
+	size = posix_acl_to_xattr(&init_user_ns, acl, buffer, size);
+	err = size;
+	if (err < 0)
+		goto out_free;
+
+	err = vfs_setxattr(upperdentry, name, buffer, size, XATTR_CREATE);
+out_free:
+	kfree(buffer);
+	return err;
+}
+
 static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
 				    struct kstat *stat, const char *link,
 				    struct dentry *hardlink)
@@ -346,10 +377,20 @@ static int ovl_create_over_whiteout(stru
 	struct dentry *upper;
 	struct dentry *newdentry;
 	int err;
+	struct posix_acl *acl, *default_acl;
 
 	if (WARN_ON(!workdir))
 		return -EROFS;
 
+	if (!hardlink) {
+		if (!IS_POSIXACL(udir))
+			stat->mode &= ~current_umask();
+
+		err = posix_acl_create(udir, &stat->mode, &default_acl, &acl);
+		if (err)
+			return err;
+	}
+
 	err = ovl_lock_rename_workdir(workdir, upperdir);
 	if (err)
 		goto out;
@@ -384,6 +425,17 @@ static int ovl_create_over_whiteout(stru
 		if (err)
 			goto out_cleanup;
 	}
+	if (!hardlink) {
+		err = ovl_set_upper_acl(newdentry, XATTR_NAME_POSIX_ACL_ACCESS,
+					acl);
+		if (err)
+			goto out_cleanup;
+
+		err = ovl_set_upper_acl(newdentry, XATTR_NAME_POSIX_ACL_DEFAULT,
+					default_acl);
+		if (err)
+			goto out_cleanup;
+	}
 
 	if (!hardlink && S_ISDIR(stat->mode)) {
 		err = ovl_set_opaque(newdentry);
@@ -410,6 +462,10 @@ static int ovl_create_over_whiteout(stru
 out_unlock:
 	unlock_rename(workdir, upperdir);
 out:
+	if (!hardlink) {
+		posix_acl_release(acl);
+		posix_acl_release(default_acl);
+	}
 	return err;
 
 out_cleanup:

Re: [PATCH] ovl: fix sgid inhertance over whiteout

[Eryu Guan]

On Fri, Aug 26, 2016 at 11:50:49AM +0200, Miklos Szeredi wrote:
> On Thu, Aug 25, 2016 at 04:15:02PM +0800, Eryu Guan wrote:
> > In commit bb0d2b8ad296 ("ovl: fix sgid on directory"), mode of newly
> > created files & dirs over whiteout was updated to pickup sgid bit. But
> > it used stat->mode directly, which could not be the correct mode,
> > because stat->mode could be tailored against umask.
> > 
> > This can be demonstrated by the following script:
> > 
> > umask 022
> > mkdir -p lower/dir upper work mnt
> > chown test:test lower/dir
> > chmod 2775 lower/dir
> > touch lower/dir/testdir
> > touch lower/dir/testfile
> > 
> > mount -t overlay -olowerdir=lower,upperdir=upper,workdir=work none mnt
> > rm -f mnt/dir/testdir
> > mkdir mnt/dir/testdir
> > rm -f mnt/dir/testfile
> > touch mnt/dir/testfile
> > touch mnt/dir/newfile
> > ls -l mnt/dir
> > 
> > -rw-r--r--. 1 root test 0 Aug 25 15:45 newfile
> > drwxrwsrwx. 2 root test 6 Aug 25 15:45 testdir
> > -rw-rw-rw-. 1 root test 0 Aug 25 15:45 testfile
> > 
> > testdir and testfile are created over whiteout, the modes contain write
> > permission for group and other, but they shouldn't, like 'newfile'.
> 
> Hi Eryu,
> 
> Yeah, the reson is that we fail to take umask into account.  And that's due to
> 
> +       sb->s_flags |= MS_POSIXACL;
> 
> in 39a25b2b3762 ("ovl: define ->get_acl() for overlay inodes").
> 
> Your patch works around this by relying on the mode of the file created in
> workdir, but this only solves part of the problem, and is also not very clean,
> since inode_init_owner() should only be called inode of the filesystem itself,
> not for foreign inodes.
> 
> The below patch should fix this together with the default posix acl inheritance,
> which also has an effect on mode of created inode.

I tested this patch and it passed my test case.

> 
> It would be nice to get complete test coverage for file creation which is quite
> complicated and apparently bug prone.  Variables to test for:

Thanks, I'll look into them and try to generate test cases for fstests.
Some cases have been covered by existing cases, I'll focus on create
over whiteout.

Thanks,
Eryu

> 
> 1) create over whiteout or not
> 
>    - behavior shouldn't change but different codepaths are taken
> 
> 2) creating directory, nodirectory, symlink or hard link
> 
>    - hard link: no new inode is created, attributes, ACL, etc. of old inode are
>      not changed
> 
>    - symlink has fix mode
> 
>    - if neither sgid or default ACL on parent then create mode calculated based
>      on supplied mode and umask
> 
>    - otherwise see below
> 
> 3) parent has sgid or not
> 
>    - if sgid is set AND creating directory then inherit sgid
> 
>    - if sgid is set then inherit group
> 
> 4) parent has default ACL or not
> 
>    - if parent has default ACL and creating a directory then inherit default ACL
> 
>    - if parent has default ACL then set mode and access ACL based on default ACL
>      of parent
> 
> 5) underlying filsystem sets MS_POSIXACL or not
> 
>    - codepaths might differ; shouldn't change behavior
> 
>    - unfrortunately I don't think there's a filesystem that supports overlayfs
>      upperdir but not POSIX ACL, so this needs separate kernel configs to test.
> 
> So that's quite a few independent parameters, resulting in lots of cases.  I
> guess it's easier to write a generic test case which can test all combinations
> above, than to write individual test cases by hand.
> 
> Thanks,
> Miklos
> ----
> 
> From: Miklos Szeredi <mszeredi@redhat.com>
> Subject: ovl: handle umask and posix_acl_default correctly on creation
> 
> Setting MS_POSIXACL in sb->s_flags has the side effect of passing mode to
> create functions without masking against umask.
> 
> Another problem when creating over a whiteout is that the default posix acl
> is not inherited from the parent dir (because the real parent dir at the
> time of creation is the work directory).
> 
> Fix these problems by:
> 
>  a) If upper fs does not have MS_POSIXACL, then mask mode with umask.
> 
>  b) If creating over a whiteout, call posix_acl_create() to get the
>  inherited acls.  After creation (but before moving to the final
>  destination) set these acls on the created file.  posix_acl_create() also
>  updates the file creation mode as appropriate.
> 
> Fixes: 39a25b2b3762 ("ovl: define ->get_acl() for overlay inodes")
> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> ---
>  fs/overlayfs/dir.c |   56 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 56 insertions(+)
> 
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -12,6 +12,8 @@
>  #include <linux/xattr.h>
>  #include <linux/security.h>
>  #include <linux/cred.h>
> +#include <linux/posix_acl.h>
> +#include <linux/posix_acl_xattr.h>
>  #include "overlayfs.h"
>  
>  void ovl_cleanup(struct inode *wdir, struct dentry *wdentry)
> @@ -186,6 +188,9 @@ static int ovl_create_upper(struct dentr
>  	struct dentry *newdentry;
>  	int err;
>  
> +	if (!hardlink && !IS_POSIXACL(udir))
> +		stat->mode &= ~current_umask();
> +
>  	inode_lock_nested(udir, I_MUTEX_PARENT);
>  	newdentry = lookup_one_len(dentry->d_name.name, upperdir,
>  				   dentry->d_name.len);
> @@ -335,6 +340,32 @@ static struct dentry *ovl_check_empty_an
>  	return ret;
>  }
>  
> +static int ovl_set_upper_acl(struct dentry *upperdentry, const char *name,
> +			     const struct posix_acl *acl)
> +{
> +	void *buffer;
> +	size_t size;
> +	int err;
> +
> +	if (!IS_ENABLED(CONFIG_FS_POSIX_ACL) || !acl)
> +		return 0;
> +
> +	size = posix_acl_to_xattr(NULL, acl, NULL, 0);
> +	buffer = kmalloc(size, GFP_KERNEL);
> +	if (!buffer)
> +		return -ENOMEM;
> +
> +	size = posix_acl_to_xattr(&init_user_ns, acl, buffer, size);
> +	err = size;
> +	if (err < 0)
> +		goto out_free;
> +
> +	err = vfs_setxattr(upperdentry, name, buffer, size, XATTR_CREATE);
> +out_free:
> +	kfree(buffer);
> +	return err;
> +}
> +
>  static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
>  				    struct kstat *stat, const char *link,
>  				    struct dentry *hardlink)
> @@ -346,10 +377,20 @@ static int ovl_create_over_whiteout(stru
>  	struct dentry *upper;
>  	struct dentry *newdentry;
>  	int err;
> +	struct posix_acl *acl, *default_acl;
>  
>  	if (WARN_ON(!workdir))
>  		return -EROFS;
>  
> +	if (!hardlink) {
> +		if (!IS_POSIXACL(udir))
> +			stat->mode &= ~current_umask();
> +
> +		err = posix_acl_create(udir, &stat->mode, &default_acl, &acl);
> +		if (err)
> +			return err;
> +	}
> +
>  	err = ovl_lock_rename_workdir(workdir, upperdir);
>  	if (err)
>  		goto out;
> @@ -384,6 +425,17 @@ static int ovl_create_over_whiteout(stru
>  		if (err)
>  			goto out_cleanup;
>  	}
> +	if (!hardlink) {
> +		err = ovl_set_upper_acl(newdentry, XATTR_NAME_POSIX_ACL_ACCESS,
> +					acl);
> +		if (err)
> +			goto out_cleanup;
> +
> +		err = ovl_set_upper_acl(newdentry, XATTR_NAME_POSIX_ACL_DEFAULT,
> +					default_acl);
> +		if (err)
> +			goto out_cleanup;
> +	}
>  
>  	if (!hardlink && S_ISDIR(stat->mode)) {
>  		err = ovl_set_opaque(newdentry);
> @@ -410,6 +462,10 @@ static int ovl_create_over_whiteout(stru
>  out_unlock:
>  	unlock_rename(workdir, upperdir);
>  out:
> +	if (!hardlink) {
> +		posix_acl_release(acl);
> +		posix_acl_release(default_acl);
> +	}
>  	return err;
>  
>  out_cleanup:
> 
>