Re: [PATCH 1/2] fs: Provide function that allocates a secure anonymous inode

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

(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.

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).

I'm less familiar with guest_memfd, but generally speaking if
userspace can act on the inode/fd then we likely don't want the
S_PRIVATE flag stripped from the anon_inode.

Ackerley can you provide an explanation about why the change in
S_PRIVATE was necessary?

quoted

-     err = security_inode_init_security_anon(inode, &QSTR(anon_name), NULL);
-     if (err) {
-             file = ERR_PTR(err);
-             goto err_free_inode;
-     }
-
      file = alloc_file_pseudo(inode, secretmem_mnt, "secretmem",
                               O_RDWR, &secretmem_fops);
      if (IS_ERR(file))
--
2.49.0.1204.g71687c7c1d-goog

-- 
paul-moore.com

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.

On Thu, Jun 5, 2025 at 1:50 AM Mike Rapoport [off-list ref] wrote:

secretmem always had S_PRIVATE set because alloc_anon_inode() clears it
anyway and this patch does not change it.

Yes, my apologies, I didn't look closely enough at the code.

I'm just thinking that it makes sense to actually allow LSM/SELinux
controls that S_PRIVATE bypasses for both secretmem and guest_memfd.

It's been a while since we added the anon_inode hooks so I'd have to
go dig through the old thread to understand the logic behind marking
secretmem S_PRIVATE, especially when the
anon_inode_make_secure_inode() function cleared it.  It's entirely
possible it may have just been an oversight.

-- 
paul-moore.com

Paul Moore wrote:

On Thu, Jun 5, 2025 at 1:50 AM Mike Rapoport [off-list ref] wrote:

quoted

secretmem always had S_PRIVATE set because alloc_anon_inode() clears it
anyway and this patch does not change it.

Yes, my apologies, I didn't look closely enough at the code.

quoted

I'm just thinking that it makes sense to actually allow LSM/SELinux
controls that S_PRIVATE bypasses for both secretmem and guest_memfd.

It's been a while since we added the anon_inode hooks so I'd have to
go dig through the old thread to understand the logic behind marking
secretmem S_PRIVATE, especially when the
anon_inode_make_secure_inode() function cleared it.  It's entirely
possible it may have just been an oversight.

I'm jumping in where I don't know what I'm talking about...

But my reading of the S_PRIVATE flag is that the memory can't be mapped by
user space.  So for guest_memfd() we need !S_PRIVATE because it is
intended to be mapped by user space.  So we want the secure checks.

I think secretmem is the same.

Do I have that right?

Ira

[snip]

On 6/6/2025 8:39 PM, Ira Weiny wrote:

Paul Moore wrote:

quoted

On Thu, Jun 5, 2025 at 1:50 AM Mike Rapoport [off-list ref] wrote:

quoted

secretmem always had S_PRIVATE set because alloc_anon_inode() clears it
anyway and this patch does not change it.

Yes, my apologies, I didn't look closely enough at the code.

quoted

I'm just thinking that it makes sense to actually allow LSM/SELinux
controls that S_PRIVATE bypasses for both secretmem and guest_memfd.

It's been a while since we added the anon_inode hooks so I'd have to
go dig through the old thread to understand the logic behind marking
secretmem S_PRIVATE, especially when the
anon_inode_make_secure_inode() function cleared it.  It's entirely
possible it may have just been an oversight.

I'm jumping in where I don't know what I'm talking about...

But my reading of the S_PRIVATE flag is that the memory can't be mapped by
user space.  So for guest_memfd() we need !S_PRIVATE because it is
intended to be mapped by user space.  So we want the secure checks.

I think secretmem is the same.

Do I have that right?

Hi Mike, Paul,

If I understand correctly,
we need to clear the S_PRIVATE flag for all secure inodes. The S_PRIVATE flag was previously
set for  secretmem (via alloc_anon_inode()), which caused security checks to be 
bypassed - this was unintentional since the original anon_inode_make_secure_inode() 
was already clearing it.

Both secretmem and guest_memfd create file descriptors
(memfd_create/kvm_create_guest_memfd)
so they should be subject to LSM/SELinux security policies rather than bypassing them with S_PRIVATE?

static struct inode *anon_inode_make_secure_inode(struct super_block *s,
		const char *name, const struct inode *context_inode)
{
...
	/* Clear S_PRIVATE for all inodes*/
	inode->i_flags &= ~S_PRIVATE;
...
}

Please let me know if this conclusion makes sense?

Thanks,
Shivank

Ira

[snip]

On Mon, Jun 16, 2025 at 06:30:09PM +0530, Shivank Garg wrote:

On 6/6/2025 8:39 PM, Ira Weiny wrote:

quoted

Paul Moore wrote:

quoted

On Thu, Jun 5, 2025 at 1:50 AM Mike Rapoport [off-list ref] wrote:

quoted

secretmem always had S_PRIVATE set because alloc_anon_inode() clears it
anyway and this patch does not change it.

Yes, my apologies, I didn't look closely enough at the code.

quoted

I'm just thinking that it makes sense to actually allow LSM/SELinux
controls that S_PRIVATE bypasses for both secretmem and guest_memfd.

It's been a while since we added the anon_inode hooks so I'd have to
go dig through the old thread to understand the logic behind marking
secretmem S_PRIVATE, especially when the
anon_inode_make_secure_inode() function cleared it.  It's entirely
possible it may have just been an oversight.

anon_inode_make_secure_inode() was introduced when more than 10 versions of
secretmem already were posted so it didn't jump at me to replace
alloc_anon_inode() with anon_inode_make_secure_inode().
 

quoted

I'm jumping in where I don't know what I'm talking about...

But my reading of the S_PRIVATE flag is that the memory can't be mapped by
user space.  So for guest_memfd() we need !S_PRIVATE because it is
intended to be mapped by user space.  So we want the secure checks.

I think secretmem is the same.

Agree.

quoted

Do I have that right?

Hi Mike, Paul,

If I understand correctly,
we need to clear the S_PRIVATE flag for all secure inodes. The S_PRIVATE flag was previously
set for  secretmem (via alloc_anon_inode()), which caused security checks to be 
bypassed - this was unintentional since the original anon_inode_make_secure_inode() 
was already clearing it.

Both secretmem and guest_memfd create file descriptors
(memfd_create/kvm_create_guest_memfd)
so they should be subject to LSM/SELinux security policies rather than bypassing them with S_PRIVATE?

static struct inode *anon_inode_make_secure_inode(struct super_block *s,
		const char *name, const struct inode *context_inode)
{
...
	/* Clear S_PRIVATE for all inodes*/
	inode->i_flags &= ~S_PRIVATE;
...
}

Please let me know if this conclusion makes sense?

Yes, makes sense to me.
 

Thanks,
Shivank

-- 
Sincerely yours,
Mike.