[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20250508142246.648785-1-alexjlzheng@tencent.com>
Date: Thu, 8 May 2025 22:22:44 +0800
From: Jinliang Zheng <alexjlzheng@...il.com>
To: paul@...l-moore.com
Cc: alexjlzheng@...il.com,
alexjlzheng@...cent.com,
chrisw@...l.org,
greg@...ah.com,
jmorris@...ei.org,
linux-kernel@...r.kernel.org,
linux-security-module@...r.kernel.org,
serge@...lyn.com,
wufan@...ux.microsoft.com
Subject: Re: [PATCH v2] securityfs: fix missing of d_delete() in securityfs_remove()
On Wed, 7 May 2025 16:10:11 -0400, Paul Moore <paul@...l-moore.com> wrote:
> On Wed, May 7, 2025 at 7:12 AM <alexjlzheng@...il.com> wrote:
> > From: Jinliang Zheng <alexjlzheng@...cent.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@...cent.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
Powered by blists - more mailing lists