> > > @@ -721,10 +721,10 @@ int chown_common(const struct path *path, uid_t user, gid_t group)
> > > return -EINVAL;
> > > if ((group != (gid_t)-1) && !setattr_vfsgid(&newattrs, gid))
> > > return -EINVAL;
> > > - if (!S_ISDIR(inode->i_mode))
> > > - newattrs.ia_valid |=
> > > - ATTR_KILL_SUID | ATTR_KILL_SGID | ATTR_KILL_PRIV;
> > > inode_lock(inode);
> > > + if (!S_ISDIR(inode->i_mode))
> > > + newattrs.ia_valid |= ATTR_KILL_SUID | ATTR_KILL_PRIV |
> > > + should_remove_sgid(mnt_userns, inode);
> >
> > This is making me stop and wonder:
> > 1. This has !S_ISDIR, should_remove_suid() has S_ISREG and
> > setattr_drop_sgid() has neither - is this consistent?
>
> I thought about that. It'very likely redundant since we deal with that
> in other parts but I need to verify all callers before we can remove
> that.
>
> > 2. SUID and PRIV are removed unconditionally and SGID is
> > removed conditionally - this is not a change of behavior
> > (at least for non-overlayfs), but is it desired???
>
> It looks that way but it isn't. The setgid bit was only killed
> unconditionally for S_IXGRP. We continue to do that. But it was always
> removed conditionally for ~S_IXGRP. The difference between this patchset
> and earlier is that it was done in settattr_prepare() or setattr_copy()
> before this change.
>
> IOW, we raised ATTR_KILL_SGID unconditionally but then only
> conditionally obeyed it in setattr_{prepare,copy}() whereas now we
> conditionally raise ATTR_KILL_SGID. That's surely a slight change but it
> just means that we don't cause bugs for filesystems that roll their own
> prepare or copy helpers and is just nicer overall.
>
Yes, that sounds right.
The point that I was trying to make and failed to articulate myself was
that chown_common() raises ATTR_KILL_SUID unconditionally,
while should_remove_suid() raises ATTR_KILL_SUID conditional
to !capable(CAP_FSETID).
Is this inconsistency in stripping SUID desired?
According to man page (I think that) it is:
"When the owner or group of an executable file is changed by an
unprivileged user, the S_ISUID and S_ISGID mode bits are cleared.
POSIX does not specify whether this also should happen when root
does the chown(); the Linux behavior depends on the kernel version,
and since Linux 2.2.13, root is treated like other users..."
So special casing SUID stripping in chown() looks intentional,
but maybe it is worth a comment.
The paragraph above *may* be interpreted that chown() should strip
S_SGID|S_IXGRP regardless of CAP_FSETID, which, as you say,
has not been the case for a while.
"...In case of a non-group-executable file (i.e., one for which the
S_IXGRP bit is not set) the S_ISGID bit indicates mandatory locking,
and is not cleared by a chown().
When the owner or group of an executable file is changed (by any user),
all capability sets for the file are cleared."
This part sounds aligned with the code.
Thanks,
Amir.
