[PATCH] lsm: make SECURITY_PATH always enabled

[Paul Moore]

On Tue, Apr 22, 2025 at 4:31 PM Song Liu <songliubraving at meta.com> wrote:
> > On Apr 22, 2025, at 12:53 PM, Paul Moore <paul at paul-moore.com> wrote:
> >
> > On Apr 22, 2025 Song Liu <song at kernel.org> wrote:
> >>
> >> Only TOMOYO needed CONFIG_SECURITY_PATH when it was introduced. But now,
> >> AppArmor, EVM, IMA and LandLock also need it. And kernels are likely built
> >> with at least one of these enabled if CONFIG_SECURITY is enabled. Let's
> >> simplify the dependency.
> >>
> >> Signed-off-by: Tetsuo Handa <penguin-kernel at I-love.SAKURA.ne.jp>
> >> Signed-off-by: Song Liu <song at kernel.org>
> >> ---
> >> This was initially proposed in [1], but got Nacked by Paul.
> >
> > I chose not to merge the patch with the following comment:
> >
> > "If a Kconfig option is only needed by a subset of LSMs, which is the
> >  case for CONFIG_SECURITY_PATH, it should remain a build-time option."
> >
> > ... and that reasoning still sounds reasonable to me today.
>
> I agree that a way to disable some LSM hooks at build time is valuable.
>
> >
> >> ... but it is really up to the LSMs to decide how
> >> they use struct path.
> >
> > LSMs are currently free to select CONFIG_SECURITY_PATH as needed.  In
> > fact, if you look any modern Linux tree you'll see that TOMOYO, AppArmor,
> > Landlock, IMA, and EVM all select CONFIG_SECURITY_PATH based on their
> > individual Kconfig settings.
>
> However, I don't think existing CONFIG_SECURITY_* are doing the right
> things. Among all the configs, CONFIG_SECURITY_PATH is the most awkward
> to me. Say , if we have CONFIG_SECURITY_PATH, shouldn't we also have
> CONFIG_SECURITY_INODE? IOW, something like:
>
> #ifdef CONFIG_SECURITY_INODE
> int security_inode_rmdir(struct inode *dir, struct dentry *dentry);
> #endif
>
> #ifdef CONFIG_SECURITY_PATH
> int security_path_rmdir(struct inode *dir, struct dentry *dentry);
> #endif

Without putting much thought into what would fall under
CONFIG_SECURITY_INODE, I think it would be interesting to see what
hooks one might be able to make conditional on such a Kconfig knob.
Using security_inode_permission() as a simple test, it looks like only
SELinux and Smack provide implementations, spot checks on a few other
security_*inode*() hooks shows similar, or even more limited, results.

You would need to spend some time to determine what LSM hooks are used
by which LSMs and adjust their Kconfigs appropriately for the new
CONFIG_SECURITY_INODE knob, but if you do that then I think that would
be okay.

> OR, maybe we should just remove security_inode_rmdir(), and users of
> security_inode_rmdir() can just use security_path_rmdir() instead?

Those two LSM hooks are called from slightly different places in the
codepath which has an impact on their environment.  For example, the
inode variant doesn't have to deal with directory inodes that don't
have a defined rmdir op, whereas the path variant does; the inode
variant doesn't have to worry about S_KERNEL_FILE files, the inode
variant has a refcount'd and locked dentry, etc.  Moving an existing
LSM, especially complex ones, from one LSM hook to another, is a
delicate operation and might not be worth it for such a small return.

If you are interested in reducing the impact of the "inode" LSM hooks,
CONFIG_SECURITY_INODE is likely a better place to start.

-- 
paul-moore.com