On Fri, Oct 28, 2016 at 3:54 PM, Miklos Szeredi <miklos@xxxxxxxxxx> 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@xxxxxxxxxx> 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-unionfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
