Message-ID: <b9e5fa41-62fd-4b3d-bb2d-24ae9d3c33da@redhat.com>
Date: Tue, 22 Apr 2025 19:25:12 +0200
From: David Hildenbrand <david@...hat.com>
To: Christoph Hellwig <hch@...radead.org>, Shivank Garg <shivankg@....com>
Cc: seanjc@...gle.com, vbabka@...e.cz, willy@...radead.org,
 akpm@...ux-foundation.org, shuah@...nel.org, pbonzini@...hat.com,
 ackerleytng@...gle.com, paul@...l-moore.com, jmorris@...ei.org,
 serge@...lyn.com, pvorel@...e.cz, bfoster@...hat.com, tabba@...gle.com,
 vannapurve@...gle.com, chao.gao@...el.com, bharata@....com, nikunj@....com,
 michael.day@....com, yan.y.zhao@...el.com, Neeraj.Upadhyay@....com,
 thomas.lendacky@....com, michael.roth@....com, aik@....com, jgg@...dia.com,
 kalyazin@...zon.com, peterx@...hat.com, linux-fsdevel@...r.kernel.org,
 linux-mm@...ck.org, linux-kernel@...r.kernel.org,
 linux-security-module@...r.kernel.org, kvm@...r.kernel.org,
 linux-kselftest@...r.kernel.org, linux-coco@...ts.linux.dev,
 Christian Göttsche <cgzones@...glemail.com>,
 Paul Moore <paul@...l-moore.com>
Subject: Re: [PATCH RFC v7 3/8] security: Export
 security_inode_init_security_anon for KVM guest_memfd

On 10.04.25 10:41, Christoph Hellwig wrote:
> On Tue, Apr 08, 2025 at 11:23:57AM +0000, Shivank Garg wrote:
>> KVM guest_memfd is implementing its own inodes to store metadata for
>> backing memory using a custom filesystem. This requires the ability to
>> initialize anonymous inode using security_inode_init_security_anon().
>>
>> As guest_memfd currently resides in the KVM module, we need to export this
>> symbol for use outside the core kernel. In the future, guest_memfd might be
>> moved to core-mm, at which point the symbols no longer would have to be
>> exported. When/if that happens is still unclear.
> 
> This really should be a EXPORT_SYMBOL_GPL, if at all.
> 
> But you really should look into a new interface in anon_inode.c that
> can be reused instead of duplicating anonymouns inode logic in kvm.ko.

I assume you mean combining the alloc_anon_inode()+
security_inode_init_security_anon(), correct?

I can see mm/secretmem.c doing the same thing, so agreed that
we're duplicating it.


Regarding your other mail, I am also starting to wonder where/why
we want security_inode_init_security_anon(). At least for
mm/secretmem.c, it was introduced by:

commit 2bfe15c5261212130f1a71f32a300bcf426443d4
Author: Christian Göttsche <cgzones@...glemail.com>
Date:   Tue Jan 25 15:33:04 2022 +0100

     mm: create security context for memfd_secret inodes
     
     Create a security context for the inodes created by memfd_secret(2) via
     the LSM hook inode_init_security_anon to allow a fine grained control.
     As secret memory areas can affect hibernation and have a global shared
     limit access control might be desirable.
     
     Signed-off-by: Christian Göttsche <cgzones@...glemail.com>
     Signed-off-by: Paul Moore <paul@...l-moore.com>


In combination with Paul's review comment [1]

"
This seems reasonable to me, and I like the idea of labeling the anon
inode as opposed to creating a new set of LSM hooks.  If we want to
apply access control policy to the memfd_secret() fds we are going to
need to attach some sort of LSM state to the inode, we might as well
use the mechanism we already have instead of inventing another one.
"


IIUC, we really only want security_inode_init_security_anon() when there
might be interest to have global access control.


Given that guest_memfd already shares many similarities with guest_memfd
(e.g., pages not swappable/migratable) and might share even more in the future
(e.g., directmap removal), I assume that we want the same thing for guest_memfd.


Would something like the following seem reasonable? We should be adding some
documentation for the new function, and I wonder if S_PRIVATE should actually
be cleared for secretmem + guest_memfd (I have no idea what this "fs-internal" flag
affects).

 From 782a6053268d8a2bddf90ba18c008495b0791710 Mon Sep 17 00:00:00 2001
From: David Hildenbrand <david@...hat.com>
Date: Tue, 22 Apr 2025 19:22:00 +0200
Subject: [PATCH] tmp

Signed-off-by: David Hildenbrand <david@...hat.com>
---
  fs/anon_inodes.c   | 20 ++++++++++++++------
  include/linux/fs.h |  1 +
  mm/secretmem.c     |  9 +--------
  3 files changed, 16 insertions(+), 14 deletions(-)

diff --git a/fs/anon_inodes.c b/fs/anon_inodes.c
index 583ac81669c24..ea51fd582deb4 100644
--- a/fs/anon_inodes.c
+++ b/fs/anon_inodes.c
@@ -55,17 +55,18 @@ 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 +76,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 +95,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 016b0fe1536e3..0fded2e3c661a 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 1b0a214ee5580..c0e459e58cb65 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);
  
-	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


[1] https://lore.kernel.org/lkml/CAHC9VhSdGeZ9x-0Hvk9mE=YMXbpk-tC5Ek+uGFGq5U+51qjChw@mail.gmail.com/

-- 
Cheers,

David / dhildenb

Powered by blists - more mailing lists