Re: [PATCH v5 2/8] LSM: Maintain a table of LSM attribute data

On 1/11/2023 1:01 PM, Paul Moore wrote:

On Mon, Jan 9, 2023 at 1:07 PM Casey Schaufler [off-list ref] wrote:

quoted

As LSMs are registered add their lsm_id pointers to a table.
This will be used later for attribute reporting.

Determine the number of possible security modules based on
their respective CONFIG options. This allows the number to be
known at build time. This allows data structures and tables
to use the constant.

Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
---
 include/linux/security.h |  2 ++
 security/security.c      | 44 +++++++++++++++++++++++++++++++++-------
 2 files changed, 39 insertions(+), 7 deletions(-)

diff --git a/include/linux/security.h b/include/linux/security.h
index 5b67f208f7de..33ed1860b96f 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h

@@ -138,6 +138,8 @@ enum lockdown_reason {
 };

 extern const char *const lockdown_reasons[LOCKDOWN_CONFIDENTIALITY_MAX+1];
+extern u32 lsm_active_cnt;
+extern struct lsm_id *lsm_idlist[];

 /* These functions are in security/commoncap.c */
 extern int cap_capable(const struct cred *cred, struct user_namespace *ns,

diff --git a/security/security.c b/security/security.c
index 07a8fe7f92bf..a590fa98ddd6 100644
--- a/security/security.c
+++ b/security/security.c

@@ -28,12 +28,29 @@
 #include <linux/backing-dev.h>
 #include <linux/string.h>
 #include <linux/msg.h>
+#include <uapi/linux/lsm.h>
 #include <net/flow.h>

 #define MAX_LSM_EVM_XATTR      2

-/* How many LSMs were built into the kernel? */
-#define LSM_COUNT (__end_lsm_info - __start_lsm_info)
+/*
+ * How many LSMs are built into the kernel as determined at
+ * build time. Used to determine fixed array sizes.
+ * The capability module is accounted for by CONFIG_SECURITY
+ */
+#define LSM_COUNT ( \
+       (IS_ENABLED(CONFIG_SECURITY) ? 1 : 0) + \
+       (IS_ENABLED(CONFIG_SECURITY_SELINUX) ? 1 : 0) + \
+       (IS_ENABLED(CONFIG_SECURITY_SMACK) ? 1 : 0) + \
+       (IS_ENABLED(CONFIG_SECURITY_TOMOYO) ? 1 : 0) + \
+       (IS_ENABLED(CONFIG_SECURITY_IMA) ? 1 : 0) + \
+       (IS_ENABLED(CONFIG_SECURITY_APPARMOR) ? 1 : 0) + \
+       (IS_ENABLED(CONFIG_SECURITY_YAMA) ? 1 : 0) + \
+       (IS_ENABLED(CONFIG_SECURITY_LOADPIN) ? 1 : 0) + \
+       (IS_ENABLED(CONFIG_SECURITY_SAFESETID) ? 1 : 0) + \
+       (IS_ENABLED(CONFIG_SECURITY_LOCKDOWN_LSM) ? 1 : 0) + \
+       (IS_ENABLED(CONFIG_BPF_LSM) ? 1 : 0) + \
+       (IS_ENABLED(CONFIG_SECURITY_LANDLOCK) ? 1 : 0))

 /*
  * These are descriptions of the reasons that can be passed to the

@@ -90,7 +107,7 @@ static __initdata const char *chosen_major_lsm;
 static __initconst const char * const builtin_lsm_order = CONFIG_LSM;

 /* Ordered list of LSMs to initialize. */
-static __initdata struct lsm_info **ordered_lsms;
+static __initdata struct lsm_info *ordered_lsms[LSM_COUNT + 1];

I'm guessing this 'LSM_COUNT + 1' logic is basically just copied from
ordered_lsm_init() - which is okay - but can you remind me why it is
'LSM_COUNT + 1' and not just 'LSM_COUNT'?  Based on the LSM_COUNT
macro above it seems like LSM_COUNT should be enough, no?

Yup. I didn't spend a lot of time investigating why the + 1.
I'll look more deeply and correct if appropriate.

quoted

 static __initdata struct lsm_info *exclusive;

 static __initdata bool debug;

@@ -341,13 +358,16 @@ static void __init report_lsm_order(void)
        pr_cont("\n");
 }

+/*
+ * Current index to use while initializing the lsm id list.
+ */
+u32 lsm_active_cnt __lsm_ro_after_init;
+struct lsm_id *lsm_idlist[LSM_COUNT] __lsm_ro_after_init;
+
 static void __init ordered_lsm_init(void)
 {
        struct lsm_info **lsm;

-       ordered_lsms = kcalloc(LSM_COUNT + 1, sizeof(*ordered_lsms),
-                               GFP_KERNEL);
-
        if (chosen_lsm_order) {
                if (chosen_major_lsm) {
                        pr_warn("security=%s is ignored because it is superseded by lsm=%s\n",

@@ -388,7 +408,7 @@ static void __init ordered_lsm_init(void)
        for (lsm = ordered_lsms; *lsm; lsm++)
                initialize_lsm(*lsm);

-       kfree(ordered_lsms);
+       init_debug("lsm count            = %d\n", lsm_active_cnt);
 }

Given 86ef3c735ec8 ("LSM: Better reporting of actual LSMs at boot"),
is this needed?

None of what comes out from lsm.debug is strictly necessary, and
human or script can parse "initializing lsm=", but sometimes the
number of LSMs is interesting.

--
paul-moore.com