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

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

[Jiangshan Yi]

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

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

[Steve French]

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

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

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

[Namjae Jeon]

> 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>
I will apply it to #for-next.
Thanks!