xfs: fix inode uid/gid initialization

I was debugging a creation failure using a vfs shifting patch set and discovered that xfs itself doesn't actually respect the superblock namespace in a couple of places (these showed up as files with the wrong ownership in my tests).

The fix is to convert xfs away from hand
rolling inode_init_owner() and to use the i_uid/gid_read/write functions.

Quoted 5 lines · James Bottomley

I was debugging a creation failure using a vfs shifting patch set and
discovered that xfs itself doesn't actually respect the superblock namespace in a couple of places (these showed up as files with the wrong ownership in my tests).

Can you submit your test case to xfstests? I would be good to have testing for this in the regular test runs.

Quoted 3 lines

The fix is to convert xfs away from hand
rolling inode_init_owner() and to use the i_uid/gid_read/write functions.

What about the various quota users of xfs_kuid_to_uid/gid in the create / symlink path?

I suspect they should be handle the same.

Also with your patch the di_uid/gid fields should probably just go away as they are pointless now. Something like the patch below, although it still doesn't take care of the quota issues pointed out above.

fs/xfs/xfs_inode.c +1 -1

@@ -814,18 +814,10 @@ xfs_ialloc(
 	if (ip->i_d.di_version == 1)
 		ip->i_d.di_version = 2;
 
-	inode->i_mode = mode;
+	inode_init_owner(inode, pip ? VFS_I(pip) : NULL, mode);
 	set_nlink(inode, nlink);

fs/xfs/xfs_inode.c xfs_ialloc( · line 828

Quoted 7 lines

fs/xfs/xfs_inode.c +0 -6

-	if (pip && XFS_INHERIT_GID(pip)) {
-		ip->i_d.di_gid = pip->i_d.di_gid;
-		if ((VFS_I(pip)->i_mode & S_ISGID) && S_ISDIR(mode))
-			inode->i_mode |= S_ISGID;
-	}
-

Doesn't this hunk break the "nogrpid" mount option?

Quoted 10 lines · Dave Chinner

fs/xfs/xfs_inode.c xfs_ialloc( · line 828

Quoted 8 lines

fs/xfs/xfs_inode.c +0 -3

-	if (pip && XFS_INHERIT_GID(pip)) {
-		ip->i_d.di_gid = pip->i_d.di_gid;
-		if ((VFS_I(pip)->i_mode & S_ISGID) &&

S_ISDIR(mode))

fs/xfs/xfs_inode.c +0 -3

-			inode->i_mode |= S_ISGID;
-	}
-

Doesn't this hunk break the "nogrpid" mount option?

It does.

Now that we have two different views of filesystem ids (the filesystem view and the kernel view), we have a problem in that current_fsuid/fsgid() return the kernel view but are sometimes used in filesystem code where the filesystem view shoud be used. This patch introduces helpers to produce the filesystem view of current fsuid and fsgid.

All of that said where are you getting sb->s_user_ns != &init_user_ns for an xfs filesystem? There are quite a few xfs interfaces that are
not ready for that. xfs has a very wide userspace interface of ioctls
that all needs to be looked at and addressed carefully if there is anything like this going on.

include/linux/cred.h do { \ · line 404

Quoted 3 lines

include/linux/cred.h +3 -0

+/* return the current id in the filesystem view */
+#define i_fsuid(i) from_kuid((i)->i_sb->s_user_ns, current_fsuid())
+#define i_fsgid(i) from_kgid((i)->i_sb->s_user_ns, current_fsgid())

Could we please place these helpers in fs.h?
That should allow them to become inline functions and live with the existing filesystem helpers in there.

Quoted 6 lines · James Bottomley

Now that we have two different views of filesystem ids (the filesystem view and the kernel view), we have a problem in that current_fsuid/fsgid() return the kernel view but are sometimes used in filesystem code where the filesystem view shoud be used. This patch introduces helpers to produce the filesystem view of current fsuid and fsgid.

If I am reading this right what we are seeing is that xfs explicitly
opted out of type safety with predictable results. Accidentally
confusing kuids and uids, which is potentially security issue.

All of that said where are you getting sb->s_user_ns != &init_user_ns for an xfs filesystem? There are quite a few xfs interfaces that are
not ready for that. xfs has a very wide userspace interface of
ioctls that all needs to be looked at and addressed carefully if there is anything like this going on.

I think we really need to ask if we should use kuids and kgids for the xfs internal quota code.

At the end of the day that is going to be a whole lot less error
prone.

Quoted 12 lines

Signed-off-by: James Bottomley <
[email protected]>

include/linux/cred.h +0 -0

index f0e70a1..18e9c41 100644
@@ -399,4 +399,9 @@ do {						
 \
 	*(_fsgid) = __cred->fsgid;		\
 } while(0)
 

Quoted 5 lines

include/linux/cred.h +2 -0

+/* return the current id in the filesystem view */
+#define i_fsuid(i) from_kuid((i)->i_sb->s_user_ns,

current_fsuid())
+#define i_fsgid(i) from_kgid((i)->i_sb->s_user_ns, current_fsgid())

Could we please place these helpers in fs.h?

That should allow them to become inline functions and live with the existing filesystem helpers in there.

My gut says the names disk_fsuid(i) and disk_fsgid(i) would be clearer.

Of course all of this has the challenge of error handling in the case when current_fsuid or current_fsgid do not map into the current filesystem.

Quoted 9 lines · Eric W. Biederman

Quoted 6 lines · James Bottomley

Now that we have two different views of filesystem ids (the filesystem view and the kernel view), we have a problem in that current_fsuid/fsgid() return the kernel view but are sometimes used in filesystem code where the filesystem view shoud be used. This patch introduces helpers to produce the filesystem view of current fsuid and fsgid.

Quoted 6 lines

If I am reading this right what we are seeing is that xfs explicitly
opted out of type safety with predictable results. Accidentally
confusing kuids and uids, which is potentially security issue.

All of that said where are you getting sb->s_user_ns != &init_user_ns for an xfs filesystem?

Quoted 18 lines · James Bottomley

Quoted 17 lines · Eric W. Biederman

Quoted 9 lines · James Bottomley

Now that we have two different views of filesystem ids (the filesystem view and the kernel view), we have a problem in that current_fsuid/fsgid() return the kernel view but are sometimes used
in filesystem code where the filesystem view shoud be used.
This
patch introduces helpers to produce the filesystem view of current
fsuid and fsgid.

If I am reading this right what we are seeing is that xfs explicitly
opted out of type safety with predictable results. Accidentally
confusing kuids and uids, which is potentially security issue.

Quoted 3 lines

Quoted 3 lines

All of that said where are you getting sb->s_user_ns != &init_user_ns
for an xfs filesystem?

James please answer this question:

Where are you getting sb->s_user_ns != &init_user_ns for an xfs
filesystem?

None of this matters if sb->s_user_ns == &init_user_ns.

This is signification because only xfs keeps any in-core data structure in it's on-disk encoding. So this problem is xfs specific.
So understanding how you are getting xfs to have sb->s_user_ns !=
&init_user_ns is important for discussing which direction we go with helper functions here.

xfs with sb->s_user_ns == &init_user_ns is perfectly fine and as such no fixes are needed.

Quoted 20 lines · Eric W. Biederman

Quoted 18 lines · James Bottomley

Quoted 17 lines · Eric W. Biederman

Quoted 9 lines · James Bottomley

Now that we have two different views of filesystem ids (the filesystem view and the kernel view), we have a problem in that current_fsuid/fsgid() return the kernel view but are sometimes used
in filesystem code where the filesystem view shoud be used.
This
patch introduces helpers to produce the filesystem view of current
fsuid and fsgid.

If I am reading this right what we are seeing is that xfs explicitly
opted out of type safety with predictable results. Accidentally
confusing kuids and uids, which is potentially security issue.

Quoted 8 lines

Quoted 3 lines

Quoted 3 lines

All of that said where are you getting sb->s_user_ns != &init_user_ns
for an xfs filesystem?

James please answer this question:

Where are you getting sb->s_user_ns != &init_user_ns for an xfs
filesystem?

I'm playing with a patch that allows host admin to set up an unprivileged container for a guest to use. One of the extensions is to allow anything possessing capability(CAP_SYS_ADMIN) to make s_user_ns follow mnt_ns->user_ns for new mounts (as an option). The idea was to see if root could set up an id shifted container with just the current s_user_ns infrastructure.

Quoted 10 lines

None of this matters if sb->s_user_ns == &init_user_ns.

This is signification because only xfs keeps any in-core data structure in it's on-disk encoding. So this problem is xfs specific.
So understanding how you are getting xfs to have sb->s_user_ns !=
&init_user_ns is important for discussing which direction we go with helper functions here.

xfs with sb->s_user_ns == &init_user_ns is perfectly fine and as such no fixes are needed.

So what you're saying is that unless the unprivileged container could mount the filesystem itself (i.e. only those possessing the FS_USERNS_MOUNT flag) the filesystems are going to be full of problems like this. I suppose whether it's worthwhile trying to fix them all depends on whether the ability of the administrator to set up an id shifted container is useful or not.

Quoted 46 lines · James Bottomley

Quoted 18 lines · Eric W. Biederman

Quoted 16 lines · James Bottomley

Quoted 15 lines · Eric W. Biederman

James Bottomley <[email protected]>

Quoted 6 lines · writes:

Now that we have two different views of filesystem ids (the filesystem view and the kernel view), we have a problem in that current_fsuid/fsgid() return the kernel view but are sometimes used in filesystem code where the filesystem view shoud be used. This patch introduces helpers to produce the filesystem view of current fsuid and fsgid.

If I am reading this right what we are seeing is that xfs explicitly opted out of type safety with predictable results.
Accidentally confusing kuids and uids, which is potentially
security issue.

Quoted 7 lines

Quoted 2 lines

Quoted 2 lines

All of that said where are you getting sb->s_user_ns != &init_user_ns for an xfs filesystem?

James please answer this question:

Where are you getting sb->s_user_ns != &init_user_ns for an xfs
filesystem?

I'm playing with a patch that allows host admin to set up an unprivileged container for a guest to use. One of the extensions is to allow anything possessing capability(CAP_SYS_ADMIN) to make s_user_ns follow mnt_ns->user_ns for new mounts (as an option).
The idea was to see if root could set up an id shifted container
with just the current s_user_ns infrastructure.

Quoted 11 lines

None of this matters if sb->s_user_ns == &init_user_ns.

This is signification because only xfs keeps any in-core data structure in it's on-disk encoding. So this problem is xfs specific.
So understanding how you are getting xfs to have sb->s_user_ns
!= &init_user_ns is important for discussing which direction we go with helper functions here.

xfs with sb->s_user_ns == &init_user_ns is perfectly fine and as such no fixes are needed.

Quoted 6 lines

So what you're saying is that unless the unprivileged container could mount the filesystem itself (i.e. only those possessing the FS_USERNS_MOUNT flag) the filesystems are going to be full of problems like this. I suppose whether it's worthwhile trying to fix them all depends on whether the ability of the administrator to set up an id shifted container is useful or not.

Yes. Setting s_user_ns and expecting everything to work with a review/test cycle of the filesystem to shake out any rough edges is likely to be problematic. For historical reasons I actually expect xfs is especially bad in this regard. So in practice I would definitely start a feature like that with another filesystem.

I would be happy to have a FS_S_USER_NS flag to say all that is well, and the filesystem supports s_user_ns != &init_user_ns. The bar is much lower if a trusted user with CAP_SYS_ADMIN is mounting the filesystem than if an unprivileged user is mounting the filesystem.
As we don't have to worry about specially crafted malicious
filesystem images.

In practice I think I would have passed in the user namespace via a file descriptor to mount rather than inheriting it from the mount namespace (more flexibility for roughly the same amount of code).

Quoted 55 lines · Eric W. Biederman

Quoted 46 lines · James Bottomley

Quoted 18 lines · Eric W. Biederman

Quoted 16 lines · James Bottomley

Quoted 15 lines · Eric W. Biederman

James Bottomley <[email protected]>

Quoted 6 lines · writes:

Now that we have two different views of filesystem ids (the filesystem view and the kernel view), we have a problem in that current_fsuid/fsgid() return the kernel view but are sometimes used in filesystem code where the filesystem view shoud be used. This patch introduces helpers to produce the filesystem view of current fsuid and fsgid.

If I am reading this right what we are seeing is that xfs explicitly opted out of type safety with predictable results.
Accidentally confusing kuids and uids, which is potentially
security issue.

Quoted 7 lines

Quoted 2 lines

Quoted 2 lines

All of that said where are you getting sb->s_user_ns != &init_user_ns for an xfs filesystem?

James please answer this question:

Where are you getting sb->s_user_ns != &init_user_ns for an xfs
filesystem?

I'm playing with a patch that allows host admin to set up an unprivileged container for a guest to use. One of the extensions is to allow anything possessing capability(CAP_SYS_ADMIN) to make s_user_ns follow mnt_ns->user_ns for new mounts (as an option).
The idea was to see if root could set up an id shifted container
with just the current s_user_ns infrastructure.

Quoted 11 lines

None of this matters if sb->s_user_ns == &init_user_ns.

This is signification because only xfs keeps any in-core data structure in it's on-disk encoding. So this problem is xfs specific.
So understanding how you are getting xfs to have sb->s_user_ns
!= &init_user_ns is important for discussing which direction we go with helper functions here.

xfs with sb->s_user_ns == &init_user_ns is perfectly fine and as such no fixes are needed.

Quoted 6 lines

So what you're saying is that unless the unprivileged container could mount the filesystem itself (i.e. only those possessing the FS_USERNS_MOUNT flag) the filesystems are going to be full of problems like this. I suppose whether it's worthwhile trying to fix them all depends on whether the ability of the administrator to set up an id shifted container is useful or not.

Quoted 5 lines

Yes. Setting s_user_ns and expecting everything to work with a review/test cycle of the filesystem to shake out any rough edges is likely to be problematic. For historical reasons I actually expect xfs is especially bad in this regard. So in practice I would definitely start a feature like that with another filesystem.

It's a pragmatic choice: xfs is the filesystem on my current laptop. I know xfs was once very problematic for the user namespace, but having looked through the code several times, the namespace shifts are now nicely abstracted and easy to identify, so I don't anticipate any extra difficulty today.

Quoted 10 lines

I would be happy to have a FS_S_USER_NS flag to say all that is well, and the filesystem supports s_user_ns != &init_user_ns. The bar is much lower if a trusted user with CAP_SYS_ADMIN is mounting the filesystem than if an unprivileged user is mounting the filesystem.
As we don't have to worry about specially crafted malicious
filesystem images.

In practice I think I would have passed in the user namespace via a file descriptor to mount rather than inheriting it from the mount namespace (more flexibility for roughly the same amount of code).

I agree on this, but lets leave the implementation details on the side for a while and examine the "should we do this?" question.

I can see two reasons why we might need to have this functionality

1. Orchestration system use case: the orchestration system wants to build an unprivileged container root from an image file or overlay (I think this covers docker).
2. USB (or other) device insertion redirected to container.  In this case, we'd like the mount on insertion to follow the container user_ns.

The reason I could see not bothering with this is that it doesn't fix the shift on a subtree issue and fixing that gives a system which can also be used to solve both cases above.

OK, so I'll fix up the s_user_mount problems and I'll let you sort out the internals of removing the di_uid/gid if you wish to.