[PATCH RFC v7 3/8] security: Export security_inode_init_security_anon for KVM guest_memfd
Shivank Garg
shivankg at amd.com
Thu May 8 06:37:25 UTC 2025
On 4/22/2025 10:55 PM, David Hildenbrand wrote:
> 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 at googlemail.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 at googlemail.com>
> Signed-off-by: Paul Moore <paul at paul-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).
>
Here's my understanding of S_PRIVATE flag:
1. S_PRIVATE tells the kernel that an inode is special and it should
skip the LSM permission checks (via IS_PRIVATE()):
For instance,
int security_inode_mknod(struct inode *dir, struct dentry *dentry,
umode_t mode, dev_t dev)
{
if (unlikely(IS_PRIVATE(dir)))
return 0;
return call_int_hook(inode_mknod, dir, dentry, mode, dev);
}
2. In landlock LSM:
S_PRIVATE inodes are denied from opening file and getting path from fd
syscall: open_by_handle_at
calls do_handle_open -> handle_to_path -> get_path_from_fd
static int get_path_from_fd(const s32 fd, struct path *const path)
{
...
/*
* Forbids ruleset FDs, internal filesystems (e.g. nsfs), including
* pseudo filesystems that will never be mountable (e.g. sockfs,
* pipefs).
*/
if ((fd_file(f)->f_op == &ruleset_fops) ||
(fd_file(f)->f_path.mnt->mnt_flags & MNT_INTERNAL) ||
(fd_file(f)->f_path.dentry->d_sb->s_flags & SB_NOUSER) ||
d_is_negative(fd_file(f)->f_path.dentry) ||
IS_PRIVATE(d_backing_inode(fd_file(f)->f_path.dentry)))
return -EBADFD;
Using is_nouser_or_private() in is_access_to_paths_allowed() (allows accesses
for requests with a common path)
static bool is_access_to_paths_allowed() {
...
if (is_nouser_or_private(path->dentry))
return true;
3. S_PRIVATE skips security attribute initialization in SELinux:
security/selinux/hooks.c
sb_finish_set_opts(){
...
if (inode) {
if (!IS_PRIVATE(inode))
inode_doinit_with_dentry(inode, NULL);
4. mm/shmem.c
/**
* shmem_kernel_file_setup - get an unlinked file living in tmpfs which must be
* kernel internal. There will be NO LSM permission checks against the
* underlying inode. So users of this interface must do LSM checks at a
* higher layer. The users are the big_key and shm implementations. LSM
* checks are provided at the key or shm level rather than the inode.
* @name: name for dentry (to be seen in /proc/<pid>/maps)
* @size: size to be set for the file
* @flags: VM_NORESERVE suppresses pre-accounting of the entire object size
*/
struct file *shmem_kernel_file_setup(const char *name, loff_t size, unsigned long flags)
{
return __shmem_file_setup(shm_mnt, name, size, flags, S_PRIVATE);
>From these observations, S_PRIVATE inodes are handled differently from other inodes.
It appears to bypass LSM checks (probably saving some cycles) or In other words, it
ensure the LSMs don't try to reason about these files. While it expects the details
of these inodes should not be leaked to userspace (indicated by comments around
S_PRIVATE refs).
I think we should keep the use of S_PRIVATE flag as it is for secretmem and kvm_gmem.
However, I'm uncertain about whether we still need security_inode_init_security_anon()
for inodes that are already marked S_PRIVATE.
The two seem contradictory. First, we mark an inode as private to bypass LSM checks,
but then initialize security context for it.
I'd appreciate the guidance from the security team.
> From 782a6053268d8a2bddf90ba18c008495b0791710 Mon Sep 17 00:00:00 2001
> From: David Hildenbrand <david at redhat.com>
> Date: Tue, 22 Apr 2025 19:22:00 +0200
> Subject: [PATCH] tmp
>
> Signed-off-by: David Hildenbrand <david at redhat.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))
Thanks for the patch.
I have split this change into two patches and added required documentation.
Best Regards,
Shivank
-------------- next part --------------
From 78f48437a88b3b70aa7d80a32db4f269a0804d18 Mon Sep 17 00:00:00 2001
From: David Hildenbrand <david at redhat.com>
Date: Tue, 6 May 2025 09:13:05 +0000
Subject: [PATCH V8 3/9] fs: add alloc_anon_secure_inode() for allocating
secure anonymous inodes
This introduces alloc_anon_secure_inode() combining alloc_anon_inode()
with security_inode_init_security_anon(), similar to secretmem's usage.
As discussed [1], we need this for cases like secretmem and kvm_gmem
when there might be interest to have global access control via LSMs and
need proper security labeling while maintaining S_PRIVATE.
The new helper avoids duplicating the security initialization for secretmem
and kvm_gmem.
[1]: https://lore.kernel.org/linux-mm/b9e5fa41-62fd-4b3d-bb2d-24ae9d3c33da@redhat.com
Signed-off-by: David Hildenbrand <david at redhat.com>
[Shivank: add documentation]
Signed-off-by: Shivank Garg <shivankg at amd.com>
---
fs/anon_inodes.c | 46 ++++++++++++++++++++++++++++++++++++++++------
include/linux/fs.h | 1 +
2 files changed, 41 insertions(+), 6 deletions(-)
diff --git a/fs/anon_inodes.c b/fs/anon_inodes.c
index 583ac81669c2..479efcec20bc 100644
--- a/fs/anon_inodes.c
+++ b/fs/anon_inodes.c
@@ -55,17 +55,33 @@ 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)
+/**
+ * anon_inode_make_secure_inode - allocate an anonymous inode with security context
+ * @sb: [in] Superblock to allocate from
+ * @name: [in] Name of the class of the newfile (e.g., "secretmem")
+ * @context_inode:
+ * [in] Optional parent inode for security inheritance
+ * @fs_internal:
+ * [in] If true, keep S_PRIVATE set (flag indicating internal fs inodes)
+ *
+ * The function ensures proper security initialization through the LSM hook
+ * security_inode_init_security_anon().
+ *
+ * Return: Pointer to new inode on success, ERR_PTR on failure.
+ */
+static struct inode *anon_inode_make_secure_inode(struct super_block *sb,
+ 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(sb);
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 +91,23 @@ static struct inode *anon_inode_make_secure_inode(
return inode;
}
+/**
+ * alloc_anon_secure_inode - allocate a secure anonymous inode
+ * @sb: [in] Superblock to allocate the inode from
+ * @name: [in] Name of the class of the newfile (e.g., "secretmem")
+ *
+ * Specialized version of anon_inode_make_secure_inode() for filesystem use.
+ * This creates an internal-use inode, marked with S_PRIVATE (hidden from
+ * userspace).
+ *
+ * Return: A pointer to the new inode on success, ERR_PTR on failure.
+ */
+struct inode *alloc_anon_secure_inode(struct super_block *sb, const char *name)
+{
+ return anon_inode_make_secure_inode(sb, 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 +121,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;
--
2.34.1
-------------- next part --------------
From cb9f114b3097d49f99a224cd3d2483d106766521 Mon Sep 17 00:00:00 2001
From: David Hildenbrand <david at redhat.com>
Date: Tue, 6 May 2025 09:21:24 +0000
Subject: [PATCH V8 4/9] mm/secretmem: use alloc_anon_secure_inode()
Use alloc_anon_secure_inode() instead of alloc_anon_inode() +
security_inode_init_security_anon() to avoid duplicating the
anon_inode_make_secure_inode() logic.
Signed-off-by: David Hildenbrand <david at redhat.com>
Signed-off-by: Shivank Garg <shivankg at amd.com>
---
mm/secretmem.c | 9 +--------
1 file changed, 1 insertion(+), 8 deletions(-)
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);
- 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.34.1
More information about the Linux-security-module-archive
mailing list