Use guest mem inodes instead of anonymous inodes

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 <[email protected]>

Signed-off-by Ackerley Tng <[email protected]>

---

+extern struct inode *alloc_anon_secure_inode(struct super_block *, const char *);

fs/anon_inodes.c static struct inode *anon_inode_make_secure_inode( · line 85

Quoted 5 lines · Ackerley Tng

fs/anon_inodes.c +5 -0

+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);

What is "secure" about this inode?

A kerneldoc explaining that would probably help.

Quoted 1 line

+extern struct inode *alloc_anon_secure_inode(struct super_block *, const char *);

No need for the extern here. Spelling out the parameter names in protypes is nice, though. (and fix the long line while you're at it).

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 <[email protected]>

Signed-off-by Ackerley Tng <[email protected]>

---
 fs/anon_inodes.c   | 22 ++++++++++++++++------
 include/linux/fs.h |  1 +
 mm/secretmem.c     |  9 +--------
 3 files changed, 18 insertions(+), 14 deletions(-)

fs/anon_inodes.c +16 -6

index 583ac81669c2..4c3110378647 100644
@@ -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;

include/linux/fs.h +1 -0

index 016b0fe1536e..0fded2e3c661 100644
@@ -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;
 

mm/secretmem.c +0 -0

index 1b0a214ee558..c0e459e58cb6 100644

mm/secretmem.c +0 -7

-	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

(added Paul Moore for selinux bits)

Quoted 89 lines

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 <[email protected]>

Signed-off-by Ackerley Tng <[email protected]>

---
 fs/anon_inodes.c   | 22 ++++++++++++++++------
 include/linux/fs.h |  1 +
 mm/secretmem.c     |  9 +--------
 3 files changed, 18 insertions(+), 14 deletions(-)

fs/anon_inodes.c +16 -6

index 583ac81669c2..4c3110378647 100644
@@ -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;

include/linux/fs.h +1 -0

index 016b0fe1536e..0fded2e3c661 100644
@@ -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;
 

mm/secretmem.c +0 -1

index 1b0a214ee558..c0e459e58cb6 100644
@@ -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;
 

mm/secretmem.c static struct file *secretmem_file_create(unsigned long flags) · line 201

Quoted 4 lines

mm/secretmem.c +1 -1

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

Quoted 11 lines

mm/secretmem.c +0 -7

-     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

Quoted 2 lines · Mike Rapoport

(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 90 lines · Ackerley Tng

Quoted 89 lines

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 <[email protected]>

Signed-off-by Ackerley Tng <[email protected]>

---
 fs/anon_inodes.c   | 22 ++++++++++++++++------
 include/linux/fs.h |  1 +
 mm/secretmem.c     |  9 +--------
 3 files changed, 18 insertions(+), 14 deletions(-)

fs/anon_inodes.c +16 -6

index 583ac81669c2..4c3110378647 100644
@@ -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;

include/linux/fs.h +1 -0

index 016b0fe1536e..0fded2e3c661 100644
@@ -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;
 

mm/secretmem.c +0 -1

index 1b0a214ee558..c0e459e58cb6 100644
@@ -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;
 

Quoted 8 lines

mm/secretmem.c static struct file *secretmem_file_create(unsigned long flags) · line 201

Quoted 4 lines

mm/secretmem.c +1 -1

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

Quoted 3 lines · Mike Rapoport

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 2 lines

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.

Quoted 7 lines · Paul Moore

Quoted 3 lines · Mike Rapoport

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 8 lines

Quoted 2 lines

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]

Quoted 8 lines · Ira Weiny

Quoted 7 lines · Paul Moore

Quoted 3 lines · Mike Rapoport

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 8 lines

Quoted 8 lines

Quoted 2 lines

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.

Quoted 7 lines

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.

Quoted 1 line

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

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 <[email protected]>

Signed-off-by Ackerley Tng <[email protected]>

---

guest_memfd's inode represents memory the guest_memfd is providing. guest_memfd's file represents a struct kvm's view of that memory.

Using a custom inode allows customization of the inode teardown process via callbacks. For example, ->evict_inode() allows customization of the truncation process on file close, and ->destroy_inode() and ->free_inode() allow customization of the inode freeing process.

Customizing the truncation process allows flexibility in management of guest_memfd memory and customization of the inode freeing process allows proper cleanup of memory metadata stored on the inode.

Memory metadata is more appropriately stored on the inode (as opposed to the file), since the metadata is for the memory and is not unique to a specific binding and struct kvm.

Signed-off-by Ackerley Tng <[email protected]>

Signed-off-by Fuad Tabba <[email protected]>

---
  include/uapi/linux/magic.h |   1 +
  virt/kvm/guest_memfd.c     | 134 +++++++++++++++++++++++++++++++------
  virt/kvm/kvm_main.c        |   7 +-
  virt/kvm/kvm_mm.h          |   9 ++-
  4 files changed, 125 insertions(+), 26 deletions(-)

include/uapi/linux/magic.h +1 -0

index bb575f3ab45e..638ca21b7a90 100644
@@ -103,5 +103,6 @@
  #define DEVMEM_MAGIC		0x454d444d	/* "DMEM" */
  #define SECRETMEM_MAGIC		0x5345434d	/* "SECM" */
  #define PID_FS_MAGIC		0x50494446	/* "PIDF" */
+#define GUEST_MEMFD_MAGIC	0x474d454d	/* "GMEM" */
 
  #endif /* __LINUX_MAGIC_H__ */

virt/kvm/guest_memfd.c +24 -2

index b2aa6bf24d3a..1283b85aeb44 100644
@@ -1,12 +1,16 @@
  // SPDX-License-Identifier: GPL-2.0
+#include <linux/anon_inodes.h>
  #include <linux/backing-dev.h>
  #include <linux/falloc.h>
+#include <linux/fs.h>
  #include <linux/kvm_host.h>
+#include <linux/pseudo_fs.h>
  #include <linux/pagemap.h>
-#include <linux/anon_inodes.h>
 
  #include "kvm_mm.h"
 
+static struct vfsmount *kvm_gmem_mnt;
+
  struct kvm_gmem {
  	struct kvm *kvm;
  	struct xarray bindings;

@@ -318,9 +322,51 @@ static struct file_operations kvm_gmem_fops = {
  	.fallocate	= kvm_gmem_fallocate,
  };
 
-void kvm_gmem_init(struct module *module)
+static const struct super_operations kvm_gmem_super_operations = {
+	.statfs		= simple_statfs,
+};
+
+static int kvm_gmem_init_fs_context(struct fs_context *fc)
+{
+	struct pseudo_fs_context *ctx;
+
+	if (!init_pseudo(fc, GUEST_MEMFD_MAGIC))
+		return -ENOMEM;
+
+	ctx = fc->fs_private;
+	ctx->ops = &kvm_gmem_super_operations;
+
+	return 0;
+}
+
+static struct file_system_type kvm_gmem_fs = {
+	.name		 = "kvm_guest_memory",