[PATCH] xfs: fix sgid inheritance for subdirectories inheriting default acls

[PATCH] xfs: fix sgid inheritance for subdirectories inheriting default acls

[Carlos Maiolino]

XFS removes sgid bits of subdirectories created under a directory containing a
default acl.

When a default acl is set, it implies xfs to call xfs_setattr_nonsize() in its
code path. Such function is shared among mkdir and chmod system calls, and
does some checks unneeded by mkdir (calling inode_change_ok()). Such checks
remove sgid bit from the inode after it has been granted. So, this patch wraps
up these checks to be used only in chmod path.

Also, chmod should not remove sgid bit from an inode if this is a directory, so,
it adds a check if S_ISDIR is set in the inode mode, into xfs_setattr_nonsize()

Done that, xfs_setattr_mode() doesn't need to re-check for group id and
capabilities permissions, this only implies in another try to remove sgid bit
from the directories. Such check is already done either on inode_change_ok() or
xfs_setattr_nonsize().

This addresses SGI bug 280:
http://oss.sgi.com/bugzilla/show_bug.cgi?id=280

Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
 fs/xfs/xfs_iops.c |   51 ++++++++++++++++++++++++++++++++++-----------------
 1 files changed, 34 insertions(+), 17 deletions(-)

diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index ca9ecaa..5c9c505 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -467,9 +467,6 @@ xfs_setattr_mode(
 	ASSERT(tp);
 	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
 
-	if (!in_group_p(inode->i_gid) && !capable(CAP_FSETID))
-		mode &= ~S_ISGID;
-
 	ip->i_d.di_mode &= S_IFMT;
 	ip->i_d.di_mode |= mode & ~S_IFMT;
 
@@ -477,23 +474,20 @@ xfs_setattr_mode(
 	inode->i_mode |= mode & ~S_IFMT;
 }
 
+/* Check inode permissions and filesystem errors
+ *
+ * Wrapper to some pre-checks needed while changing
+ * inode attributes
+ */
 int
-xfs_setattr_nonsize(
+xfs_setattr_setup(
 	struct xfs_inode	*ip,
 	struct iattr		*iattr,
 	int			flags)
 {
 	xfs_mount_t		*mp = ip->i_mount;
 	struct inode		*inode = VFS_I(ip);
-	int			mask = iattr->ia_valid;
-	xfs_trans_t		*tp;
 	int			error;
-	uid_t			uid = 0, iuid = 0;
-	gid_t			gid = 0, igid = 0;
-	struct xfs_dquot	*udqp = NULL, *gdqp = NULL;
-	struct xfs_dquot	*olddquot1 = NULL, *olddquot2 = NULL;
-
-	trace_xfs_setattr(ip);
 
 	if (mp->m_flags & XFS_MOUNT_RDONLY)
 		return XFS_ERROR(EROFS);
@@ -505,6 +499,27 @@ xfs_setattr_nonsize(
 	if (error)
 		return XFS_ERROR(error);
 
+	return xfs_setattr_nonsize(ip, iattr, flags);
+}
+
+int
+xfs_setattr_nonsize(
+	struct xfs_inode	*ip,
+	struct iattr		*iattr,
+	int			flags)
+{
+	xfs_mount_t		*mp = ip->i_mount;
+	struct inode		*inode = VFS_I(ip);
+	int			mask = iattr->ia_valid;
+	xfs_trans_t		*tp;
+	int			error;
+	uid_t			uid = 0, iuid = 0;
+	gid_t			gid = 0, igid = 0;
+	struct xfs_dquot	*udqp = NULL, *gdqp = NULL;
+	struct xfs_dquot	*olddquot1 = NULL, *olddquot2 = NULL;
+
+	trace_xfs_setattr(ip);
+
 	ASSERT((mask & ATTR_SIZE) == 0);
 
 	/*
@@ -592,11 +607,13 @@ xfs_setattr_nonsize(
 		 * CAP_FSETID overrides the following restrictions:
 		 *
 		 * The set-user-ID and set-group-ID bits of a file will be
-		 * cleared upon successful return from chown()
+		 * cleared upon successful return from chown().
+		 * Of a directory, these bits should be kept.
 		 */
-		if ((ip->i_d.di_mode & (S_ISUID|S_ISGID)) &&
-		    !capable(CAP_FSETID))
-			ip->i_d.di_mode &= ~(S_ISUID|S_ISGID);
+		if (!S_ISDIR(inode->i_mode))
+			if ((ip->i_d.di_mode & (S_ISUID|S_ISGID)) &&
+			    !capable(CAP_FSETID))
+				ip->i_d.di_mode &= ~(S_ISUID|S_ISGID);
 
 		/*
 		 * Change the ownerships and register quota modifications
@@ -915,7 +932,7 @@ xfs_vn_setattr(
 {
 	if (iattr->ia_valid & ATTR_SIZE)
 		return -xfs_setattr_size(XFS_I(dentry->d_inode), iattr, 0);
-	return -xfs_setattr_nonsize(XFS_I(dentry->d_inode), iattr, 0);
+	return -xfs_setattr_setup(XFS_I(dentry->d_inode), iattr, 0);
 }
 
 STATIC int
-- 
1.7.1

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: fix sgid inheritance for subdirectories inheriting default acls

[Brian Foster]

On 06/14/2013 03:41 PM, Carlos Maiolino wrote:
> XFS removes sgid bits of subdirectories created under a directory containing a
> default acl.
> 
> When a default acl is set, it implies xfs to call xfs_setattr_nonsize() in its
> code path. Such function is shared among mkdir and chmod system calls, and
> does some checks unneeded by mkdir (calling inode_change_ok()). Such checks
> remove sgid bit from the inode after it has been granted. So, this patch wraps
> up these checks to be used only in chmod path.
> 
> Also, chmod should not remove sgid bit from an inode if this is a directory, so,
> it adds a check if S_ISDIR is set in the inode mode, into xfs_setattr_nonsize()
> 
> Done that, xfs_setattr_mode() doesn't need to re-check for group id and
> capabilities permissions, this only implies in another try to remove sgid bit
> from the directories. Such check is already done either on inode_change_ok() or
> xfs_setattr_nonsize().
> 
> This addresses SGI bug 280:
> http://oss.sgi.com/bugzilla/show_bug.cgi?id=280
> 
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---

Hi Carlos,

A couple comments, mostly aesthetic...

>  fs/xfs/xfs_iops.c |   51 ++++++++++++++++++++++++++++++++++-----------------
>  1 files changed, 34 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index ca9ecaa..5c9c505 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -467,9 +467,6 @@ xfs_setattr_mode(
>  	ASSERT(tp);
>  	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
>  
> -	if (!in_group_p(inode->i_gid) && !capable(CAP_FSETID))
> -		mode &= ~S_ISGID;
> -
>  	ip->i_d.di_mode &= S_IFMT;
>  	ip->i_d.di_mode |= mode & ~S_IFMT;
>  
> @@ -477,23 +474,20 @@ xfs_setattr_mode(
>  	inode->i_mode |= mode & ~S_IFMT;
>  }
>  
> +/* Check inode permissions and filesystem errors
> + *
> + * Wrapper to some pre-checks needed while changing
> + * inode attributes
> + */
>  int
> -xfs_setattr_nonsize(
> +xfs_setattr_setup(
>  	struct xfs_inode	*ip,
>  	struct iattr		*iattr,
>  	int			flags)
>  {
>  	xfs_mount_t		*mp = ip->i_mount;
>  	struct inode		*inode = VFS_I(ip);
> -	int			mask = iattr->ia_valid;
> -	xfs_trans_t		*tp;
>  	int			error;
> -	uid_t			uid = 0, iuid = 0;
> -	gid_t			gid = 0, igid = 0;
> -	struct xfs_dquot	*udqp = NULL, *gdqp = NULL;
> -	struct xfs_dquot	*olddquot1 = NULL, *olddquot2 = NULL;
> -
> -	trace_xfs_setattr(ip);
>  
>  	if (mp->m_flags & XFS_MOUNT_RDONLY)
>  		return XFS_ERROR(EROFS);
> @@ -505,6 +499,27 @@ xfs_setattr_nonsize(
>  	if (error)
>  		return XFS_ERROR(error);
>  

Is it safe to package up these other checks as well out of that inherit
code path (i.e., the read-only and shutdown check)?

> +	return xfs_setattr_nonsize(ip, iattr, flags);

It seems a little confusing (IMO) for a _setup() function to go and do
work by calling xfs_setattr_nonsize(). What about creating the helper
check function, but calling it and xfs_attr_nonsize() from
xfs_vn_setattr() (assuming _setup() doesn't return an error)?

Brian

> +}
> +
> +int
> +xfs_setattr_nonsize(
> +	struct xfs_inode	*ip,
> +	struct iattr		*iattr,
> +	int			flags)
> +{
> +	xfs_mount_t		*mp = ip->i_mount;
> +	struct inode		*inode = VFS_I(ip);
> +	int			mask = iattr->ia_valid;
> +	xfs_trans_t		*tp;
> +	int			error;
> +	uid_t			uid = 0, iuid = 0;
> +	gid_t			gid = 0, igid = 0;
> +	struct xfs_dquot	*udqp = NULL, *gdqp = NULL;
> +	struct xfs_dquot	*olddquot1 = NULL, *olddquot2 = NULL;
> +
> +	trace_xfs_setattr(ip);
> +
>  	ASSERT((mask & ATTR_SIZE) == 0);
>  
>  	/*
> @@ -592,11 +607,13 @@ xfs_setattr_nonsize(
>  		 * CAP_FSETID overrides the following restrictions:
>  		 *
>  		 * The set-user-ID and set-group-ID bits of a file will be
> -		 * cleared upon successful return from chown()
> +		 * cleared upon successful return from chown().
> +		 * Of a directory, these bits should be kept.
>  		 */
> -		if ((ip->i_d.di_mode & (S_ISUID|S_ISGID)) &&
> -		    !capable(CAP_FSETID))
> -			ip->i_d.di_mode &= ~(S_ISUID|S_ISGID);
> +		if (!S_ISDIR(inode->i_mode))
> +			if ((ip->i_d.di_mode & (S_ISUID|S_ISGID)) &&
> +			    !capable(CAP_FSETID))
> +				ip->i_d.di_mode &= ~(S_ISUID|S_ISGID);
>  
>  		/*
>  		 * Change the ownerships and register quota modifications
> @@ -915,7 +932,7 @@ xfs_vn_setattr(
>  {
>  	if (iattr->ia_valid & ATTR_SIZE)
>  		return -xfs_setattr_size(XFS_I(dentry->d_inode), iattr, 0);
> -	return -xfs_setattr_nonsize(XFS_I(dentry->d_inode), iattr, 0);
> +	return -xfs_setattr_setup(XFS_I(dentry->d_inode), iattr, 0);
>  }
>  
>  STATIC int
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: fix sgid inheritance for subdirectories inheriting default acls

[Dave Chinner]

On Fri, Jun 14, 2013 at 04:41:17PM -0300, Carlos Maiolino wrote:
> XFS removes sgid bits of subdirectories created under a directory containing a
> default acl.
> 
> When a default acl is set, it implies xfs to call xfs_setattr_nonsize() in its
> code path. Such function is shared among mkdir and chmod system calls, and
> does some checks unneeded by mkdir (calling inode_change_ok()). Such checks
> remove sgid bit from the inode after it has been granted. So, this patch wraps
> up these checks to be used only in chmod path.
> 
> Also, chmod should not remove sgid bit from an inode if this is a directory, so,
> it adds a check if S_ISDIR is set in the inode mode, into xfs_setattr_nonsize()
> 
> Done that, xfs_setattr_mode() doesn't need to re-check for group id and
> capabilities permissions, this only implies in another try to remove sgid bit
> from the directories. Such check is already done either on inode_change_ok() or
> xfs_setattr_nonsize().
> 
> This addresses SGI bug 280:
> http://oss.sgi.com/bugzilla/show_bug.cgi?id=280

I guess that shows how urgent fixing this behaviour is ;)

> 
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
>  fs/xfs/xfs_iops.c |   51 ++++++++++++++++++++++++++++++++++-----------------
>  1 files changed, 34 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index ca9ecaa..5c9c505 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -467,9 +467,6 @@ xfs_setattr_mode(
>  	ASSERT(tp);
>  	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
>  
> -	if (!in_group_p(inode->i_gid) && !capable(CAP_FSETID))
> -		mode &= ~S_ISGID;
> -
>  	ip->i_d.di_mode &= S_IFMT;
>  	ip->i_d.di_mode |= mode & ~S_IFMT;
>  
> @@ -477,23 +474,20 @@ xfs_setattr_mode(
>  	inode->i_mode |= mode & ~S_IFMT;
>  }
>  
> +/* Check inode permissions and filesystem errors
> + *
> + * Wrapper to some pre-checks needed while changing
> + * inode attributes
> + */
>  int
> -xfs_setattr_nonsize(
> +xfs_setattr_setup(

The change looks fine from a functional POV, but that is a bit of a
strange name and structure. :/

What the fix introduces is a 3rd setattr code paths:

	1. xfs_setattr_size();
	2. xfs_setattr_nonsize();

and the new one:

	3. xfs_setattr_nonsize_nomodecheck();

It's obvious that xfs_setattr_nonsize() is just
xfs_setattr_nonsize_nomodecheck() with a couple of extra checks, and
so the structure should probably be:

	xfs_setattr_nonsize_nomodecheck()
	{
		do the changes
	}

	xfs_setattr_nonsize()
	{
		inode_change_ok()

		return xfs_setattr_nonsize_nomodecheck()
	}

	xfs_set_mode()
	{
		setup iattr
		return xfs_setattr_nonsize_nomodecheck()
	}


That way the code is pretty clear that the normal setattr path does
checks on the way in, while the ACL code does not need to do them.
i.e the code documents itself as being intentional behaviour...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: fix sgid inheritance for subdirectories inheriting default acls

[Christoph Hellwig]

On Tue, Jun 18, 2013 at 02:49:31PM +1000, Dave Chinner wrote:
> 	xfs_set_mode()
> 	{
> 		setup iattr
> 		return xfs_setattr_nonsize_nomodecheck()
> 	}

I'd rather opencode updating the mode here, it's really just a tiny bit
of boilerplate code.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: fix sgid inheritance for subdirectories inheriting default acls

[Dave Chinner]

On Mon, Jun 17, 2013 at 09:55:29PM -0700, Christoph Hellwig wrote:
> On Tue, Jun 18, 2013 at 02:49:31PM +1000, Dave Chinner wrote:
> > 	xfs_set_mode()
> > 	{
> > 		setup iattr
> > 		return xfs_setattr_nonsize_nomodecheck()
> > 	}
> 
> I'd rather opencode updating the mode here, it's really just a tiny bit
> of boilerplate code.

Needs a transaction, though. Right now, there's no transaction code
at all in xfs_acl.c and I'd kind of like to keep it that way as it's
just a translation layer....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: fix sgid inheritance for subdirectories inheriting default acls

[Christoph Hellwig]

On Tue, Jun 18, 2013 at 03:53:51PM +1000, Dave Chinner wrote:
> > I'd rather opencode updating the mode here, it's really just a tiny bit
> > of boilerplate code.
> 
> Needs a transaction, though. Right now, there's no transaction code
> at all in xfs_acl.c and I'd kind of like to keep it that way as it's
> just a translation layer....

True.  Maybe as a quick hack just extend the meaning of XFS_ATTR_NOACL
to also not do the checks?  It's already the magic flag to mean that
we are called from the ACL code.

Alternatively move all of xfs_set_mode, or at least everything around
the "if (mode != inode->i_mode) " to xfs_iops.c.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: fix sgid inheritance for subdirectories inheriting default acls

[Dave Chinner]

On Mon, Jun 17, 2013 at 10:57:09PM -0700, Christoph Hellwig wrote:
> On Tue, Jun 18, 2013 at 03:53:51PM +1000, Dave Chinner wrote:
> > > I'd rather opencode updating the mode here, it's really just a tiny bit
> > > of boilerplate code.
> > 
> > Needs a transaction, though. Right now, there's no transaction code
> > at all in xfs_acl.c and I'd kind of like to keep it that way as it's
> > just a translation layer....
> 
> True.  Maybe as a quick hack just extend the meaning of XFS_ATTR_NOACL
> to also not do the checks?  It's already the magic flag to mean that
> we are called from the ACL code.

That would work, too. I hadn't connected the dots with that flag,
but it's a good fit for this case. It's probably the least intrusive
fix....

> Alternatively move all of xfs_set_mode, or at least everything around
> the "if (mode != inode->i_mode) " to xfs_iops.c.

<shrug>

I'm don't really care that much where it is ;)

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs