[PATCH v3] securityfs: fix missing of d_delete() in securityfs_remove()

[PATCH v3] securityfs: fix missing of d_delete() in securityfs_remove()

[alexjlzheng]

From: Jinliang Zheng <alexjlzheng@tencent.com>

Consider the following execution flow:

  Thread 0: securityfs_create_dir("A")
  Thread 1: cd /sys/kernel/security/A           <- we hold 'A'
  Thread 0: securityfs_remove(dentry)           <- 'A' don't go away
  Thread 0: securityfs_create_dir("A")          <- Failed: File exists!

Although the LSM module will not be dynamically added or deleted after
the kernel is started, it may dynamically add or delete pseudo files
for status export or function configuration in userspace according to
different status, which we are not prohibited from doing so.

In addition, securityfs_recursive_remove() avoids this problem by calling
__d_drop() directly. As a non-recursive version, it is somewhat strange
that securityfs_remove() does not clean up the deleted dentry.

Fix this by adding d_delete() in securityfs_remove().

Fixes: b67dbf9d4c198 ("[PATCH] add securityfs for all LSMs to use")
Signed-off-by: Jinliang Zheng <alexjlzheng@tencent.com>
---
changelog:
v3: Modify the commit message to avoid readers mistakenly thinking that the LSM is being dynamically loaded
v2: https://lore.kernel.org/all/20250507111204.2585739-1-alexjlzheng@tencent.com/
v1: https://lore.kernel.org/all/20250425092548.6828-1-alexjlzheng@tencent.com/
---
 security/inode.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/security/inode.c b/security/inode.c
index da3ab44c8e57..d99baf26350a 100644
--- a/security/inode.c
+++ b/security/inode.c
@@ -306,6 +306,7 @@ void securityfs_remove(struct dentry *dentry)
 			simple_rmdir(dir, dentry);
 		else
 			simple_unlink(dir, dentry);
+		d_delete(dentry);
 		dput(dentry);
 	}
 	inode_unlock(dir);
-- 
2.49.0

Re: [PATCH v3] securityfs: fix missing of d_delete() in securityfs_remove()

[Fan Wu]

On Thu, May 8, 2025 at 7:11 AM <alexjlzheng@gmail.com> wrote:
>
> From: Jinliang Zheng <alexjlzheng@tencent.com>
>
> Consider the following execution flow:
>
>   Thread 0: securityfs_create_dir("A")
>   Thread 1: cd /sys/kernel/security/A           <- we hold 'A'
>   Thread 0: securityfs_remove(dentry)           <- 'A' don't go away
>   Thread 0: securityfs_create_dir("A")          <- Failed: File exists!
>
> Although the LSM module will not be dynamically added or deleted after
> the kernel is started, it may dynamically add or delete pseudo files
> for status export or function configuration in userspace according to
> different status, which we are not prohibited from doing so.
>
> In addition, securityfs_recursive_remove() avoids this problem by calling
> __d_drop() directly. As a non-recursive version, it is somewhat strange
> that securityfs_remove() does not clean up the deleted dentry.
>
> Fix this by adding d_delete() in securityfs_remove().
>
> Fixes: b67dbf9d4c198 ("[PATCH] add securityfs for all LSMs to use")
> Signed-off-by: Jinliang Zheng <alexjlzheng@tencent.com>
> ---
> changelog:
> v3: Modify the commit message to avoid readers mistakenly thinking that the LSM is being dynamically loaded
> v2: https://lore.kernel.org/all/20250507111204.2585739-1-alexjlzheng@tencent.com/
> v1: https://lore.kernel.org/all/20250425092548.6828-1-alexjlzheng@tencent.com/
> ---
>  security/inode.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/security/inode.c b/security/inode.c
> index da3ab44c8e57..d99baf26350a 100644
> --- a/security/inode.c
> +++ b/security/inode.c
> @@ -306,6 +306,7 @@ void securityfs_remove(struct dentry *dentry)
>                         simple_rmdir(dir, dentry);
>                 else
>                         simple_unlink(dir, dentry);
> +               d_delete(dentry);
>                 dput(dentry);
>         }
>         inode_unlock(dir);
> --
> 2.49.0
>
>

Since this could impact efi_secret_unlink(), I would suggest adding linux-efi.

-Fan

Re: [PATCH v3] securityfs: fix missing of d_delete() in securityfs_remove()

[Jinliang Zheng]

On Thu, 8 May 2025 18:55:30 -0700, Fan Wu <wufan@kernel.org> wrote:
> On Thu, May 8, 2025 at 7:11 AM <alexjlzheng@gmail.com> wrote:
> >
> > From: Jinliang Zheng <alexjlzheng@tencent.com>
> >
> > Consider the following execution flow:
> >
> >   Thread 0: securityfs_create_dir("A")
> >   Thread 1: cd /sys/kernel/security/A           <- we hold 'A'
> >   Thread 0: securityfs_remove(dentry)           <- 'A' don't go away
> >   Thread 0: securityfs_create_dir("A")          <- Failed: File exists!
> >
> > Although the LSM module will not be dynamically added or deleted after
> > the kernel is started, it may dynamically add or delete pseudo files
> > for status export or function configuration in userspace according to
> > different status, which we are not prohibited from doing so.
> >
> > In addition, securityfs_recursive_remove() avoids this problem by calling
> > __d_drop() directly. As a non-recursive version, it is somewhat strange
> > that securityfs_remove() does not clean up the deleted dentry.
> >
> > Fix this by adding d_delete() in securityfs_remove().
> >
> > Fixes: b67dbf9d4c198 ("[PATCH] add securityfs for all LSMs to use")
> > Signed-off-by: Jinliang Zheng <alexjlzheng@tencent.com>
> > ---
> > changelog:
> > v3: Modify the commit message to avoid readers mistakenly thinking that the LSM is being dynamically loaded
> > v2: https://lore.kernel.org/all/20250507111204.2585739-1-alexjlzheng@tencent.com/
> > v1: https://lore.kernel.org/all/20250425092548.6828-1-alexjlzheng@tencent.com/
> > ---
> >  security/inode.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/security/inode.c b/security/inode.c
> > index da3ab44c8e57..d99baf26350a 100644
> > --- a/security/inode.c
> > +++ b/security/inode.c
> > @@ -306,6 +306,7 @@ void securityfs_remove(struct dentry *dentry)
> >                         simple_rmdir(dir, dentry);
> >                 else
> >                         simple_unlink(dir, dentry);
> > +               d_delete(dentry);
> >                 dput(dentry);
> >         }
> >         inode_unlock(dir);
> > --
> > 2.49.0
> >
> >
> 
> Since this could impact efi_secret_unlink(), I would suggest adding linux-efi.

Thank you for your reply. :)

Did you mean cc to linux-efi?

thanks,
Jinliang Zheng.

> 
> -Fan

Re: [PATCH v3] securityfs: fix missing of d_delete() in securityfs_remove()

[Al Viro]

On Thu, May 08, 2025 at 10:04:39PM +0800, alexjlzheng@gmail.com wrote:

> In addition, securityfs_recursive_remove() avoids this problem by calling
> __d_drop() directly. As a non-recursive version, it is somewhat strange
> that securityfs_remove() does not clean up the deleted dentry.
> 
> Fix this by adding d_delete() in securityfs_remove().

This is not a fix.  First and foremost, securityfs_recursive_remove()
does *not* just call __d_drop() - it calls simple_recursive_removal(),
which takes care to evict anything possibly mounted on those suckers.

Your variant trivially turns into a mount leak - just bind anything
on that thing and trigger removal.

<a bit of a rant follows; if it offends somebody, feel free to report
to CoC committee>

What's more, securityfs object creation is... special.  It does, for
some odd reason, leave you dentry with refcount *two*.  For no reason
whatsoever, as far as I can tell.

securityfs_remove() matches that; securityfs_recursive_remove(),
as far as I can tell, should simply leak them.  That's from RTFS
alone, but I don't see how it could possibly *not* happen -
securityfs_create_file() is a call of securityfs_create_dentry(),
which
	* calls lookup_one_len(), getting a negative dentry with
refcount 1.
	* verifies it's negative
	* gets a new inode
	* does d_instantiate(), attaching it to dentry.
	* does dget(), for some unspeakable reason.  Refcount is 2 now.
	* returns that dentry to caller.

policyfs stuff calls securityfs_create_dir() (which is a wrapper for
securityfs_create_file(), with nothing extra done to refcounts),
then populates it with a bunch of files, all with the same refcount
weirdness.

Result: directory dentry with refcount 2 + number of children and
a bunch of children, each with refcount 2.

Now, securityfs_recursive_remove() calls simple_recursive_removal(),
which will strip _one_ off the refcount of each dentry in that tree.
Yes, they are all unhashed and any stuff mounted on them is kicked
out, but you have a massive dentry leak now - all of those dentries
have refcount at least 1.

I'm not blaming securityfs_recursive_remove() authors - it *should*
have worked; their only fault is that they hadn't guessed that
object creation on securityfs happens to be that strange.

Another special snowflake is efi_secret_unlink() - it calls
securityfs_remove(), which is needed instead of simple_unlink()
since
	* that double refcount needs to be dropped
	* having internal mount pinned is something that needs
to be undone, innit?

Of course, it runs afoul of the parent being locked, but nevermind that -
it just unlocks and relocks it, 'cuz what can go wrong?  That - instead
of discussing that with VFS and filesystem folks.

As for "what can go wrong"...  Consider what happens if another process
calls unlink() on the same file, just before the first one drops the
lock on parent.  Parent found, process 2 blocked on the lock.  Process 1
unlocks that lock and loses CPU.  Process 2 runs and tries to lock the
victim; blocks since process 1 is still holding it locked.  Process 1,
in securityfs_remove(): blocks trying to lock the parent.  AB-BA deadlock.

Oh, well...

Anyway, the reasons for securityfs_remove() use there are real deficiencies
of securityfs.  Weird shit with refcounts is one thing; internal mount
pinning is a bit more subtle, but it's also solvable.

The thing is, objects on securityfs never change parents.  So you only
need to pin for subdirectories of root - everything deeper will be
automatically fine.  And that kills the second reason for those games.
With that dealt with, efi_secret_unlink() can simply call simple_unlink()
instead of those games.

After that securityfs_remove() can become an alias for
securityfs_recursive_remove() (or the other way round, preferably).

BTW,
        d_inode(dent)->i_op = &efi_secret_dir_inode_operations;
in the same drivers/virt/coco is also nasty - you don't change the method
table on an object that is already exposed in shared data structures.
Basic multithreaded programming safety rules...  Yes, _that_ probably runs
too early in the boot for anything to hit it, so it's not a security hole,
but the same "what if somebody copies that code and gets screwed" applies
there...  If anything, that points to the need of securityfs_create_dir()
variant that would override ->i_op, which should've been discussed back
when the thing had been merged.

</rant>

I have fixes for some of that crap done on top of tree-in-dcache series;
give me an hour or two and I'll separate those and rebase to mainline...

Re: [PATCH v3] securityfs: fix missing of d_delete() in securityfs_remove()

[Al Viro]

On Fri, May 09, 2025 at 04:23:26AM +0100, Al Viro wrote:

> I have fixes for some of that crap done on top of tree-in-dcache series;
> give me an hour or two and I'll separate those and rebase to mainline...

Completely untested:
git://git.kernel.org:/pub/scm/linux/kernel/git/viro/vfs.git #untested.securityfs

on top of v6.15-rc5.  And I'm serious about the "untested" part - it builds
with allmodconfig, but that's all I've checked.  So treat that as an outline
of what could be done, but don't use as-is without serious testing.

Re: [PATCH v3] securityfs: fix missing of d_delete() in securityfs_remove()

[Al Viro]

On Fri, May 09, 2025 at 05:37:12AM +0100, Al Viro wrote:
> On Fri, May 09, 2025 at 04:23:26AM +0100, Al Viro wrote:
> 
> > I have fixes for some of that crap done on top of tree-in-dcache series;
> > give me an hour or two and I'll separate those and rebase to mainline...
> 
> Completely untested:
> git://git.kernel.org:/pub/scm/linux/kernel/git/viro/vfs.git #untested.securityfs
> 
> on top of v6.15-rc5.  And I'm serious about the "untested" part - it builds
> with allmodconfig, but that's all I've checked.  So treat that as an outline
> of what could be done, but don't use as-is without serious testing.

PS: I'm really, really serious - do not use without a serious review; this
is a rebase of a branch last touched back in March and it was a part of
long tail, with pretty much zero testing even back then.

Patches are simple enough to have a chance to be somewhere in the vicinity
of being correct, but that's all I can promise.

Re: [PATCH v3] securityfs: fix missing of d_delete() in securityfs_remove()

[Paul Moore]

On Fri, May 9, 2025 at 12:46 AM Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Fri, May 09, 2025 at 05:37:12AM +0100, Al Viro wrote:
> > On Fri, May 09, 2025 at 04:23:26AM +0100, Al Viro wrote:
> >
> > > I have fixes for some of that crap done on top of tree-in-dcache series;
> > > give me an hour or two and I'll separate those and rebase to mainline...
> >
> > Completely untested:
> > git://git.kernel.org:/pub/scm/linux/kernel/git/viro/vfs.git #untested.securityfs
> >
> > on top of v6.15-rc5.  And I'm serious about the "untested" part - it builds
> > with allmodconfig, but that's all I've checked.  So treat that as an outline
> > of what could be done, but don't use as-is without serious testing.
>
> PS: I'm really, really serious - do not use without a serious review; this
> is a rebase of a branch last touched back in March and it was a part of
> long tail, with pretty much zero testing even back then.
>
> Patches are simple enough to have a chance to be somewhere in the vicinity
> of being correct, but that's all I can promise.

Fair enough, although unfortunately I don't think anyone has anything
close to a securityfs test suite so I suspect this may languish on the
lists for a bit unless someone has the cycles to pick it up and
properly test it.

I haven't compared the patches you posted on-list with the stuff in
the tree above, but based on the timestamps I'm guessing the on-list
patches are simply the ones from the tree above?

-- 
paul-moore.com

Re: [PATCH v3] securityfs: fix missing of d_delete() in securityfs_remove()

[Al Viro]

On Mon, May 12, 2025 at 05:19:39PM -0400, Paul Moore wrote:
> On Fri, May 9, 2025 at 12:46 AM Al Viro <viro@zeniv.linux.org.uk> wrote:
> > On Fri, May 09, 2025 at 05:37:12AM +0100, Al Viro wrote:
> > > On Fri, May 09, 2025 at 04:23:26AM +0100, Al Viro wrote:
> > >
> > > > I have fixes for some of that crap done on top of tree-in-dcache series;
> > > > give me an hour or two and I'll separate those and rebase to mainline...
> > >
> > > Completely untested:
> > > git://git.kernel.org:/pub/scm/linux/kernel/git/viro/vfs.git #untested.securityfs
> > >
> > > on top of v6.15-rc5.  And I'm serious about the "untested" part - it builds
> > > with allmodconfig, but that's all I've checked.  So treat that as an outline
> > > of what could be done, but don't use as-is without serious testing.
> >
> > PS: I'm really, really serious - do not use without a serious review; this
> > is a rebase of a branch last touched back in March and it was a part of
> > long tail, with pretty much zero testing even back then.
> >
> > Patches are simple enough to have a chance to be somewhere in the vicinity
> > of being correct, but that's all I can promise.
> 
> Fair enough, although unfortunately I don't think anyone has anything
> close to a securityfs test suite so I suspect this may languish on the
> lists for a bit unless someone has the cycles to pick it up and
> properly test it.
> 
> I haven't compared the patches you posted on-list with the stuff in
> the tree above, but based on the timestamps I'm guessing the on-list
> patches are simply the ones from the tree above?

git format-patch output for that branch...

Re: [PATCH v3] securityfs: fix missing of d_delete() in securityfs_remove()

[Fan Wu]

On Mon, May 12, 2025 at 2:19 PM Paul Moore <paul@paul-moore.com> wrote:
>
> On Fri, May 9, 2025 at 12:46 AM Al Viro <viro@zeniv.linux.org.uk> wrote:
> > On Fri, May 09, 2025 at 05:37:12AM +0100, Al Viro wrote:
> > > On Fri, May 09, 2025 at 04:23:26AM +0100, Al Viro wrote:
> > >
> > > > I have fixes for some of that crap done on top of tree-in-dcache series;
> > > > give me an hour or two and I'll separate those and rebase to mainline...
> > >
> > > Completely untested:
> > > git://git.kernel.org:/pub/scm/linux/kernel/git/viro/vfs.git #untested.securityfs
> > >
> > > on top of v6.15-rc5.  And I'm serious about the "untested" part - it builds
> > > with allmodconfig, but that's all I've checked.  So treat that as an outline
> > > of what could be done, but don't use as-is without serious testing.
> >
> > PS: I'm really, really serious - do not use without a serious review; this
> > is a rebase of a branch last touched back in March and it was a part of
> > long tail, with pretty much zero testing even back then.
> >
> > Patches are simple enough to have a chance to be somewhere in the vicinity
> > of being correct, but that's all I can promise.
>
> Fair enough, although unfortunately I don't think anyone has anything
> close to a securityfs test suite so I suspect this may languish on the
> lists for a bit unless someone has the cycles to pick it up and
> properly test it.
>

Since it's me who added the recursive remove, I'm interested in
helping get the fix tested and verified. However, I might not have
enough cycles in the near future. Happy to let someone else test it if
they have bandwidth.

-Fan

[PATCH 1/8] securityfs: don't pin dentries twice, once is enough...

[Al Viro]

From 5c463d47c814e16adb6e997a05ca5625df41152d Mon Sep 17 00:00:00 2001
From: Al Viro <viro@zeniv.linux.org.uk>
Date: Thu, 8 May 2025 23:38:01 -0400
Subject: [PATCH 1/8] securityfs: don't pin dentries twice, once is enough...

incidentally, securityfs_recursive_remove() is broken without that -
it leaks dentries, since simple_recursive_removal() does not expect
anything of that sort.  It could be worked around by dput() in
remove_one() callback, but it's easier to just drop that double-get
stuff.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 security/inode.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/security/inode.c b/security/inode.c
index da3ab44c8e57..58cc60c50498 100644
--- a/security/inode.c
+++ b/security/inode.c
@@ -159,7 +159,6 @@ static struct dentry *securityfs_create_dentry(const char *name, umode_t mode,
 		inode->i_fop = fops;
 	}
 	d_instantiate(dentry, inode);
-	dget(dentry);
 	inode_unlock(dir);
 	return dentry;
 
@@ -306,7 +305,6 @@ void securityfs_remove(struct dentry *dentry)
 			simple_rmdir(dir, dentry);
 		else
 			simple_unlink(dir, dentry);
-		dput(dentry);
 	}
 	inode_unlock(dir);
 	simple_release_fs(&mount, &mount_count);
-- 
2.39.5

Re: [PATCH 1/8] securityfs: don't pin dentries twice, once is enough...

[Paul Moore]

On Fri, May 9, 2025 at 12:38 AM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> From 5c463d47c814e16adb6e997a05ca5625df41152d Mon Sep 17 00:00:00 2001
> From: Al Viro <viro@zeniv.linux.org.uk>
> Date: Thu, 8 May 2025 23:38:01 -0400
> Subject: [PATCH 1/8] securityfs: don't pin dentries twice, once is enough...
>
> incidentally, securityfs_recursive_remove() is broken without that -
> it leaks dentries, since simple_recursive_removal() does not expect
> anything of that sort.  It could be worked around by dput() in
> remove_one() callback, but it's easier to just drop that double-get
> stuff.
>
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
>  security/inode.c | 2 --
>  1 file changed, 2 deletions(-)

Replying with a lore link to the associated discussion and warning so
we have some record of it in the LSM patchwork.

https://lore.kernel.org/linux-security-module/20250509044613.GT2023217@ZenIV/

-- 
paul-moore.com

[PATCH 2/8] securityfs: pin filesystem only for objects directly in root

[Al Viro]

From 23947dd21a91b95208863c74d4dbc67d0027f43a Mon Sep 17 00:00:00 2001
From: Al Viro <viro@zeniv.linux.org.uk>
Date: Tue, 14 May 2024 08:46:00 -0600
Subject: [PATCH 2/8] securityfs: pin filesystem only for objects directly in
 root

Nothing on securityfs ever changes parents, so we don't need
to pin the internal mount if it's already pinned for parent.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 security/inode.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/security/inode.c b/security/inode.c
index 58cc60c50498..19ab99feeee3 100644
--- a/security/inode.c
+++ b/security/inode.c
@@ -112,18 +112,20 @@ static struct dentry *securityfs_create_dentry(const char *name, umode_t mode,
 	struct dentry *dentry;
 	struct inode *dir, *inode;
 	int error;
+	bool pinned = false;
 
 	if (!(mode & S_IFMT))
 		mode = (mode & S_IALLUGO) | S_IFREG;
 
 	pr_debug("securityfs: creating file '%s'\n",name);
 
-	error = simple_pin_fs(&fs_type, &mount, &mount_count);
-	if (error)
-		return ERR_PTR(error);
-
-	if (!parent)
+	if (!parent) {
+		error = simple_pin_fs(&fs_type, &mount, &mount_count);
+		if (error)
+			return ERR_PTR(error);
+		pinned = true;
 		parent = mount->mnt_root;
+	}
 
 	dir = d_inode(parent);
 
@@ -167,7 +169,8 @@ static struct dentry *securityfs_create_dentry(const char *name, umode_t mode,
 	dentry = ERR_PTR(error);
 out:
 	inode_unlock(dir);
-	simple_release_fs(&mount, &mount_count);
+	if (pinned)
+		simple_release_fs(&mount, &mount_count);
 	return dentry;
 }
 
@@ -307,13 +310,15 @@ void securityfs_remove(struct dentry *dentry)
 			simple_unlink(dir, dentry);
 	}
 	inode_unlock(dir);
-	simple_release_fs(&mount, &mount_count);
+	if (dir != dir->i_sb->s_root->d_inode)
+		simple_release_fs(&mount, &mount_count);
 }
 EXPORT_SYMBOL_GPL(securityfs_remove);
 
 static void remove_one(struct dentry *victim)
 {
-	simple_release_fs(&mount, &mount_count);
+	if (victim->d_parent != victim->d_sb->s_root)
+		simple_release_fs(&mount, &mount_count);
 }
 
 /**
-- 
2.39.5

[PATCH 3/8] fix locking in efi_secret_unlink()

[Al Viro]

From d332a8fcb3c1219f5e0ae1961a8ff4a4e3cd3bcc Mon Sep 17 00:00:00 2001
From: Al Viro <viro@zeniv.linux.org.uk>
Date: Tue, 14 May 2024 08:48:58 -0600
Subject: [PATCH 3/8] fix locking in efi_secret_unlink()

now we can just have it call simple_unlink() and be done with that

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 drivers/virt/coco/efi_secret/efi_secret.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/virt/coco/efi_secret/efi_secret.c b/drivers/virt/coco/efi_secret/efi_secret.c
index 1864f9f80617..f2da4819ec3b 100644
--- a/drivers/virt/coco/efi_secret/efi_secret.c
+++ b/drivers/virt/coco/efi_secret/efi_secret.c
@@ -136,15 +136,7 @@ static int efi_secret_unlink(struct inode *dir, struct dentry *dentry)
 		if (s->fs_files[i] == dentry)
 			s->fs_files[i] = NULL;
 
-	/*
-	 * securityfs_remove tries to lock the directory's inode, but we reach
-	 * the unlink callback when it's already locked
-	 */
-	inode_unlock(dir);
-	securityfs_remove(dentry);
-	inode_lock(dir);
-
-	return 0;
+	return simple_unlink(inode, dentry);
 }
 
 static const struct inode_operations efi_secret_dir_inode_operations = {
-- 
2.39.5

[PATCH 4/8] make securityfs_remove() remove the entire subtree

[Al Viro]

From 7b5a69f41094b9a4aff491d00e9b8515fa248cef Mon Sep 17 00:00:00 2001
From: Al Viro <viro@zeniv.linux.org.uk>
Date: Mon, 13 May 2024 23:36:53 -0600
Subject: [PATCH 4/8] make securityfs_remove() remove the entire subtree

... and fix the mount leak when anything's mounted there.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 security/inode.c | 47 +++++++++++++----------------------------------
 1 file changed, 13 insertions(+), 34 deletions(-)

diff --git a/security/inode.c b/security/inode.c
index 19ab99feeee3..7604040d5c45 100644
--- a/security/inode.c
+++ b/security/inode.c
@@ -281,6 +281,12 @@ struct dentry *securityfs_create_symlink(const char *name,
 }
 EXPORT_SYMBOL_GPL(securityfs_create_symlink);
 
+static void remove_one(struct dentry *victim)
+{
+	if (victim->d_parent != victim->d_sb->s_root)
+		simple_release_fs(&mount, &mount_count);
+}
+
 /**
  * securityfs_remove - removes a file or directory from the securityfs filesystem
  *
@@ -293,51 +299,24 @@ EXPORT_SYMBOL_GPL(securityfs_create_symlink);
  * This function is required to be called in order for the file to be
  * removed. No automatic cleanup of files will happen when a module is
  * removed; you are responsible here.
+ *
+ * AV: when applied to directory it will take all children out; no need to call
+ * it for descendents if ancestor is getting killed.
  */
 void securityfs_remove(struct dentry *dentry)
 {
-	struct inode *dir;
-
 	if (IS_ERR_OR_NULL(dentry))
 		return;
 
-	dir = d_inode(dentry->d_parent);
-	inode_lock(dir);
-	if (simple_positive(dentry)) {
-		if (d_is_dir(dentry))
-			simple_rmdir(dir, dentry);
-		else
-			simple_unlink(dir, dentry);
-	}
-	inode_unlock(dir);
-	if (dir != dir->i_sb->s_root->d_inode)
-		simple_release_fs(&mount, &mount_count);
+	simple_pin_fs(&fs_type, &mount, &mount_count);
+	simple_recursive_removal(dentry, remove_one);
+	simple_release_fs(&mount, &mount_count);
 }
 EXPORT_SYMBOL_GPL(securityfs_remove);
 
-static void remove_one(struct dentry *victim)
-{
-	if (victim->d_parent != victim->d_sb->s_root)
-		simple_release_fs(&mount, &mount_count);
-}
-
-/**
- * securityfs_recursive_remove - recursively removes a file or directory
- *
- * @dentry: a pointer to a the dentry of the file or directory to be removed.
- *
- * This function recursively removes a file or directory in securityfs that was
- * previously created with a call to another securityfs function (like
- * securityfs_create_file() or variants thereof.)
- */
 void securityfs_recursive_remove(struct dentry *dentry)
 {
-	if (IS_ERR_OR_NULL(dentry))
-		return;
-
-	simple_pin_fs(&fs_type, &mount, &mount_count);
-	simple_recursive_removal(dentry, remove_one);
-	simple_release_fs(&mount, &mount_count);
+	securityfs_remove(dentry);
 }
 EXPORT_SYMBOL_GPL(securityfs_recursive_remove);
 
-- 
2.39.5

[PATCH 5/8] efi_secret: clean securityfs use up

[Al Viro]

From 3d85b8d99ead2537b8be972631a9c88e6814f18b Mon Sep 17 00:00:00 2001
From: Al Viro <viro@zeniv.linux.org.uk>
Date: Tue, 14 May 2024 08:53:07 -0600
Subject: [PATCH 5/8] efi_secret: clean securityfs use up

securityfs_remove() does take care of entire subtree now; no need
to mess with them individually.

NB: ->i_op replacement in there is still buggy.  One shouldn't
ever modify ->i_op of live accessible inode.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 drivers/virt/coco/efi_secret/efi_secret.c | 37 +++++------------------
 1 file changed, 8 insertions(+), 29 deletions(-)

diff --git a/drivers/virt/coco/efi_secret/efi_secret.c b/drivers/virt/coco/efi_secret/efi_secret.c
index f2da4819ec3b..5946c5abeae8 100644
--- a/drivers/virt/coco/efi_secret/efi_secret.c
+++ b/drivers/virt/coco/efi_secret/efi_secret.c
@@ -31,8 +31,6 @@
 
 struct efi_secret {
 	struct dentry *secrets_dir;
-	struct dentry *fs_dir;
-	struct dentry *fs_files[EFI_SECRET_NUM_FILES];
 	void __iomem *secret_data;
 	u64 secret_data_len;
 };
@@ -119,10 +117,8 @@ static void wipe_memory(void *addr, size_t size)
 
 static int efi_secret_unlink(struct inode *dir, struct dentry *dentry)
 {
-	struct efi_secret *s = efi_secret_get();
 	struct inode *inode = d_inode(dentry);
 	struct secret_entry *e = (struct secret_entry *)inode->i_private;
-	int i;
 
 	if (e) {
 		/* Zero out the secret data */
@@ -132,10 +128,6 @@ static int efi_secret_unlink(struct inode *dir, struct dentry *dentry)
 
 	inode->i_private = NULL;
 
-	for (i = 0; i < EFI_SECRET_NUM_FILES; i++)
-		if (s->fs_files[i] == dentry)
-			s->fs_files[i] = NULL;
-
 	return simple_unlink(inode, dentry);
 }
 
@@ -186,15 +178,6 @@ static int efi_secret_map_area(struct platform_device *dev)
 static void efi_secret_securityfs_teardown(struct platform_device *dev)
 {
 	struct efi_secret *s = efi_secret_get();
-	int i;
-
-	for (i = (EFI_SECRET_NUM_FILES - 1); i >= 0; i--) {
-		securityfs_remove(s->fs_files[i]);
-		s->fs_files[i] = NULL;
-	}
-
-	securityfs_remove(s->fs_dir);
-	s->fs_dir = NULL;
 
 	securityfs_remove(s->secrets_dir);
 	s->secrets_dir = NULL;
@@ -209,7 +192,7 @@ static int efi_secret_securityfs_setup(struct platform_device *dev)
 	unsigned char *ptr;
 	struct secret_header *h;
 	struct secret_entry *e;
-	struct dentry *dent;
+	struct dentry *dent, *dir;
 	char guid_str[EFI_VARIABLE_GUID_LEN + 1];
 
 	ptr = (void __force *)s->secret_data;
@@ -232,8 +215,6 @@ static int efi_secret_securityfs_setup(struct platform_device *dev)
 	}
 
 	s->secrets_dir = NULL;
-	s->fs_dir = NULL;
-	memset(s->fs_files, 0, sizeof(s->fs_files));
 
 	dent = securityfs_create_dir("secrets", NULL);
 	if (IS_ERR(dent)) {
@@ -243,14 +224,13 @@ static int efi_secret_securityfs_setup(struct platform_device *dev)
 	}
 	s->secrets_dir = dent;
 
-	dent = securityfs_create_dir("coco", s->secrets_dir);
-	if (IS_ERR(dent)) {
+	dir = securityfs_create_dir("coco", s->secrets_dir);
+	if (IS_ERR(dir)) {
 		dev_err(&dev->dev, "Error creating coco securityfs directory entry err=%ld\n",
-			PTR_ERR(dent));
-		return PTR_ERR(dent);
+			PTR_ERR(dir));
+		return PTR_ERR(dir);
 	}
-	d_inode(dent)->i_op = &efi_secret_dir_inode_operations;
-	s->fs_dir = dent;
+	d_inode(dir)->i_op = &efi_secret_dir_inode_operations;
 
 	bytes_left = h->len - sizeof(*h);
 	ptr += sizeof(*h);
@@ -266,15 +246,14 @@ static int efi_secret_securityfs_setup(struct platform_device *dev)
 		if (efi_guidcmp(e->guid, NULL_GUID)) {
 			efi_guid_to_str(&e->guid, guid_str);
 
-			dent = securityfs_create_file(guid_str, 0440, s->fs_dir, (void *)e,
+			dent = securityfs_create_file(guid_str, 0440, dir, (void *)e,
 						      &efi_secret_bin_file_fops);
 			if (IS_ERR(dent)) {
 				dev_err(&dev->dev, "Error creating efi_secret securityfs entry\n");
 				ret = PTR_ERR(dent);
 				goto err_cleanup;
 			}
-
-			s->fs_files[i++] = dent;
+			i++;
 		}
 		ptr += e->len;
 		bytes_left -= e->len;
-- 
2.39.5

[PATCH 6/8] ima_fs: don't bother with removal of files in directory we'll be removing

[Al Viro]

From 08433f2507554980bc891d8b17c1968c81cb144b Mon Sep 17 00:00:00 2001
From: Al Viro <viro@zeniv.linux.org.uk>
Date: Mon, 13 May 2024 23:41:51 -0600
Subject: [PATCH 6/8] ima_fs: don't bother with removal of files in directory
 we'll be removing

removal of parent takes all children out

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 security/integrity/ima/ima_fs.c | 54 +++++++++++----------------------
 1 file changed, 17 insertions(+), 37 deletions(-)

diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
index e4a79a9b2d58..8e2c132ce640 100644
--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c
@@ -396,11 +396,6 @@ static ssize_t ima_write_policy(struct file *file, const char __user *buf,
 
 static struct dentry *ima_dir;
 static struct dentry *ima_symlink;
-static struct dentry *binary_runtime_measurements;
-static struct dentry *ascii_runtime_measurements;
-static struct dentry *runtime_measurements_count;
-static struct dentry *violations;
-static struct dentry *ima_policy;
 
 enum ima_fs_flags {
 	IMA_FS_BUSY,
@@ -419,14 +414,7 @@ static const struct seq_operations ima_policy_seqops = {
 
 static void __init remove_securityfs_measurement_lists(struct dentry **lists)
 {
-	int i;
-
-	if (lists) {
-		for (i = 0; i < securityfs_measurement_list_count; i++)
-			securityfs_remove(lists[i]);
-
-		kfree(lists);
-	}
+	kfree(lists);
 }
 
 static int __init create_securityfs_measurement_lists(void)
@@ -553,6 +541,7 @@ static const struct file_operations ima_measure_policy_ops = {
 
 int __init ima_fs_init(void)
 {
+	struct dentry *dentry;
 	int ret;
 
 	ascii_securityfs_measurement_lists = NULL;
@@ -573,54 +562,45 @@ int __init ima_fs_init(void)
 	if (ret != 0)
 		goto out;
 
-	binary_runtime_measurements =
-	    securityfs_create_symlink("binary_runtime_measurements", ima_dir,
+	dentry = securityfs_create_symlink("binary_runtime_measurements", ima_dir,
 				      "binary_runtime_measurements_sha1", NULL);
-	if (IS_ERR(binary_runtime_measurements)) {
-		ret = PTR_ERR(binary_runtime_measurements);
+	if (IS_ERR(dentry)) {
+		ret = PTR_ERR(dentry);
 		goto out;
 	}
 
-	ascii_runtime_measurements =
-	    securityfs_create_symlink("ascii_runtime_measurements", ima_dir,
+	dentry = securityfs_create_symlink("ascii_runtime_measurements", ima_dir,
 				      "ascii_runtime_measurements_sha1", NULL);
-	if (IS_ERR(ascii_runtime_measurements)) {
-		ret = PTR_ERR(ascii_runtime_measurements);
+	if (IS_ERR(dentry)) {
+		ret = PTR_ERR(dentry);
 		goto out;
 	}
 
-	runtime_measurements_count =
-	    securityfs_create_file("runtime_measurements_count",
+	dentry = securityfs_create_file("runtime_measurements_count",
 				   S_IRUSR | S_IRGRP, ima_dir, NULL,
 				   &ima_measurements_count_ops);
-	if (IS_ERR(runtime_measurements_count)) {
-		ret = PTR_ERR(runtime_measurements_count);
+	if (IS_ERR(dentry)) {
+		ret = PTR_ERR(dentry);
 		goto out;
 	}
 
-	violations =
-	    securityfs_create_file("violations", S_IRUSR | S_IRGRP,
+	dentry = securityfs_create_file("violations", S_IRUSR | S_IRGRP,
 				   ima_dir, NULL, &ima_htable_violations_ops);
-	if (IS_ERR(violations)) {
-		ret = PTR_ERR(violations);
+	if (IS_ERR(dentry)) {
+		ret = PTR_ERR(dentry);
 		goto out;
 	}
 
-	ima_policy = securityfs_create_file("policy", POLICY_FILE_FLAGS,
+	dentry = securityfs_create_file("policy", POLICY_FILE_FLAGS,
 					    ima_dir, NULL,
 					    &ima_measure_policy_ops);
-	if (IS_ERR(ima_policy)) {
-		ret = PTR_ERR(ima_policy);
+	if (IS_ERR(dentry)) {
+		ret = PTR_ERR(dentry);
 		goto out;
 	}
 
 	return 0;
 out:
-	securityfs_remove(ima_policy);
-	securityfs_remove(violations);
-	securityfs_remove(runtime_measurements_count);
-	securityfs_remove(ascii_runtime_measurements);
-	securityfs_remove(binary_runtime_measurements);
 	remove_securityfs_measurement_lists(ascii_securityfs_measurement_lists);
 	remove_securityfs_measurement_lists(binary_securityfs_measurement_lists);
 	securityfs_measurement_list_count = 0;
-- 
2.39.5

[PATCH 7/8] ima_fs: get rid of lookup-by-dentry stuff

[Al Viro]

From 35fa1b12f16a61b59886d5aae7e45af9d324a6f1 Mon Sep 17 00:00:00 2001
From: Al Viro <viro@zeniv.linux.org.uk>
Date: Mon, 10 Mar 2025 12:30:20 -0400
Subject: [PATCH 7/8] ima_fs: get rid of lookup-by-dentry stuff

lookup_template_data_hash_algo() machinery is used to locate the
matching ima_algo_array[] element at read time; securityfs
allows to stash that into inode->i_private at object creation
time, so there's no need to bother

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 security/integrity/ima/ima_fs.c | 82 +++++++--------------------------
 1 file changed, 16 insertions(+), 66 deletions(-)

diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
index 8e2c132ce640..07efd71b6310 100644
--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c
@@ -116,28 +116,6 @@ void ima_putc(struct seq_file *m, void *data, int datalen)
 		seq_putc(m, *(char *)data++);
 }
 
-static struct dentry **ascii_securityfs_measurement_lists __ro_after_init;
-static struct dentry **binary_securityfs_measurement_lists __ro_after_init;
-static int securityfs_measurement_list_count __ro_after_init;
-
-static void lookup_template_data_hash_algo(int *algo_idx, enum hash_algo *algo,
-					   struct seq_file *m,
-					   struct dentry **lists)
-{
-	struct dentry *dentry;
-	int i;
-
-	dentry = file_dentry(m->file);
-
-	for (i = 0; i < securityfs_measurement_list_count; i++) {
-		if (dentry == lists[i]) {
-			*algo_idx = i;
-			*algo = ima_algo_array[i].algo;
-			break;
-		}
-	}
-}
-
 /* print format:
  *       32bit-le=pcr#
  *       char[n]=template digest
@@ -160,9 +138,10 @@ int ima_measurements_show(struct seq_file *m, void *v)
 	algo_idx = ima_sha1_idx;
 	algo = HASH_ALGO_SHA1;
 
-	if (m->file != NULL)
-		lookup_template_data_hash_algo(&algo_idx, &algo, m,
-					       binary_securityfs_measurement_lists);
+	if (m->file != NULL) {
+		algo_idx = (unsigned long)file_inode(m->file)->i_private;
+		algo = ima_algo_array[algo_idx].algo;
+	}
 
 	/* get entry */
 	e = qe->entry;
@@ -256,9 +235,10 @@ static int ima_ascii_measurements_show(struct seq_file *m, void *v)
 	algo_idx = ima_sha1_idx;
 	algo = HASH_ALGO_SHA1;
 
-	if (m->file != NULL)
-		lookup_template_data_hash_algo(&algo_idx, &algo, m,
-					       ascii_securityfs_measurement_lists);
+	if (m->file != NULL) {
+		algo_idx = (unsigned long)file_inode(m->file)->i_private;
+		algo = ima_algo_array[algo_idx].algo;
+	}
 
 	/* get entry */
 	e = qe->entry;
@@ -412,57 +392,33 @@ static const struct seq_operations ima_policy_seqops = {
 };
 #endif
 
-static void __init remove_securityfs_measurement_lists(struct dentry **lists)
-{
-	kfree(lists);
-}
-
 static int __init create_securityfs_measurement_lists(void)
 {
-	char file_name[NAME_MAX + 1];
-	struct dentry *dentry;
-	u16 algo;
-	int i;
-
-	securityfs_measurement_list_count = NR_BANKS(ima_tpm_chip);
+	int count = NR_BANKS(ima_tpm_chip);
 
 	if (ima_sha1_idx >= NR_BANKS(ima_tpm_chip))
-		securityfs_measurement_list_count++;
+		count++;
 
-	ascii_securityfs_measurement_lists =
-	    kcalloc(securityfs_measurement_list_count, sizeof(struct dentry *),
-		    GFP_KERNEL);
-	if (!ascii_securityfs_measurement_lists)
-		return -ENOMEM;
-
-	binary_securityfs_measurement_lists =
-	    kcalloc(securityfs_measurement_list_count, sizeof(struct dentry *),
-		    GFP_KERNEL);
-	if (!binary_securityfs_measurement_lists)
-		return -ENOMEM;
-
-	for (i = 0; i < securityfs_measurement_list_count; i++) {
-		algo = ima_algo_array[i].algo;
+	for (int i = 0; i < count; i++) {
+		u16 algo = ima_algo_array[i].algo;
+		char file_name[NAME_MAX + 1];
+		struct dentry *dentry;
 
 		sprintf(file_name, "ascii_runtime_measurements_%s",
 			hash_algo_name[algo]);
 		dentry = securityfs_create_file(file_name, S_IRUSR | S_IRGRP,
-						ima_dir, NULL,
+						ima_dir, (void *)(uintptr_t)i,
 						&ima_ascii_measurements_ops);
 		if (IS_ERR(dentry))
 			return PTR_ERR(dentry);
 
-		ascii_securityfs_measurement_lists[i] = dentry;
-
 		sprintf(file_name, "binary_runtime_measurements_%s",
 			hash_algo_name[algo]);
 		dentry = securityfs_create_file(file_name, S_IRUSR | S_IRGRP,
-						ima_dir, NULL,
+						ima_dir, (void *)(uintptr_t)i,
 						&ima_measurements_ops);
 		if (IS_ERR(dentry))
 			return PTR_ERR(dentry);
-
-		binary_securityfs_measurement_lists[i] = dentry;
 	}
 
 	return 0;
@@ -544,9 +500,6 @@ int __init ima_fs_init(void)
 	struct dentry *dentry;
 	int ret;
 
-	ascii_securityfs_measurement_lists = NULL;
-	binary_securityfs_measurement_lists = NULL;
-
 	ima_dir = securityfs_create_dir("ima", integrity_dir);
 	if (IS_ERR(ima_dir))
 		return PTR_ERR(ima_dir);
@@ -601,9 +554,6 @@ int __init ima_fs_init(void)
 
 	return 0;
 out:
-	remove_securityfs_measurement_lists(ascii_securityfs_measurement_lists);
-	remove_securityfs_measurement_lists(binary_securityfs_measurement_lists);
-	securityfs_measurement_list_count = 0;
 	securityfs_remove(ima_symlink);
 	securityfs_remove(ima_dir);
 
-- 
2.39.5

[PATCH 8/8] evm_secfs: clear securityfs interactions

[Al Viro]

From b0b8e25f92ec20e266859de5f823b4e39b8e2f9d Mon Sep 17 00:00:00 2001
From: Al Viro <viro@zeniv.linux.org.uk>
Date: Mon, 13 May 2024 23:44:12 -0600
Subject: [PATCH 8/8] evm_secfs: clear securityfs interactions

1) creation never returns NULL; error is reported as ERR_PTR()
2) no need to remove file before removing its parent

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 security/integrity/evm/evm_secfs.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/security/integrity/evm/evm_secfs.c b/security/integrity/evm/evm_secfs.c
index 9b907c2fee60..b0d2aad27850 100644
--- a/security/integrity/evm/evm_secfs.c
+++ b/security/integrity/evm/evm_secfs.c
@@ -17,7 +17,6 @@
 #include "evm.h"
 
 static struct dentry *evm_dir;
-static struct dentry *evm_init_tpm;
 static struct dentry *evm_symlink;
 
 #ifdef CONFIG_EVM_ADD_XATTRS
@@ -286,7 +285,7 @@ static int evm_init_xattrs(void)
 {
 	evm_xattrs = securityfs_create_file("evm_xattrs", 0660, evm_dir, NULL,
 					    &evm_xattr_ops);
-	if (!evm_xattrs || IS_ERR(evm_xattrs))
+	if (IS_ERR(evm_xattrs))
 		return -EFAULT;
 
 	return 0;
@@ -301,21 +300,22 @@ static int evm_init_xattrs(void)
 int __init evm_init_secfs(void)
 {
 	int error = 0;
+	struct dentry *dentry;
 
 	evm_dir = securityfs_create_dir("evm", integrity_dir);
-	if (!evm_dir || IS_ERR(evm_dir))
+	if (IS_ERR(evm_dir))
 		return -EFAULT;
 
-	evm_init_tpm = securityfs_create_file("evm", 0660,
-					      evm_dir, NULL, &evm_key_ops);
-	if (!evm_init_tpm || IS_ERR(evm_init_tpm)) {
+	dentry = securityfs_create_file("evm", 0660,
+				      evm_dir, NULL, &evm_key_ops);
+	if (IS_ERR(dentry)) {
 		error = -EFAULT;
 		goto out;
 	}
 
 	evm_symlink = securityfs_create_symlink("evm", NULL,
 						"integrity/evm/evm", NULL);
-	if (!evm_symlink || IS_ERR(evm_symlink)) {
+	if (IS_ERR(evm_symlink)) {
 		error = -EFAULT;
 		goto out;
 	}
@@ -328,7 +328,6 @@ int __init evm_init_secfs(void)
 	return 0;
 out:
 	securityfs_remove(evm_symlink);
-	securityfs_remove(evm_init_tpm);
 	securityfs_remove(evm_dir);
 	return error;
 }
-- 
2.39.5