| Message ID | 20161028125457.GA8412@veci.piliscsaba.szeredi.hu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Oct 28, 2016 at 3:54 PM, Miklos Szeredi <miklos@szeredi.hu> wrote: > On Fri, Oct 28, 2016 at 07:47:12AM +0300, Amir Goldstein wrote: >> On Fri, Oct 28, 2016 at 1:37 AM, Vivek Goyal <vgoyal@redhat.com> wrote: >> > On Wed, Oct 26, 2016 at 09:30:16PM +0300, Amir Goldstein wrote: >> >> Since operations on upper are performed using mounter's credentials, >> >> we need to call posix_acl_update_mode() with current credentials on >> >> overlay inode to possibly copy-up and clear setgid bit, before setting >> >> posix ACLs on upper inode. >> >> >> >> Also wrap posix acl handlers with #ifdef CONFIG_FS_POSIX_ACL to >> >> avoid compiler warning for implicit declaration of function >> >> 'posix_acl_update_mode' on build without that config option. >> >> >> >> This change fixes xfstest generic/375, which failed to clear the >> >> setgid bit in the following test case over overlayfs: >> >> >> >> touch $testfile >> >> chown 100:100 $testfile >> >> chmod 2755 $testfile >> >> _runas -u 100 -g 101 -- setfacl -m u::rwx,g::rwx,o::rwx $testfile > > Instead of calculating and setting the equivalent mode in overlayfs code (as > well as in the upper layer later), how about just clearing the sgid bit when > necessary? > > Untested patch follows. > Tested your patch. It fixes generic/375 and passes xfs/pjd/union tests. You may re-cycle my commit message (relevant parts) and either keep my Signed-off-by or add Tested-by me Thanks, Amir. > Thanks, > Miklos > > --- > fs/overlayfs/super.c | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > --- a/fs/overlayfs/super.c > +++ b/fs/overlayfs/super.c > @@ -562,6 +562,21 @@ ovl_posix_acl_xattr_set(const struct xat > > posix_acl_release(acl); > > + /* > + * Check if sgid bit needs to be cleared (actual setacl operation will > + * be done with mounter's capabilities and so that won't do it for us). > + */ > + if (unlikely(inode->i_mode & S_ISGID) && > + handler->flags == ACL_TYPE_ACCESS && > + !in_group_p(inode->i_gid) && > + !capable_wrt_inode_uidgid(inode, CAP_FSETID)) { > + struct iattr iattr = { .ia_valid = ATTR_KILL_SGID }; > + > + err = ovl_setattr(dentry, &iattr); > + if (err) > + return err; > + } > + > err = ovl_xattr_set(dentry, handler->name, value, size, flags); > if (!err) > ovl_copyattr(ovl_inode_real(inode, NULL), inode); -- 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
--- a/fs/overlayfs/super.c +++ b/fs/overlayfs/super.c @@ -562,6 +562,21 @@ ovl_posix_acl_xattr_set(const struct xat posix_acl_release(acl); + /* + * Check if sgid bit needs to be cleared (actual setacl operation will + * be done with mounter's capabilities and so that won't do it for us). + */ + if (unlikely(inode->i_mode & S_ISGID) && + handler->flags == ACL_TYPE_ACCESS && + !in_group_p(inode->i_gid) && + !capable_wrt_inode_uidgid(inode, CAP_FSETID)) { + struct iattr iattr = { .ia_valid = ATTR_KILL_SGID }; + + err = ovl_setattr(dentry, &iattr); + if (err) + return err; + } + err = ovl_xattr_set(dentry, handler->name, value, size, flags); if (!err) ovl_copyattr(ovl_inode_real(inode, NULL), inode);