summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorLinus Torvalds <torvalds@linux-foundation.org>2022-09-20 20:08:37 +0300
committerLinus Torvalds <torvalds@linux-foundation.org>2022-09-20 20:08:37 +0300
commit84a31938831f6d2338ceffe630a1271ec2a51a59 (patch)
tree183909bfc87fc109dc6a5812800dffd0c52384ba
parentf489921dba468b3d208a5234dcbf7f21a615f395 (diff)
parentf52d74b190f8d10ec01cd5774eca77c2186c8ab7 (diff)
downloadlinux-84a31938831f6d2338ceffe630a1271ec2a51a59.tar.xz
Merge tag 'fs.fixes.v6.0-rc7' of git://git.kernel.org/pub/scm/linux/kernel/git/vfs/idmapping
Pull vfs fix from Christian Brauner: "Beginning of the merge window we introduced the vfs{g,u}id_t types in b27c82e12965 ("attr: port attribute changes to new types") and changed various codepaths over including chown_common(). When userspace passes -1 for an ownership change the ownership fields in struct iattr stay uninitialized. Usually this is fine because any code making use of any fields in struct iattr must check the ->ia_valid field whether the value of interest has been initialized. That's true for all struct iattr passing code. However, over the course of the last year with more heavy use of KMSAN we found quite a few places that got this wrong. A recent one I fixed was 3cb6ee991496 ("9p: only copy valid iattrs in 9P2000.L setattr implementation"). But we also have LSM hooks. Actually we have two. The first one is security_inode_setattr() in notify_change() which does the right thing and passes the full struct iattr down to LSMs and thus LSMs can check whether it is initialized. But then we also have security_path_chown() which passes down a path argument and the target ownership as the filesystem would see it. For the latter we now generate the target values based on struct iattr and pass it down. However, when userspace passes -1 then struct iattr isn't initialized. This patch simply initializes ->ia_vfs{g,u}id with INVALID_VFS{G,U}ID so the hook continue to see invalid ownership when -1 is passed from userspace. The only LSM that cares about the actual values is Tomoyo. The vfs codepaths don't look at these fields without ->ia_valid being set so there's no harm in initializing ->ia_vfs{g,u}id. Arguably this is also safer since we can't end up copying valid ownership values when invalid ownership values should be passed. This only affects mainline. No kernel has been released with this and thus no backport is needed. The commit is thus marked with a Fixes: tag but annotated with "# mainline only" (I didn't quite remember what Greg said about how to tell stable autoselect to not bother with fixes for mainline only)" * tag 'fs.fixes.v6.0-rc7' of git://git.kernel.org/pub/scm/linux/kernel/git/vfs/idmapping: open: always initialize ownership fields
-rw-r--r--fs/open.c2
1 files changed, 2 insertions, 0 deletions
diff --git a/fs/open.c b/fs/open.c
index 8a813fa5ca56..cf7e5c350a54 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -716,6 +716,8 @@ int chown_common(const struct path *path, uid_t user, gid_t group)
fs_userns = i_user_ns(inode);
retry_deleg:
+ newattrs.ia_vfsuid = INVALID_VFSUID;
+ newattrs.ia_vfsgid = INVALID_VFSGID;
newattrs.ia_valid = ATTR_CTIME;
if ((user != (uid_t)-1) && !setattr_vfsuid(&newattrs, uid))
return -EINVAL;