Re: Re: [PATCH] smb: client: clear setuid/setgid bit on write with cifsacl/modefromsid/posix extensions

[Jiangshan Yi]

Thanks for the review, Steve.

The patch does not over-clear the setuid/setgid bits when root writes,
because the VFS layer already performs the CAP_FSETID check upstream
of the CIFS setattr code. No changes are needed to re-check for root
in the patch.

The sole producer of the ATTR_KILL_SUID / ATTR_KILL_SGID mask in the
write path is setattr_should_drop_suidgid() (fs/attr.c:63, referring
to mainline v7.2-rc5 / commit f5098b6bae76).  The local variable
"kill" there is a bitmask of ATTR_KILL_SUID | ATTR_KILL_SGID (not a
boolean), and the function ends with:

    int setattr_should_drop_suidgid(struct mnt_idmap *idmap,
                                    struct inode *inode)
    {
            umode_t mode = inode->i_mode;
            int kill = 0;

            /* suid always must be killed */
            if (unlikely(mode & S_ISUID))
                    kill = ATTR_KILL_SUID;

            kill |= setattr_should_drop_sgid(idmap, inode);

            if (unlikely(kill && !capable(CAP_FSETID) && S_ISREG(mode)))   /* line 75 */
                    return kill;

            return 0;
    }

The decisive condition is "!capable(CAP_FSETID)" on line 75.  A process
that holds CAP_FSETID (typically root) causes the function to return 0,
so the kill mask is never produced for root writes.

The full write-path call chain is:

  write()
    -> file_remove_privs()                 [fs/inode.c:2411]
      -> file_remove_privs_flags()         [fs/inode.c:2376]
        -> dentry_needs_remove_privs()     [fs/inode.c:2349]
          -> setattr_should_drop_suidgid() [fs/attr.c:63]
               |
               +-- !capable(CAP_FSETID) -> returns non-zero kill mask
               |      -> __remove_privs() -> notify_change(KILL_S*ID)
               |           -> KILL-to-MODE translation [fs/attr.c:499]
               |              -> cifs_setattr()  <-- patch modifies here
               |
               +-- capable(CAP_FSETID) [root] -> returns 0
                      -> kill == 0, __remove_privs() skipped
                           -> notify_change() not called
                                -> cifs_setattr_* not triggered  (*)

At the point marked (*), the ATTR_KILL_SUID / ATTR_KILL_SGID flags
never appear in the iattr passed to CIFS when root writes.  Therefore
the patched branch:

    if ((attrs->ia_valid & (ATTR_KILL_SUID|ATTR_KILL_SGID)) &&
        !((sbflags & (CIFS_MOUNT_CIFS_ACL | CIFS_MOUNT_MODE_FROM_SID)) ||
          cifs_sb_master_tcon(cifs_sb)->posix_extensions))
        attrs->ia_valid &= ~ATTR_MODE;

does not execute in the root-write scenario, so there is no risk of
incorrectly clearing bits that should be preserved.  The CIFS layer
does not need to duplicate the CAP_FSETID check that the VFS already
performs.

(For completeness: the chown path in fs/open.c does set ATTR_KILL_SUID
unconditionally for non-directories, but that is the standard POSIX
chown semantic of stripping setuid/setgid on ownership change -
independent of CAP_FSETID - and is outside the scope of this write-path
fix.)

Best regards,
Jiangshan Yi