Skip to content

Commit 0bd23d0

Browse files
committed
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 <[email protected]> Signed-off-by: "Eric W. Biederman" <[email protected]>
1 parent 5f65e5c commit 0bd23d0

File tree

5 files changed

+48
-5
lines changed

5 files changed

+48
-5
lines changed

fs/attr.c

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -266,6 +266,14 @@ int notify_change(struct dentry * dentry, struct iattr * attr, struct inode **de
266266
!kgid_has_mapping(inode->i_sb->s_user_ns, attr->ia_gid))
267267
return -EOVERFLOW;
268268

269+
/* Don't allow modifications of files with invalid uids or
270+
* gids unless those uids & gids are being made valid.
271+
*/
272+
if (!(ia_valid & ATTR_UID) && !uid_valid(inode->i_uid))
273+
return -EOVERFLOW;
274+
if (!(ia_valid & ATTR_GID) && !gid_valid(inode->i_gid))
275+
return -EOVERFLOW;
276+
269277
error = security_inode_setattr(dentry, attr);
270278
if (error)
271279
return error;

fs/inode.c

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1617,6 +1617,13 @@ bool atime_needs_update(const struct path *path, struct inode *inode)
16171617

16181618
if (inode->i_flags & S_NOATIME)
16191619
return false;
1620+
1621+
/* Atime updates will likely cause i_uid and i_gid to be written
1622+
* back improprely if their true value is unknown to the vfs.
1623+
*/
1624+
if (HAS_UNMAPPED_ID(inode))
1625+
return false;
1626+
16201627
if (IS_NOATIME(inode))
16211628
return false;
16221629
if ((inode->i_sb->s_flags & MS_NODIRATIME) && S_ISDIR(inode->i_mode))

fs/namei.c

Lines changed: 21 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -410,6 +410,14 @@ int __inode_permission(struct inode *inode, int mask)
410410
*/
411411
if (IS_IMMUTABLE(inode))
412412
return -EACCES;
413+
414+
/*
415+
* Updating mtime will likely cause i_uid and i_gid to be
416+
* written back improperly if their true value is unknown
417+
* to the vfs.
418+
*/
419+
if (HAS_UNMAPPED_ID(inode))
420+
return -EACCES;
413421
}
414422

415423
retval = do_inode_permission(inode, mask);
@@ -2759,10 +2767,11 @@ EXPORT_SYMBOL(__check_sticky);
27592767
* c. have CAP_FOWNER capability
27602768
* 6. If the victim is append-only or immutable we can't do antyhing with
27612769
* links pointing to it.
2762-
* 7. If we were asked to remove a directory and victim isn't one - ENOTDIR.
2763-
* 8. If we were asked to remove a non-directory and victim isn't one - EISDIR.
2764-
* 9. We can't remove a root or mountpoint.
2765-
* 10. We don't allow removal of NFS sillyrenamed files; it's handled by
2770+
* 7. If the victim has an unknown uid or gid we can't change the inode.
2771+
* 8. If we were asked to remove a directory and victim isn't one - ENOTDIR.
2772+
* 9. If we were asked to remove a non-directory and victim isn't one - EISDIR.
2773+
* 10. We can't remove a root or mountpoint.
2774+
* 11. We don't allow removal of NFS sillyrenamed files; it's handled by
27662775
* nfs_async_unlink().
27672776
*/
27682777
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)
27842793
return -EPERM;
27852794

27862795
if (check_sticky(dir, inode) || IS_APPEND(inode) ||
2787-
IS_IMMUTABLE(inode) || IS_SWAPFILE(inode))
2796+
IS_IMMUTABLE(inode) || IS_SWAPFILE(inode) || HAS_UNMAPPED_ID(inode))
27882797
return -EPERM;
27892798
if (isdir) {
27902799
if (!d_is_dir(victim))
@@ -4190,6 +4199,13 @@ int vfs_link(struct dentry *old_dentry, struct inode *dir, struct dentry *new_de
41904199
*/
41914200
if (IS_APPEND(inode) || IS_IMMUTABLE(inode))
41924201
return -EPERM;
4202+
/*
4203+
* Updating the link count will likely cause i_uid and i_gid to
4204+
* be writen back improperly if their true value is unknown to
4205+
* the vfs.
4206+
*/
4207+
if (HAS_UNMAPPED_ID(inode))
4208+
return -EPERM;
41934209
if (!dir->i_op->link)
41944210
return -EPERM;
41954211
if (S_ISDIR(inode->i_mode))

fs/xattr.c

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,13 @@ xattr_permission(struct inode *inode, const char *name, int mask)
3838
if (mask & MAY_WRITE) {
3939
if (IS_IMMUTABLE(inode) || IS_APPEND(inode))
4040
return -EPERM;
41+
/*
42+
* Updating an xattr will likely cause i_uid and i_gid
43+
* to be writen back improperly if their true value is
44+
* unknown to the vfs.
45+
*/
46+
if (HAS_UNMAPPED_ID(inode))
47+
return -EPERM;
4148
}
4249

4350
/*

include/linux/fs.h

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1874,6 +1874,11 @@ struct super_operations {
18741874
#define IS_WHITEOUT(inode) (S_ISCHR(inode->i_mode) && \
18751875
(inode)->i_rdev == WHITEOUT_DEV)
18761876

1877+
static inline bool HAS_UNMAPPED_ID(struct inode *inode)
1878+
{
1879+
return !uid_valid(inode->i_uid) || !gid_valid(inode->i_gid);
1880+
}
1881+
18771882
/*
18781883
* Inode state bits. Protected by inode->i_lock
18791884
*

0 commit comments

Comments
 (0)