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

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

[alexjlzheng]

From: Jinliang Zheng <alexjlzheng@tencent.com>

Consider the following module code (just an example to make it easier to
illustrate the problem, in fact the LSM module will not be dynamically
unloaded):

  static struct dentry *dentry;

  static int __init securityfs_test_init(void)
  {
          dentry = securityfs_create_dir("standon", NULL);
          return PTR_ERR(dentry);
  }

  static void __exit securityfs_test_exit(void)
  {
          securityfs_remove(dentry);
  }

  module_init(securityfs_test_init);
  module_exit(securityfs_test_exit);

and then:

  insmod /path/to/thismodule
  cd /sys/kernel/security/standon     <- we hold 'standon'
  rmmod thismodule                    <- 'standon' don't go away
  insmod /path/to/thismodule          <- 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:
v2: Modify the commit message to make it clearer
v1: https://lore.kernel.org/all/20250426150931.2840-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 v2] securityfs: fix missing of d_delete() in securityfs_remove()

[Paul Moore]

On Wed, May 7, 2025 at 7:12 AM <alexjlzheng@gmail.com> wrote:
> From: Jinliang Zheng <alexjlzheng@tencent.com>
>
> Consider the following module code (just an example to make it easier to
> illustrate the problem, in fact the LSM module will not be dynamically
> unloaded):
>
>   static struct dentry *dentry;
>
>   static int __init securityfs_test_init(void)
>   {
>           dentry = securityfs_create_dir("standon", NULL);
>           return PTR_ERR(dentry);
>   }
>
>   static void __exit securityfs_test_exit(void)
>   {
>           securityfs_remove(dentry);
>   }
>
>   module_init(securityfs_test_init);
>   module_exit(securityfs_test_exit);
>
> and then:
>
>   insmod /path/to/thismodule
>   cd /sys/kernel/security/standon     <- we hold 'standon'
>   rmmod thismodule                    <- 'standon' don't go away
>   insmod /path/to/thismodule          <- Failed: File exists!

I mentioned this on your original patch, but I'll mention it again
with a bit more of an explanation behind it.  As you know, we don't
currently support dynamically loaded LSMs, which means the reproducer
above isn't really valid from a supported configuration perspective,
even if it does happen to trigger the behavior you are describing.
This may seem silly to you, but you really should stick with valid
configurations when trying to reproduce things as sometimes when
developers see an invalid/unsupported config they may stop reading and
dismiss your concern with a "don't do that!", which is surely not what
you want.

At the very least, I'm personally not sure we would want an
invalid/unsupported reproducer in the git log for the LSM subsystem.

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

I wondering why we don't simply replace all instances of
securityfs_remove() with securityfs_recursive_remove(), or more likely
just remove the existing securityfs_remove() and rename the
securityfs_recursive_remove() to securityfs_remove().  Do any existing
LSMs rely on securityfs_remove() *not* acting recursively?

> Fixes: b67dbf9d4c198 ("[PATCH] add securityfs for all LSMs to use")
> Signed-off-by: Jinliang Zheng <alexjlzheng@tencent.com>
> ---
> changelog:
> v2: Modify the commit message to make it clearer
> v1: https://lore.kernel.org/all/20250426150931.2840-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

-- 
paul-moore.com

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

[Al Viro]

On Wed, May 07, 2025 at 04:10:11PM -0400, Paul Moore 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().
> 
> I wondering why we don't simply replace all instances of
> securityfs_remove() with securityfs_recursive_remove(), or more likely
> just remove the existing securityfs_remove() and rename the
> securityfs_recursive_remove() to securityfs_remove().  Do any existing
> LSMs rely on securityfs_remove() *not* acting recursively?

It's a bit trickier than that (there are interesting issues around
efi_secret_unlink() nonsense, as well as insane refcounting grabbing
two references where only one is needed to pin the damn thing)...

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

[Paul Moore]

On Wed, May 7, 2025 at 5:23 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Wed, May 07, 2025 at 04:10:11PM -0400, Paul Moore 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().
> >
> > I wondering why we don't simply replace all instances of
> > securityfs_remove() with securityfs_recursive_remove(), or more likely
> > just remove the existing securityfs_remove() and rename the
> > securityfs_recursive_remove() to securityfs_remove().  Do any existing
> > LSMs rely on securityfs_remove() *not* acting recursively?
>
> It's a bit trickier than that (there are interesting issues around
> efi_secret_unlink() nonsense, as well as insane refcounting grabbing
> two references where only one is needed to pin the damn thing)...

Fun :/

In that case, what do you think of the change suggested here by
Jinliang Zheng where we add a d_delete() to the existing
securityfs_remove() implementation?

-- 
paul-moore.com

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

[Jinliang Zheng]

On Wed, 7 May 2025 16:10:11 -0400, Paul Moore <paul@paul-moore.com> wrote:
> On Wed, May 7, 2025 at 7:12 AM <alexjlzheng@gmail.com> wrote:
> > From: Jinliang Zheng <alexjlzheng@tencent.com>
> >
> > Consider the following module code (just an example to make it easier to
> > illustrate the problem, in fact the LSM module will not be dynamically
> > unloaded):
> >
> >   static struct dentry *dentry;
> >
> >   static int __init securityfs_test_init(void)
> >   {
> >           dentry = securityfs_create_dir("standon", NULL);
> >           return PTR_ERR(dentry);
> >   }
> >
> >   static void __exit securityfs_test_exit(void)
> >   {
> >           securityfs_remove(dentry);
> >   }
> >
> >   module_init(securityfs_test_init);
> >   module_exit(securityfs_test_exit);
> >
> > and then:
> >
> >   insmod /path/to/thismodule
> >   cd /sys/kernel/security/standon     <- we hold 'standon'
> >   rmmod thismodule                    <- 'standon' don't go away
> >   insmod /path/to/thismodule          <- Failed: File exists!
> 
> I mentioned this on your original patch, but I'll mention it again
> with a bit more of an explanation behind it.  As you know, we don't
> currently support dynamically loaded LSMs, which means the reproducer
> above isn't really valid from a supported configuration perspective,
> even if it does happen to trigger the behavior you are describing.
> This may seem silly to you, but you really should stick with valid
> configurations when trying to reproduce things as sometimes when
> developers see an invalid/unsupported config they may stop reading and
> dismiss your concern with a "don't do that!", which is surely not what
> you want.
> 
> At the very least, I'm personally not sure we would want an
> invalid/unsupported reproducer in the git log for the LSM subsystem.

Thank you for your reply. :)

To clarify, the reproducer code never invokes security_add_hooks(), thus
this clearly does not constitute loading a new LSM. 

However, if you believe the current approach might be misinterpreted,
my v3 patch is available for consideration:
- https://lore.kernel.org/all/20250508140438.648533-2-alexjlzheng@tencent.com/

While I personally find the v2 reproducer more readable and straightforward,
I fully defer to your judgment on this matter.

thanks,
Jinliang Zheng. :)

> 
> > 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().
> 
> I wondering why we don't simply replace all instances of
> securityfs_remove() with securityfs_recursive_remove(), or more likely
> just remove the existing securityfs_remove() and rename the
> securityfs_recursive_remove() to securityfs_remove().  Do any existing
> LSMs rely on securityfs_remove() *not* acting recursively?
> 
> > Fixes: b67dbf9d4c198 ("[PATCH] add securityfs for all LSMs to use")
> > Signed-off-by: Jinliang Zheng <alexjlzheng@tencent.com>
> > ---
> > changelog:
> > v2: Modify the commit message to make it clearer
> > v1: https://lore.kernel.org/all/20250426150931.2840-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
> 
> -- 
> paul-moore.com

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