vfs: Don't modify inodes with a uid or gid unknown to the vfs

When a filesystem outside of init_user_ns is mounted it could have
uids and gids stored in it that do not map to init_user_ns.

The plan is to allow those filesystems to set i_uid to INVALID_UID and
i_gid to INVALID_GID for unmapped uids and gids and then to handle
that strange case in the vfs to ensure there is consistent robust
handling of the weirdness.

Upon a careful review of the vfs and filesystems about the only case
where there is any possibility of confusion or trouble is when the
inode is written back to disk.  In that case filesystems typically
read the inode->i_uid and inode->i_gid and write them to disk even
when just an inode timestamp is being updated.

Which leads to a rule that is very simple to implement and understand
inodes whose i_uid or i_gid is not valid may not be written.

In dealing with access times this means treat those inodes as if the
inode flag S_NOATIME was set.  Reads of the inodes appear safe and
useful, but any write or modification is disallowed.  The only inode
write that is allowed is a chown that sets the uid and gid on the
inode to valid values.  After such a chown the inode is normal and may
be treated as such.

Denying all writes to inodes with uids or gids unknown to the vfs also
prevents several oddball cases where corruption would have occurred
because the vfs does not have complete information.

One problem case that is prevented is attempting to use the gid of a
directory for new inodes where the directories sgid bit is set but the
directories gid is not mapped.

Another problem case avoided is attempting to update the evm hash
after setxattr, removexattr, and setattr.  As the evm hash includeds
the inode->i_uid or inode->i_gid not knowning the uid or gid prevents
a correct evm hash from being computed.  evm hash verification also
fails when i_uid or i_gid is unknown but that is essentially harmless
as it does not cause filesystem corruption.

Acked-by: Seth Forshee <seth.forshee@canonical.com>
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>

diff --git a/fs/attr.c b/fs/attr.c
index dd723578ddce7c6e1a536df3ceda92750e04fb26..42bb42bb3c72c206923294a3f08f0020df6c5a43 100644 (file)
--- a/fs/attr.c
+++ b/fs/attr.c
@@ -266,6 +266,14 @@ int notify_change(struct dentry * dentry, struct iattr * attr, struct inode **de
            !kgid_has_mapping(inode->i_sb->s_user_ns, attr->ia_gid))
                return -EOVERFLOW;
 
+       /* Don't allow modifications of files with invalid uids or
+        * gids unless those uids & gids are being made valid.
+        */
+       if (!(ia_valid & ATTR_UID) && !uid_valid(inode->i_uid))
+               return -EOVERFLOW;
+       if (!(ia_valid & ATTR_GID) && !gid_valid(inode->i_gid))
+               return -EOVERFLOW;
+
        error = security_inode_setattr(dentry, attr);
        if (error)
                return error;

diff --git a/fs/inode.c b/fs/inode.c
index 4ccbc21b30ce6fa791c42df70986d41e490cef10..c0ebb97fb0856a24b49af82983024006759fe501 100644 (file)
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -1617,6 +1617,13 @@ bool atime_needs_update(const struct path *path, struct inode *inode)
 
        if (inode->i_flags & S_NOATIME)
                return false;
+
+       /* Atime updates will likely cause i_uid and i_gid to be written
+        * back improprely if their true value is unknown to the vfs.
+        */
+       if (HAS_UNMAPPED_ID(inode))
+               return false;
+
        if (IS_NOATIME(inode))
                return false;
        if ((inode->i_sb->s_flags & MS_NODIRATIME) && S_ISDIR(inode->i_mode))

diff --git a/fs/namei.c b/fs/namei.c
index 8701bd9a5270329eeb043c7142f6bc37dd6195dd..840201c4c29062ff315cd71251a20ffb9bd71134 100644 (file)
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -410,6 +410,14 @@ int __inode_permission(struct inode *inode, int mask)
                 */
                if (IS_IMMUTABLE(inode))
                        return -EACCES;
+
+               /*
+                * Updating mtime will likely cause i_uid and i_gid to be
+                * written back improperly if their true value is unknown
+                * to the vfs.
+                */
+               if (HAS_UNMAPPED_ID(inode))
+                       return -EACCES;
        }
 
        retval = do_inode_permission(inode, mask);
@@ -2759,10 +2767,11 @@ EXPORT_SYMBOL(__check_sticky);
  *     c. have CAP_FOWNER capability
  *  6. If the victim is append-only or immutable we can't do antyhing with
  *     links pointing to it.
- *  7. If we were asked to remove a directory and victim isn't one - ENOTDIR.
- *  8. If we were asked to remove a non-directory and victim isn't one - EISDIR.
- *  9. We can't remove a root or mountpoint.
- * 10. We don't allow removal of NFS sillyrenamed files; it's handled by
+ *  7. If the victim has an unknown uid or gid we can't change the inode.
+ *  8. If we were asked to remove a directory and victim isn't one - ENOTDIR.
+ *  9. If we were asked to remove a non-directory and victim isn't one - EISDIR.
+ * 10. We can't remove a root or mountpoint.
+ * 11. We don't allow removal of NFS sillyrenamed files; it's handled by
  *     nfs_async_unlink().
  */
 static int may_delete(struct inode *dir, struct dentry *victim, bool isdir)
@@ -2784,7 +2793,7 @@ static int may_delete(struct inode *dir, struct dentry *victim, bool isdir)
                return -EPERM;
 
        if (check_sticky(dir, inode) || IS_APPEND(inode) ||
-           IS_IMMUTABLE(inode) || IS_SWAPFILE(inode))
+           IS_IMMUTABLE(inode) || IS_SWAPFILE(inode) || HAS_UNMAPPED_ID(inode))
                return -EPERM;
        if (isdir) {
                if (!d_is_dir(victim))
@@ -4190,6 +4199,13 @@ int vfs_link(struct dentry *old_dentry, struct inode *dir, struct dentry *new_de
         */
        if (IS_APPEND(inode) || IS_IMMUTABLE(inode))
                return -EPERM;
+       /*
+        * Updating the link count will likely cause i_uid and i_gid to
+        * be writen back improperly if their true value is unknown to
+        * the vfs.
+        */
+       if (HAS_UNMAPPED_ID(inode))
+               return -EPERM;
        if (!dir->i_op->link)
                return -EPERM;
        if (S_ISDIR(inode->i_mode))

diff --git a/fs/xattr.c b/fs/xattr.c
index 4beafc43daa58bff015f0839c78b5a65f8b8d2ab..c243905835abd25b52eb8daf061b5f11b3a5708e 100644 (file)
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -38,6 +38,13 @@ xattr_permission(struct inode *inode, const char *name, int mask)
        if (mask & MAY_WRITE) {
                if (IS_IMMUTABLE(inode) || IS_APPEND(inode))
                        return -EPERM;
+               /*
+                * Updating an xattr will likely cause i_uid and i_gid
+                * to be writen back improperly if their true value is
+                * unknown to the vfs.
+                */
+               if (HAS_UNMAPPED_ID(inode))
+                       return -EPERM;
        }
 
        /*

diff --git a/include/linux/fs.h b/include/linux/fs.h
index 375e37f42cdf6e019e8dcbc1aa687ff44509caeb..cb25ceb6d1efcf3ba696259e84ea3da31485a114 100644 (file)
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1874,6 +1874,11 @@ struct super_operations {
 #define IS_WHITEOUT(inode)     (S_ISCHR(inode->i_mode) && \
                                 (inode)->i_rdev == WHITEOUT_DEV)
 
+static inline bool HAS_UNMAPPED_ID(struct inode *inode)
+{
+       return !uid_valid(inode->i_uid) || !gid_valid(inode->i_gid);
+}
+
 /*
  * Inode state bits.  Protected by inode->i_lock
  *