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@xxxxxxxxxxxxx
