Re: [PATCH] xfs: do not clear S_ISUID|S_ISGID for idmapped mounts

[Dave Chinner <david@fromorbit.com>]

On Tue, Feb 22, 2022 at 05:54:07PM +0300, Andrey Zhadchenko wrote:
> On 2/22/22 15:36, Christian Brauner wrote:
> > > > > Because as of right now the code seems to imply that the xfs code itself
> > > > > is responsible for stripping s{g,u}id bits for all files whereas it is
> > > > > the vfs that does it for any non-directory. So I'd propose to either try
> > > > > and switch that code to setattr_copy() or to do open-code the
> > > > > setattr_copy() check:
> 
> I did some more research on it and seems like modes are already stripped
> enough.
> 
> notify_change() -> inode->i_op->setattr() -> xfs_vn_setattr() ->
> xfs_vn_change_ok() -> prepare_setattr()
> which has the following:
>         if (!in_group_p((ia_valid & ATTR_GID) ? attr->ia_gid :
>                          i_gid_into_mnt(mnt_userns, inode)) &&
>              !capable_wrt_inode_uidgid(mnt_userns, inode, CAP_FSETID))
>                  attr->ia_mode &= ~S_ISGID;
> 
> After xfs_vn_change_ok() xfs_setattr_nonsize() is finally called and
> additionally strips sgid and suid.
> 
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index 09211e1d08ad..7fda5ff3ef17 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -767,16 +767,6 @@ xfs_setattr_nonsize(
>                 gid = (mask & ATTR_GID) ? iattr->ia_gid : igid;
>                 uid = (mask & ATTR_UID) ? iattr->ia_uid : iuid;
> 
> -               /*
> -                * 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()
> -                */
> -               if ((inode->i_mode & (S_ISUID|S_ISGID)) &&
> -                   !capable(CAP_FSETID))
> -                       inode->i_mode &= ~(S_ISUID|S_ISGID);

THis code has been in XFS since 1997 - it addressed shortcomings in
the Irix chown implementation w.r.t. the requirements of CAP_CHOWN
and CAP_FSETID in _POSIX_CHOWN_RESTRICTED configurations.

If the VFS handles all this correctly these days then, yes, we can
just get rid of this code - it's legacy code and we should behave
consistently across all filesystems w.r.t. su/gid files.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com