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

[Steve French <smfrench@gmail.com>]

doesn't the bit only get cleared if it is a non-root user? Does the
patch need changes to see if root is writing?  When I asked AI it
responded with:

On modern Linux:
A process without CAP_FSETID will have the setuid/setgid bits cleared
when writing to a regular file.
A process with CAP_FSETID (typically root, though capabilities can be
granted independently) may preserve them.

On Wed, Jul 15, 2026 at 9:23 PM Jiangshan Yi <yijiangshan@kylinos.cn> wrote:
>
> When a file has the setuid or setgid bit set and is written to, the VFS
> strips those bits and issues a setattr with ATTR_KILL_SUID/ATTR_KILL_SGID
> together with an ATTR_MODE carrying the already-cleared mode.
>
> Both cifs_setattr_unix() and cifs_setattr_nounix() unconditionally dropped
> ATTR_MODE in that case:
>
>         /* skip mode change if it's just for clearing setuid/setgid */
>         if (attrs->ia_valid & (ATTR_KILL_SUID|ATTR_KILL_SGID))
>                 attrs->ia_valid &= ~ATTR_MODE;
>
> This is fine for the default mount, where the mode is only emulated via
> the DOS read-only attribute and cannot represent the setuid/setgid bits
> anyway.  However, with the "cifsacl" or "modefromsid" mount options the
> mode is stored on the server through an ACL (id_mode_to_cifs_acl()), with
> the SMB3.1.1 POSIX extensions the mode is sent to the server directly,
> and with the SMB1 Unix extensions (cifs_setattr_unix) the mode is sent
> via CIFSSMBUnixSetPathInfo().  In all those cases dropping ATTR_MODE means
> the cleared mode is never pushed to the server, so the setuid/setgid bit
> survives the write.
>
> This is a security issue: on local filesystems the setuid bit is stripped
> when a file is written, but over these cifs.ko mounts the bit persists on
> the server, potentially allowing an unexpected privilege escalation on
> subsequent execution.
>
> Fix this in two places:
>
>   1. cifs_setattr_nounix(): only take the "skip mode change" shortcut
>      when the mode is emulated via the DOS read-only attribute (i.e.
>      neither cifsacl/modefromsid nor the SMB3.1.1 POSIX extensions are
>      in effect), so that the cleared mode is propagated to the server
>      in the ACL / POSIX cases.
>
>   2. cifs_setattr_unix(): this function is only called when Unix
>      extensions are in effect, so the mode is always stored on the
>      server.  Remove the shortcut entirely so that the cleared mode is
>      always pushed.
>
> Fixes: d32c4f2626ac ("CIFS: ignore mode change if it's just for clearing setuid/setgid bits")
> Cc: stable@vger.kernel.org
> Signed-off-by: Jiangshan Yi <yijiangshan@kylinos.cn>
> ---
>  fs/smb/client/inode.c | 29 ++++++++++++++++++++++++-----
>  1 file changed, 24 insertions(+), 5 deletions(-)
>
> diff --git a/fs/smb/client/inode.c b/fs/smb/client/inode.c
> index deed04dd9b91..e1cecd58ea9d 100644
> --- a/fs/smb/client/inode.c
> +++ b/fs/smb/client/inode.c
> @@ -3200,9 +3200,13 @@ cifs_setattr_unix(struct dentry *direntry, struct iattr *attrs)
>                 attrs->ia_valid &= ~(ATTR_CTIME | ATTR_MTIME);
>         }
>
> -       /* skip mode change if it's just for clearing setuid/setgid */
> -       if (attrs->ia_valid & (ATTR_KILL_SUID|ATTR_KILL_SGID))
> -               attrs->ia_valid &= ~ATTR_MODE;
> +       /*
> +        * This function is only called when Unix extensions are in effect,
> +        * so the mode is always sent to and stored on the server.  Do not
> +        * skip the mode change when clearing setuid/setgid bits: dropping
> +        * ATTR_MODE here would leave those bits set on the server after a
> +        * write, which is a security issue.
> +        */
>
>         args = kmalloc_obj(*args);
>         if (args == NULL) {
> @@ -3400,8 +3404,23 @@ cifs_setattr_nounix(struct dentry *direntry, struct iattr *attrs)
>                 attrs->ia_valid &= ~(ATTR_UID | ATTR_GID);
>         }
>
> -       /* skip mode change if it's just for clearing setuid/setgid */
> -       if (attrs->ia_valid & (ATTR_KILL_SUID|ATTR_KILL_SGID))
> +       /*
> +        * Skip the mode change if it is only being done to clear the
> +        * setuid/setgid bits *and* the mode is emulated via the DOS
> +        * read-only attribute (the default, non-ACL case), which cannot
> +        * represent the setuid/setgid bits anyway.
> +        *
> +        * When the mode is instead stored on the server - i.e. with the
> +        * cifsacl or modefromsid mount options (via an ACL) or with the
> +        * SMB3.1.1 POSIX extensions - the cleared mode must be pushed to
> +        * the server.  Dropping ATTR_MODE here would leave the setuid/
> +        * setgid bit set on the server after a write, which is a security
> +        * issue (the bits are not stripped as they are on local
> +        * filesystems).
> +        */
> +       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;
>
>         if (attrs->ia_valid & ATTR_MODE) {
> --
> 2.25.1
>
>


-- 
Thanks,

Steve