From 5c083cda15f2ff52093c1fae1e25d540fb162940 Mon Sep 17 00:00:00 2001 From: Christian Brauner Date: Mon, 19 Feb 2024 16:30:57 +0100 Subject: [PATCH] pidfs: convert to path_from_stashed() helper Moving pidfds from the anonymous inode infrastructure to a separate tiny in-kernel filesystem similar to sockfs, pipefs, and anon_inodefs causes Selinux denials and thus various userspace components that make heavy use of pidfds to fail if we would be using dentry_open() directly. pidfds used anon_inode_getfile() which never was subject to any LSM hooks. But dentry_open() is and that would cause regressions: Feb 23 12:09:58 fed1 audit[353]: AVC avc: denied { read write open } for pid=353 comm="systemd-userdbd" path="pidfd:[709]" dev="pidfs" ino=709 scontext=system_u:system_r:systemd_userdbd_t:> The failures that are seen are selinux denials. But the core failure is dbus-broker. That cascades into other services failing that depend on dbus-broker. For example, when dbus-broker fails to start polkit and all the others won't be able to work because they depend on dbus-broker. The reason for dbus-broker failing is because it doesn't handle failures for SO_PEERPIDFD correctly. Last kernel release (either v6.7 or v6.6, I'm not completely sure right now) we introduced SO_PEERPIDFD (and SCM_PIDFD). SO_PEERPIDFD allows dbus-broker and polkit and others to receive a pidfd for the peer of an AF_UNIX socket. This is the first time in the history of Linux that we can safely authenticate clients in a race-free manner. dbus-broker immediately made use of this but messed up the error checking. It only allowed EINVAL as a valid failure for SO_PEERPIDFD. That's obviously problematic not just because of LSM denials but because of seccomp denials that would prevent SO_PEERPIDFD from working; or any other new error code from there. So this is catching a flawed implementation in dbus-broker as well. It has to fallback to the old pid-based authentication when SO_PEERPIDFD doesn't work no matter the reasons otherwise it'll always risk such failures. So overall that LSM denial should not have caused dbus-broker to fail. It can never assume that a feature released one kernel ago like SO_PEERPIDFD can be assumed to be available. So, the next fix separate from the selinux policy update is to fix dbus-broker at [3]. That should make it into Fedora as well. In addition the selinux reference policy should also be updated. See [4] for that. If Selinux is in enforcing mode in userspace and it encounters anything that it doesn't know about it will deny it by default. And the policy is entirely in userspace including declaring new types for stuff like nsfs or pidfs to allow it. There's just nothing to do in the kernel so we can't use dentry_open(). So instead we use alloc_file(). Once selinux is ready we can switch to dentry_open() or we introduce separate LSM hook for pidfds. Link: https://bugzilla.redhat.com/show_bug.cgi?id=2265630 [1] Link: https://github.com/fedora-selinux/selinux-policy/pull/2050 [2] Link: https://github.com/bus1/dbus-broker/pull/343 [3] Link: https://github.com/SELinuxProject/refpolicy/pull/762 [4] Reported-by: Nathan Chancellor Link: https://lore.kernel.org/r/20240222190334.GA412503@dev-arch.thelio-3990X Link: https://lore.kernel.org/r/20240218-neufahrzeuge-brauhaus-fb0eb6459771@brauner Signed-off-by: Christian Brauner --- fs/file_table.c | 4 ++-- fs/internal.h | 5 ++++- fs/libfs.c | 12 ++++++++--- fs/nsfs.c | 7 ++++--- fs/pidfs.c | 51 ++++++++++++++++++++++++++------------------- include/linux/pid.h | 1 + kernel/pid.c | 1 + 7 files changed, 51 insertions(+), 30 deletions(-) diff --git a/fs/file_table.c b/fs/file_table.c index b991f90571b4d..685198338c4b6 100644 --- a/fs/file_table.c +++ b/fs/file_table.c @@ -282,8 +282,8 @@ struct file *alloc_empty_backing_file(int flags, const struct cred *cred) * @flags: O_... flags with which the new file will be opened * @fop: the 'struct file_operations' for the new file */ -static struct file *alloc_file(const struct path *path, int flags, - const struct file_operations *fop) +struct file *alloc_file(const struct path *path, int flags, + const struct file_operations *fop) { struct file *file; diff --git a/fs/internal.h b/fs/internal.h index cfddaec6fbf64..7b14f05efa96f 100644 --- a/fs/internal.h +++ b/fs/internal.h @@ -312,4 +312,7 @@ struct mnt_idmap *mnt_idmap_get(struct mnt_idmap *idmap); void mnt_idmap_put(struct mnt_idmap *idmap); int path_from_stashed(struct dentry **stashed, unsigned long ino, struct vfsmount *mnt, const struct file_operations *fops, - void *data, struct path *path); + const struct inode_operations *iops, void *data, + struct path *path); +struct file *alloc_file(const struct path *path, int flags, + const struct file_operations *fop); diff --git a/fs/libfs.c b/fs/libfs.c index 2a55e87e1439a..ab8f6af14729f 100644 --- a/fs/libfs.c +++ b/fs/libfs.c @@ -1990,6 +1990,7 @@ static inline struct dentry *get_stashed_dentry(struct dentry *stashed) static struct dentry *stash_dentry(struct dentry **stashed, unsigned long ino, struct super_block *sb, const struct file_operations *fops, + const struct inode_operations *iops, void *data) { struct dentry *dentry; @@ -2008,7 +2009,10 @@ static struct dentry *stash_dentry(struct dentry **stashed, unsigned long ino, inode->i_ino = ino; inode->i_flags |= S_IMMUTABLE; inode->i_mode = S_IFREG | S_IRUGO; - inode->i_fop = fops; + if (iops) + inode->i_op = iops; + if (fops) + inode->i_fop = fops; inode->i_private = data; simple_inode_init_ts(inode); @@ -2030,6 +2034,7 @@ static struct dentry *stash_dentry(struct dentry **stashed, unsigned long ino, * @stashed: where to retrieve or stash dentry * @ino: inode number to use * @mnt: mnt of the filesystems to use + * @iops: inode operations to use * @fops: file operations to use * @data: data to store in inode->i_private * @path: path to create @@ -2048,7 +2053,8 @@ static struct dentry *stash_dentry(struct dentry **stashed, unsigned long ino, */ int path_from_stashed(struct dentry **stashed, unsigned long ino, struct vfsmount *mnt, const struct file_operations *fops, - void *data, struct path *path) + const struct inode_operations *iops, void *data, + struct path *path) { struct dentry *dentry; int ret = 0; @@ -2057,7 +2063,7 @@ int path_from_stashed(struct dentry **stashed, unsigned long ino, if (dentry) goto out_path; - dentry = stash_dentry(stashed, ino, mnt->mnt_sb, fops, data); + dentry = stash_dentry(stashed, ino, mnt->mnt_sb, fops, iops, data); if (IS_ERR(dentry)) return PTR_ERR(dentry); ret = 1; diff --git a/fs/nsfs.c b/fs/nsfs.c index 39dc2604bec01..e2da645c3d02d 100644 --- a/fs/nsfs.c +++ b/fs/nsfs.c @@ -67,7 +67,7 @@ int ns_get_path_cb(struct path *path, ns_get_path_helper_t *ns_get_cb, if (!ns) return -ENOENT; ret = path_from_stashed(&ns->stashed, ns->inum, nsfs_mnt, - &ns_file_operations, ns, path); + &ns_file_operations, NULL, ns, path); if (ret <= 0 && ret != -EAGAIN) ns->ops->put(ns); } while (ret == -EAGAIN); @@ -122,8 +122,9 @@ int open_related_ns(struct ns_common *ns, return PTR_ERR(relative); } - err = path_from_stashed(&relative->stashed, relative->inum, nsfs_mnt, - &ns_file_operations, relative, &path); + err = path_from_stashed(&relative->stashed, relative->inum, + nsfs_mnt, &ns_file_operations, NULL, + relative, &path); if (err <= 0 && err != -EAGAIN) relative->ops->put(relative); } while (err == -EAGAIN); diff --git a/fs/pidfs.c b/fs/pidfs.c index 2e1b447a9f3f3..521b08318dd72 100644 --- a/fs/pidfs.c +++ b/fs/pidfs.c @@ -14,6 +14,8 @@ #include #include +#include "internal.h" + static int pidfd_release(struct inode *inode, struct file *file) { #ifndef CONFIG_FS_PID @@ -186,9 +188,21 @@ static char *pidfs_dname(struct dentry *dentry, char *buffer, int buflen) d_inode(dentry)->i_ino); } +static void pidfdfs_prune_dentry(struct dentry *dentry) +{ + struct inode *inode; + + inode = d_inode(dentry); + if (inode) { + struct pid *pid = inode->i_private; + WRITE_ONCE(pid->stashed, NULL); + } +} + static const struct dentry_operations pidfs_dentry_operations = { .d_delete = always_delete_dentry, .d_dname = pidfs_dname, + .d_prune = pidfdfs_prune_dentry, }; static int pidfs_init_fs_context(struct fs_context *fc) @@ -213,29 +227,24 @@ static struct file_system_type pidfs_type = { struct file *pidfs_alloc_file(struct pid *pid, unsigned int flags) { - struct inode *inode; struct file *pidfd_file; - - inode = iget_locked(pidfs_sb, pid->ino); - if (!inode) - return ERR_PTR(-ENOMEM); - - if (inode->i_state & I_NEW) { - inode->i_ino = pid->ino; - inode->i_mode = S_IFREG | S_IRUGO; - inode->i_op = &pidfs_inode_operations; - inode->i_fop = &pidfs_file_operations; - inode->i_flags |= S_IMMUTABLE; - inode->i_private = get_pid(pid); - simple_inode_init_ts(inode); - unlock_new_inode(inode); - } - - pidfd_file = alloc_file_pseudo(inode, pidfs_mnt, "", flags, - &pidfs_file_operations); + struct path path; + int ret; + + do { + ret = path_from_stashed(&pid->stashed, pid->ino, pidfs_mnt, + &pidfs_file_operations, + &pidfs_inode_operations, get_pid(pid), + &path); + if (ret <= 0 && ret != -EAGAIN) + put_pid(pid); + } while (ret == -EAGAIN); + if (ret < 0) + return ERR_PTR(ret); + + pidfd_file = alloc_file(&path, flags, &pidfs_file_operations); if (IS_ERR(pidfd_file)) - iput(inode); - + path_put(&path); return pidfd_file; } diff --git a/include/linux/pid.h b/include/linux/pid.h index 956481128e8d4..c79a0efd02586 100644 --- a/include/linux/pid.h +++ b/include/linux/pid.h @@ -56,6 +56,7 @@ struct pid unsigned int level; spinlock_t lock; #ifdef CONFIG_FS_PID + struct dentry *stashed; unsigned long ino; #endif /* lists of tasks that use this pid */ diff --git a/kernel/pid.c b/kernel/pid.c index 7f8d029d5ad13..8ab095e9ff76a 100644 --- a/kernel/pid.c +++ b/kernel/pid.c @@ -277,6 +277,7 @@ struct pid *alloc_pid(struct pid_namespace *ns, pid_t *set_tid, if (!(ns->pid_allocated & PIDNS_ADDING)) goto out_unlock; #ifdef CONFIG_FS_PID + pid->stashed = NULL; pid->ino = ++pidfs_ino; #endif for ( ; upid >= pid->numbers; --upid) {