Project

General

Profile

Bug #21004

fs: client/mds has wrong check to clear S_ISGID on chown

Added by Patrick Donnelly about 2 years ago. Updated about 2 years ago.

Status:
Resolved
Priority:
High
Category:
-
Target version:
Start date:
08/15/2017
Due date:
% Done:

0%

Source:
Q/A
Tags:
Backport:
luminous
Regression:
No
Severity:
3 - minor
Reviewed:
Affected Versions:
ceph-qa-suite:
Component(FS):
MDS, ceph-fuse, kceph
Labels (FS):
Pull request ID:


Related issues

Copied to fs - Backport #21107: luminous: fs: client/mds has wrong check to clear S_ISGID on chown Resolved

History

#1 Updated by Patrick Donnelly about 2 years ago

  • Status changed from New to In Progress

#2 Updated by Patrick Donnelly about 2 years ago

  • Subject changed from fs: client/mds do not always clear S_ISGID on chown to fs: client/mds has wrong check to clear S_ISGID on chown

Well actually the test above fails because the chown was a no-op due to an earlier chown failure. In any case I've found that S_ISGID is not cleared correctly which I identified in my own testing.

https://github.com/ceph/ceph/blob/a155b84508bb8dfff7180b73748f445e6d2d89ad/src/client/Client.cc#L6704

The condition should be:

-    } else if (kill_sguid && S_ISREG(in->mode)) {
+    } else if (kill_sguid && S_ISREG(in->mode) && (in->mode & (S_IXUSR|S_IXGRP|S_IXOTH))) {
       /* Must squash the any setuid/setgid bits with an ownership change */
-      in->mode &= ~S_ISUID;
-      if ((in->mode & (S_ISGID|S_IXGRP)) == (S_ISGID|S_IXGRP))
-       in->mode &= ~S_ISGID;
+      in->mode &= ~(S_ISUID|S_ISGID);

According to: http://pubs.opengroup.org/onlinepubs/9699919799/

#3 Updated by Zheng Yan about 2 years ago

The logical is from kernel_src/fs/attr.c

        if (ia_valid & ATTR_KILL_SUID) {
                if (mode & S_ISUID) {
                        ia_valid = attr->ia_valid |= ATTR_MODE;
                        attr->ia_mode = (inode->i_mode & ~S_ISUID);
                }
        }
        if (ia_valid & ATTR_KILL_SGID) {
                if ((mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP)) {
                        if (!(ia_valid & ATTR_MODE)) {
                                ia_valid = attr->ia_valid |= ATTR_MODE;
                                attr->ia_mode = inode->i_mode;
                        }
                        attr->ia_mode &= ~S_ISGID;
                }
        }

Besides, it's possible that mds does not handle setattr request for chmod correctly. (client does not have CEPH_CAP_AUTH_EXCL)

#4 Updated by Ken Dreyer about 2 years ago

  • Backport set to luminous

#6 Updated by Patrick Donnelly about 2 years ago

  • Status changed from In Progress to Need Review

#7 Updated by Patrick Donnelly about 2 years ago

  • Status changed from Need Review to Pending Backport

#8 Updated by Nathan Cutler about 2 years ago

  • Copied to Backport #21107: luminous: fs: client/mds has wrong check to clear S_ISGID on chown added

#9 Updated by Nathan Cutler about 2 years ago

  • Status changed from Pending Backport to Resolved

Also available in: Atom PDF