On Sat, Feb 24, 2024 at 10:48:11AM -0800, Linus Torvalds wrote:
> On Fri, 23 Feb 2024 at 21:52, Christian Brauner <brauner@xxxxxxxxxx> wrote:
> >
> > This is selinux. So I think this is a misunderstanding. This isn't
> > something we can fix in the kernel.
>
> Sure it is. SELinux just goes by what the kernel tells it anyway.
>
> Presumably this is purely about the fact that the inode in question
> *used* to be that magical 'anon_inode_inode' that is shared when you
> don't want or need a separate inode allocation. I assume it doesn't
> even look at that, it just looks at the 'anon_inode_fs_type' thing (or
> maybe at the anon_inode_mnt->mnt_sb that is created by kern_mount in
> anon_inode_init?)
>
> IOW, isn't the *only* difference that selinux can actually see just
> the inode allocation? It used to be that
>
> inode = anon_inode_getfile();
>
> now it is
>
> inode = new_inode_pseudo(pidfdfs_sb);
>
> and instead of sharing one single inode (like anon_inode_getfile()
> does unless you ask for separate inodes), it now shares the dentry
> instead (for the same pid).
>
> Would selinux be happy if the inode allocation just used the
> anon_inode superblock instead of pidfdfs_sb?
No, unfortunately not. The core issue is that anon_inode_getfile() isn't
subject to any LSM hooks which is what pidfds used. But dentry_open() is
via security_file_open(). LSMs wanted to have a say in pidfd mediation
which is now possible. So the switch to dentry_open() is what is causing
the issue.
But here's a straightforward fix appended. We let pidfs.c use that fix
as and then we introduce a new LSM hook for pidfds that allows mediation
of pidfds and selinux can implement it when they're ready. This is
regression free and future proof. I actually tested this already today.
How does that sounds?
>From f2281c90f9c7f67c5f3d2457268cd9877acc1fa9 Mon Sep 17 00:00:00 2001
From: Christian Brauner <brauner@xxxxxxxxxx>
Date: Sat, 24 Feb 2024 19:55:46 +0100
Subject: [PATCH] FIX
Signed-off-by: Christian Brauner <brauner@xxxxxxxxxx>
---
fs/file_table.c | 4 ++--
fs/internal.h | 2 ++
fs/pidfs.c | 5 +++--
3 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/fs/file_table.c b/fs/file_table.c
index b991f90571b4..685198338c4b 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 b0c843c3fa3c..ac0d1fbd6d8d 100644
--- a/fs/internal.h
+++ b/fs/internal.h
@@ -315,3 +315,5 @@ int path_from_stashed(struct dentry **stashed, unsigned long ino,
const struct inode_operations *iops, void *data,
struct path *path);
void stashed_dentry_prune(struct dentry *dentry);
+struct file *alloc_file(const struct path *path, int flags,
+ const struct file_operations *fop);
diff --git a/fs/pidfs.c b/fs/pidfs.c
index 2e165e6911df..57585de8f973 100644
--- a/fs/pidfs.c
+++ b/fs/pidfs.c
@@ -227,8 +227,9 @@ struct file *pidfs_alloc_file(struct pid *pid, unsigned int flags)
if (ret < 0)
return ERR_PTR(ret);
- pidfd_file = dentry_open(&path, flags, current_cred());
- path_put(&path);
+ pidfd_file = alloc_file(&path, flags, &pidfs_file_operations);
+ if (IS_ERR(pidfd_file))
+ path_put(&path);
return pidfd_file;
}
--
2.43.0
