On Wed, Jun 04, 2025 at 05:13:35PM -0400, Paul Moore wrote:

On Wed, Jun 4, 2025 at 3:59 AM Mike Rapoport [off-list ref] wrote:

quoted

(added Paul Moore for selinux bits)

Thanks Mike.

I'm adding the LSM and SELinux lists too since there are others that
will be interested as well.

quoted

On Mon, Jun 02, 2025 at 12:17:54PM -0700, Ackerley Tng wrote:

quoted

The new function, alloc_anon_secure_inode(), returns an inode after
running checks in security_inode_init_security_anon().

Also refactor secretmem's file creation process to use the new
function.

Suggested-by: David Hildenbrand <redacted>
Signed-off-by: Ackerley Tng <redacted>
---
 fs/anon_inodes.c   | 22 ++++++++++++++++------
 include/linux/fs.h |  1 +
 mm/secretmem.c     |  9 +--------
 3 files changed, 18 insertions(+), 14 deletions(-)

diff --git a/fs/anon_inodes.c b/fs/anon_inodes.c
index 583ac81669c2..4c3110378647 100644
--- a/fs/anon_inodes.c
+++ b/fs/anon_inodes.c

@@ -55,17 +55,20 @@ static struct file_system_type anon_inode_fs_type = {
      .kill_sb        = kill_anon_super,
 };

-static struct inode *anon_inode_make_secure_inode(
-     const char *name,
-     const struct inode *context_inode)
+static struct inode *anon_inode_make_secure_inode(struct super_block *s,
+             const char *name, const struct inode *context_inode,
+             bool fs_internal)
 {
      struct inode *inode;
      int error;

-     inode = alloc_anon_inode(anon_inode_mnt->mnt_sb);
+     inode = alloc_anon_inode(s);
      if (IS_ERR(inode))
              return inode;
-     inode->i_flags &= ~S_PRIVATE;
+
+     if (!fs_internal)
+             inode->i_flags &= ~S_PRIVATE;
+
      error = security_inode_init_security_anon(inode, &QSTR(name),
                                                context_inode);
      if (error) {

@@ -75,6 +78,12 @@ static struct inode *anon_inode_make_secure_inode(
      return inode;
 }

+struct inode *alloc_anon_secure_inode(struct super_block *s, const char *name)
+{
+     return anon_inode_make_secure_inode(s, name, NULL, true);
+}
+EXPORT_SYMBOL_GPL(alloc_anon_secure_inode);
+
 static struct file *__anon_inode_getfile(const char *name,
                                       const struct file_operations *fops,
                                       void *priv, int flags,

@@ -88,7 +97,8 @@ static struct file *__anon_inode_getfile(const char *name,
              return ERR_PTR(-ENOENT);

      if (make_inode) {
-             inode = anon_inode_make_secure_inode(name, context_inode);
+             inode = anon_inode_make_secure_inode(anon_inode_mnt->mnt_sb,
+                                                  name, context_inode, false);
              if (IS_ERR(inode)) {
                      file = ERR_CAST(inode);
                      goto err;

diff --git a/include/linux/fs.h b/include/linux/fs.h
index 016b0fe1536e..0fded2e3c661 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h

@@ -3550,6 +3550,7 @@ extern int simple_write_begin(struct file *file, struct address_space *mapping,
 extern const struct address_space_operations ram_aops;
 extern int always_delete_dentry(const struct dentry *);
 extern struct inode *alloc_anon_inode(struct super_block *);
+extern struct inode *alloc_anon_secure_inode(struct super_block *, const char *);
 extern int simple_nosetlease(struct file *, int, struct file_lease **, void **);
 extern const struct dentry_operations simple_dentry_operations;

diff --git a/mm/secretmem.c b/mm/secretmem.c
index 1b0a214ee558..c0e459e58cb6 100644
--- a/mm/secretmem.c
+++ b/mm/secretmem.c

@@ -195,18 +195,11 @@ static struct file *secretmem_file_create(unsigned long flags)
      struct file *file;
      struct inode *inode;
      const char *anon_name = "[secretmem]";
-     int err;

-     inode = alloc_anon_inode(secretmem_mnt->mnt_sb);
+     inode = alloc_anon_secure_inode(secretmem_mnt->mnt_sb, anon_name);
      if (IS_ERR(inode))
              return ERR_CAST(inode);

I don't think we should not hide secretmem and guest_memfd inodes from
selinux, so clearing S_PRIVATE for them is not needed and you can just drop
fs_internal parameter in anon_inode_make_secure_inode()

It's especially odd since I don't see any comments or descriptions
about why this is being done.  The secretmem change is concerning as
this is user accessible and marking the inode with S_PRIVATE will
bypass a number of LSM/SELinux access controls, possibly resulting in
a security regression (one would need to dig a bit deeper to see what
is possible with secretmem and which LSM/SELinux code paths would be
affected).

secretmem always had S_PRIVATE set because alloc_anon_inode() clears it
anyway and this patch does not change it.
I'm just thinking that it makes sense to actually allow LSM/SELinux
controls that S_PRIVATE bypasses for both secretmem and guest_memfd.
 
-- 
Sincerely yours,
Mike.