[PATCH v23 02/23] LSM: Create and manage the lsmblob data structure.

[PATCH v23 02/23] LSM: Create and manage the lsmblob data structure.

[Casey Schaufler]

When more than one security module is exporting data to
audit and networking sub-systems a single 32 bit integer
is no longer sufficient to represent the data. Add a
structure to be used instead.

The lsmblob structure is currently an array of
u32 "secids". There is an entry for each of the
security modules built into the system that would
use secids if active. The system assigns the module
a "slot" when it registers hooks. If modules are
compiled in but not registered there will be unused
slots.

A new lsm_id structure, which contains the name
of the LSM and its slot number, is created. There
is an instance for each LSM, which assigns the name
and passes it to the infrastructure to set the slot.

The audit rules data is expanded to use an array of
security module data rather than a single instance.
Because IMA uses the audit rule functions it is
affected as well.

Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
Acked-by: Paul Moore <paul@paul-moore.com>
Acked-by: John Johansen <john.johansen@canonical.com>
Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
Cc: <bpf@vger.kernel.org>
Cc: linux-audit@redhat.com
Cc: linux-security-module@vger.kernel.org
Cc: selinux@vger.kernel.org
---
 include/linux/audit.h               |  4 +-
 include/linux/lsm_hooks.h           | 12 ++++-
 include/linux/security.h            | 67 +++++++++++++++++++++++++--
 kernel/auditfilter.c                | 24 +++++-----
 kernel/auditsc.c                    | 12 ++---
 security/apparmor/lsm.c             |  7 ++-
 security/bpf/hooks.c                | 12 ++++-
 security/commoncap.c                |  7 ++-
 security/integrity/ima/ima_policy.c | 40 +++++++++++-----
 security/loadpin/loadpin.c          |  8 +++-
 security/lockdown/lockdown.c        |  7 ++-
 security/safesetid/lsm.c            |  8 +++-
 security/security.c                 | 72 ++++++++++++++++++++++++-----
 security/selinux/hooks.c            |  8 +++-
 security/smack/smack_lsm.c          |  7 ++-
 security/tomoyo/tomoyo.c            |  8 +++-
 security/yama/yama_lsm.c            |  7 ++-
 17 files changed, 254 insertions(+), 56 deletions(-)

diff --git a/include/linux/audit.h b/include/linux/audit.h
index b3d859831a31..ba1cd38d601b 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -11,6 +11,7 @@
 
 #include <linux/sched.h>
 #include <linux/ptrace.h>
+#include <linux/security.h>
 #include <uapi/linux/audit.h>
 #include <uapi/linux/netfilter/nf_tables.h>
 
@@ -65,8 +66,9 @@ struct audit_field {
 		kuid_t			uid;
 		kgid_t			gid;
 		struct {
+			bool		lsm_isset;
 			char		*lsm_str;
-			void		*lsm_rule;
+			void		*lsm_rules[LSMBLOB_ENTRIES];
 		};
 	};
 	u32				op;
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index d8f492ed6ebf..fe9203f15993 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1545,6 +1545,14 @@ struct security_hook_heads {
 	#undef LSM_HOOK
 } __randomize_layout;
 
+/*
+ * Information that identifies a security module.
+ */
+struct lsm_id {
+	const char	*lsm;	/* Name of the LSM */
+	int		slot;	/* Slot in lsmblob if one is allocated */
+};
+
 /*
  * Security module hook list structure.
  * For use with generic list macros for common operations.
@@ -1553,7 +1561,7 @@ struct security_hook_list {
 	struct hlist_node		list;
 	struct hlist_head		*head;
 	union security_list_options	hook;
-	char				*lsm;
+	struct lsm_id			*lsmid;
 } __randomize_layout;
 
 /*
@@ -1588,7 +1596,7 @@ extern struct security_hook_heads security_hook_heads;
 extern char *lsm_names;
 
 extern void security_add_hooks(struct security_hook_list *hooks, int count,
-				char *lsm);
+			       struct lsm_id *lsmid);
 
 #define LSM_FLAG_LEGACY_MAJOR	BIT(0)
 #define LSM_FLAG_EXCLUSIVE	BIT(1)
diff --git a/include/linux/security.h b/include/linux/security.h
index bc2725491560..fdb6e95c98e8 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -132,6 +132,65 @@ enum lockdown_reason {
 
 extern const char *const lockdown_reasons[LOCKDOWN_CONFIDENTIALITY_MAX+1];
 
+/*
+ * Data exported by the security modules
+ *
+ * Any LSM that provides secid or secctx based hooks must be included.
+ */
+#define LSMBLOB_ENTRIES ( \
+	(IS_ENABLED(CONFIG_SECURITY_SELINUX) ? 1 : 0) + \
+	(IS_ENABLED(CONFIG_SECURITY_SMACK) ? 1 : 0) + \
+	(IS_ENABLED(CONFIG_SECURITY_APPARMOR) ? 1 : 0) + \
+	(IS_ENABLED(CONFIG_BPF_LSM) ? 1 : 0))
+
+struct lsmblob {
+	u32     secid[LSMBLOB_ENTRIES];
+};
+
+#define LSMBLOB_INVALID		-1	/* Not a valid LSM slot number */
+#define LSMBLOB_NEEDED		-2	/* Slot requested on initialization */
+#define LSMBLOB_NOT_NEEDED	-3	/* Slot not requested */
+
+/**
+ * lsmblob_init - initialize an lsmblob structure.
+ * @blob: Pointer to the data to initialize
+ * @secid: The initial secid value
+ *
+ * Set all secid for all modules to the specified value.
+ */
+static inline void lsmblob_init(struct lsmblob *blob, u32 secid)
+{
+	int i;
+
+	for (i = 0; i < LSMBLOB_ENTRIES; i++)
+		blob->secid[i] = secid;
+}
+
+/**
+ * lsmblob_is_set - report if there is an value in the lsmblob
+ * @blob: Pointer to the exported LSM data
+ *
+ * Returns true if there is a secid set, false otherwise
+ */
+static inline bool lsmblob_is_set(struct lsmblob *blob)
+{
+	struct lsmblob empty = {};
+
+	return !!memcmp(blob, &empty, sizeof(*blob));
+}
+
+/**
+ * lsmblob_equal - report if the two lsmblob's are equal
+ * @bloba: Pointer to one LSM data
+ * @blobb: Pointer to the other LSM data
+ *
+ * Returns true if all entries in the two are equal, false otherwise
+ */
+static inline bool lsmblob_equal(struct lsmblob *bloba, struct lsmblob *blobb)
+{
+	return !memcmp(bloba, blobb, sizeof(*bloba));
+}
+
 /* These functions are in security/commoncap.c */
 extern int cap_capable(const struct cred *cred, struct user_namespace *ns,
 		       int cap, unsigned int opts);
@@ -1833,8 +1892,8 @@ static inline int security_key_getsecurity(struct key *key, char **_buffer)
 #ifdef CONFIG_SECURITY
 int security_audit_rule_init(u32 field, u32 op, char *rulestr, void **lsmrule);
 int security_audit_rule_known(struct audit_krule *krule);
-int security_audit_rule_match(u32 secid, u32 field, u32 op, void *lsmrule);
-void security_audit_rule_free(void *lsmrule);
+int security_audit_rule_match(u32 secid, u32 field, u32 op, void **lsmrule);
+void security_audit_rule_free(void **lsmrule);
 
 #else
 
@@ -1850,12 +1909,12 @@ static inline int security_audit_rule_known(struct audit_krule *krule)
 }
 
 static inline int security_audit_rule_match(u32 secid, u32 field, u32 op,
-					    void *lsmrule)
+					    void **lsmrule)
 {
 	return 0;
 }
 
-static inline void security_audit_rule_free(void *lsmrule)
+static inline void security_audit_rule_free(void **lsmrule)
 { }
 
 #endif /* CONFIG_SECURITY */
diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index 333b3bcfc545..45da229f9f1f 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -74,7 +74,7 @@ static void audit_free_lsm_field(struct audit_field *f)
 	case AUDIT_OBJ_LEV_LOW:
 	case AUDIT_OBJ_LEV_HIGH:
 		kfree(f->lsm_str);
-		security_audit_rule_free(f->lsm_rule);
+		security_audit_rule_free(f->lsm_rules);
 	}
 }
 
@@ -519,9 +519,10 @@ static struct audit_entry *audit_data_to_entry(struct audit_rule_data *data,
 				goto exit_free;
 			}
 			entry->rule.buflen += f_val;
+			f->lsm_isset = true;
 			f->lsm_str = str;
 			err = security_audit_rule_init(f->type, f->op, str,
-						       (void **)&f->lsm_rule);
+						       f->lsm_rules);
 			/* Keep currently invalid fields around in case they
 			 * become valid after a policy reload. */
 			if (err == -EINVAL) {
@@ -774,7 +775,7 @@ static int audit_compare_rule(struct audit_krule *a, struct audit_krule *b)
 	return 0;
 }
 
-/* Duplicate LSM field information.  The lsm_rule is opaque, so must be
+/* Duplicate LSM field information.  The lsm_rules is opaque, so must be
  * re-initialized. */
 static inline int audit_dupe_lsm_field(struct audit_field *df,
 					   struct audit_field *sf)
@@ -788,9 +789,9 @@ static inline int audit_dupe_lsm_field(struct audit_field *df,
 		return -ENOMEM;
 	df->lsm_str = lsm_str;
 
-	/* our own (refreshed) copy of lsm_rule */
+	/* our own (refreshed) copy of lsm_rules */
 	ret = security_audit_rule_init(df->type, df->op, df->lsm_str,
-				       (void **)&df->lsm_rule);
+				       df->lsm_rules);
 	/* Keep currently invalid fields around in case they
 	 * become valid after a policy reload. */
 	if (ret == -EINVAL) {
@@ -842,7 +843,7 @@ struct audit_entry *audit_dupe_rule(struct audit_krule *old)
 	new->tree = old->tree;
 	memcpy(new->fields, old->fields, sizeof(struct audit_field) * fcount);
 
-	/* deep copy this information, updating the lsm_rule fields, because
+	/* deep copy this information, updating the lsm_rules fields, because
 	 * the originals will all be freed when the old rule is freed. */
 	for (i = 0; i < fcount; i++) {
 		switch (new->fields[i].type) {
@@ -1358,10 +1359,11 @@ int audit_filter(int msgtype, unsigned int listtype)
 			case AUDIT_SUBJ_TYPE:
 			case AUDIT_SUBJ_SEN:
 			case AUDIT_SUBJ_CLR:
-				if (f->lsm_rule) {
+				if (f->lsm_isset) {
 					security_task_getsecid(current, &sid);
 					result = security_audit_rule_match(sid,
-						   f->type, f->op, f->lsm_rule);
+						   f->type, f->op,
+						   f->lsm_rules);
 				}
 				break;
 			case AUDIT_EXE:
@@ -1388,7 +1390,7 @@ int audit_filter(int msgtype, unsigned int listtype)
 	return ret;
 }
 
-static int update_lsm_rule(struct audit_krule *r)
+static int update_lsm_rules(struct audit_krule *r)
 {
 	struct audit_entry *entry = container_of(r, struct audit_entry, rule);
 	struct audit_entry *nentry;
@@ -1420,7 +1422,7 @@ static int update_lsm_rule(struct audit_krule *r)
 	return err;
 }
 
-/* This function will re-initialize the lsm_rule field of all applicable rules.
+/* This function will re-initialize the lsm_rules field of all applicable rules.
  * It will traverse the filter lists serarching for rules that contain LSM
  * specific filter fields.  When such a rule is found, it is copied, the
  * LSM field is re-initialized, and the old rule is replaced with the
@@ -1435,7 +1437,7 @@ int audit_update_lsm_rules(void)
 
 	for (i = 0; i < AUDIT_NR_FILTERS; i++) {
 		list_for_each_entry_safe(r, n, &audit_rules_list[i], list) {
-			int res = update_lsm_rule(r);
+			int res = update_lsm_rules(r);
 			if (!err)
 				err = res;
 		}
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 8dba8f0983b5..16e3430f7d07 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -667,14 +667,14 @@ static int audit_filter_rules(struct task_struct *tsk,
 			   match for now to avoid losing information that
 			   may be wanted.   An error message will also be
 			   logged upon error */
-			if (f->lsm_rule) {
+			if (f->lsm_isset) {
 				if (need_sid) {
 					security_task_getsecid(tsk, &sid);
 					need_sid = 0;
 				}
 				result = security_audit_rule_match(sid, f->type,
 								   f->op,
-								   f->lsm_rule);
+								   f->lsm_rules);
 			}
 			break;
 		case AUDIT_OBJ_USER:
@@ -684,21 +684,21 @@ static int audit_filter_rules(struct task_struct *tsk,
 		case AUDIT_OBJ_LEV_HIGH:
 			/* The above note for AUDIT_SUBJ_USER...AUDIT_SUBJ_CLR
 			   also applies here */
-			if (f->lsm_rule) {
+			if (f->lsm_isset) {
 				/* Find files that match */
 				if (name) {
 					result = security_audit_rule_match(
 								name->osid,
 								f->type,
 								f->op,
-								f->lsm_rule);
+								f->lsm_rules);
 				} else if (ctx) {
 					list_for_each_entry(n, &ctx->names_list, list) {
 						if (security_audit_rule_match(
 								n->osid,
 								f->type,
 								f->op,
-								f->lsm_rule)) {
+								f->lsm_rules)) {
 							++result;
 							break;
 						}
@@ -709,7 +709,7 @@ static int audit_filter_rules(struct task_struct *tsk,
 					break;
 				if (security_audit_rule_match(ctx->ipc.osid,
 							      f->type, f->op,
-							      f->lsm_rule))
+							      f->lsm_rules))
 					++result;
 			}
 			break;
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index f1c365905d5e..432915c1d427 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -1152,6 +1152,11 @@ struct lsm_blob_sizes apparmor_blob_sizes __lsm_ro_after_init = {
 	.lbs_sock = sizeof(struct aa_sk_ctx),
 };
 
+static struct lsm_id apparmor_lsmid __lsm_ro_after_init = {
+	.lsm  = "apparmor",
+	.slot = LSMBLOB_NEEDED
+};
+
 static struct security_hook_list apparmor_hooks[] __lsm_ro_after_init = {
 	LSM_HOOK_INIT(ptrace_access_check, apparmor_ptrace_access_check),
 	LSM_HOOK_INIT(ptrace_traceme, apparmor_ptrace_traceme),
@@ -1852,7 +1857,7 @@ static int __init apparmor_init(void)
 		goto buffers_out;
 	}
 	security_add_hooks(apparmor_hooks, ARRAY_SIZE(apparmor_hooks),
-				"apparmor");
+				&apparmor_lsmid);
 
 	/* Report that AppArmor successfully initialized */
 	apparmor_initialized = 1;
diff --git a/security/bpf/hooks.c b/security/bpf/hooks.c
index 788667d582ae..a1a5032a4d87 100644
--- a/security/bpf/hooks.c
+++ b/security/bpf/hooks.c
@@ -14,9 +14,19 @@ static struct security_hook_list bpf_lsm_hooks[] __lsm_ro_after_init = {
 	LSM_HOOK_INIT(inode_free_security, bpf_inode_storage_free),
 };
 
+/*
+ * slot has to be LSMBLOB_NEEDED because some of the hooks
+ * supplied by this module require a slot.
+ */
+struct lsm_id bpf_lsmid __lsm_ro_after_init = {
+	.lsm = "bpf",
+	.slot = LSMBLOB_NEEDED
+};
+
 static int __init bpf_lsm_init(void)
 {
-	security_add_hooks(bpf_lsm_hooks, ARRAY_SIZE(bpf_lsm_hooks), "bpf");
+	security_add_hooks(bpf_lsm_hooks, ARRAY_SIZE(bpf_lsm_hooks),
+			   &bpf_lsmid);
 	pr_info("LSM support for eBPF active\n");
 	return 0;
 }
diff --git a/security/commoncap.c b/security/commoncap.c
index 59bf3c1674c8..959a9f96b7f1 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -1341,6 +1341,11 @@ int cap_mmap_file(struct file *file, unsigned long reqprot,
 
 #ifdef CONFIG_SECURITY
 
+static struct lsm_id capability_lsmid __lsm_ro_after_init = {
+	.lsm  = "capability",
+	.slot = LSMBLOB_NOT_NEEDED
+};
+
 static struct security_hook_list capability_hooks[] __lsm_ro_after_init = {
 	LSM_HOOK_INIT(capable, cap_capable),
 	LSM_HOOK_INIT(settime, cap_settime),
@@ -1365,7 +1370,7 @@ static struct security_hook_list capability_hooks[] __lsm_ro_after_init = {
 static int __init capability_init(void)
 {
 	security_add_hooks(capability_hooks, ARRAY_SIZE(capability_hooks),
-				"capability");
+			   &capability_lsmid);
 	return 0;
 }
 
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 9b5adeaa47fc..cd393aaa17d5 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -79,7 +79,7 @@ struct ima_rule_entry {
 	bool (*fowner_op)(kuid_t, kuid_t); /* uid_eq(), uid_gt(), uid_lt() */
 	int pcr;
 	struct {
-		void *rule;	/* LSM file metadata specific */
+		void *rules[LSMBLOB_ENTRIES]; /* LSM file metadata specific */
 		char *args_p;	/* audit value */
 		int type;	/* audit type */
 	} lsm[MAX_LSM_RULES];
@@ -88,6 +88,22 @@ struct ima_rule_entry {
 	struct ima_template_desc *template;
 };
 
+/**
+ * ima_lsm_isset - Is a rule set for any of the active security modules
+ * @rules: The set of IMA rules to check.
+ *
+ * If a rule is set for any LSM return true, otherwise return false.
+ */
+static inline bool ima_lsm_isset(void *rules[])
+{
+	int i;
+
+	for (i = 0; i < LSMBLOB_ENTRIES; i++)
+		if (rules[i])
+			return true;
+	return false;
+}
+
 /*
  * Without LSM specific knowledge, the default policy can only be
  * written in terms of .action, .func, .mask, .fsmagic, .uid, and .fowner
@@ -326,9 +342,11 @@ static void ima_free_rule_opt_list(struct ima_rule_opt_list *opt_list)
 static void ima_lsm_free_rule(struct ima_rule_entry *entry)
 {
 	int i;
+	int r;
 
 	for (i = 0; i < MAX_LSM_RULES; i++) {
-		ima_filter_rule_free(entry->lsm[i].rule);
+		for (r = 0; r < LSMBLOB_ENTRIES; r++)
+			ima_filter_rule_free(entry->lsm[i].rules[r]);
 		kfree(entry->lsm[i].args_p);
 	}
 }
@@ -379,8 +397,8 @@ static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry)
 
 		ima_filter_rule_init(nentry->lsm[i].type, Audit_equal,
 				     nentry->lsm[i].args_p,
-				     &nentry->lsm[i].rule);
-		if (!nentry->lsm[i].rule)
+				     &nentry->lsm[i].rules[0]);
+		if (!ima_lsm_isset(nentry->lsm[i].rules))
 			pr_warn("rule for LSM \'%s\' is undefined\n",
 				nentry->lsm[i].args_p);
 	}
@@ -545,7 +563,7 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
 		int rc = 0;
 		u32 osid;
 
-		if (!rule->lsm[i].rule) {
+		if (!ima_lsm_isset(rule->lsm[i].rules)) {
 			if (!rule->lsm[i].args_p)
 				continue;
 			else
@@ -558,14 +576,14 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
 			security_inode_getsecid(inode, &osid);
 			rc = ima_filter_rule_match(osid, rule->lsm[i].type,
 						   Audit_equal,
-						   rule->lsm[i].rule);
+						   rule->lsm[i].rules);
 			break;
 		case LSM_SUBJ_USER:
 		case LSM_SUBJ_ROLE:
 		case LSM_SUBJ_TYPE:
 			rc = ima_filter_rule_match(secid, rule->lsm[i].type,
 						   Audit_equal,
-						   rule->lsm[i].rule);
+						   rule->lsm[i].rules);
 		default:
 			break;
 		}
@@ -952,7 +970,7 @@ static int ima_lsm_rule_init(struct ima_rule_entry *entry,
 {
 	int result;
 
-	if (entry->lsm[lsm_rule].rule)
+	if (ima_lsm_isset(entry->lsm[lsm_rule].rules))
 		return -EINVAL;
 
 	entry->lsm[lsm_rule].args_p = match_strdup(args);
@@ -962,8 +980,8 @@ static int ima_lsm_rule_init(struct ima_rule_entry *entry,
 	entry->lsm[lsm_rule].type = audit_type;
 	result = ima_filter_rule_init(entry->lsm[lsm_rule].type, Audit_equal,
 				      entry->lsm[lsm_rule].args_p,
-				      &entry->lsm[lsm_rule].rule);
-	if (!entry->lsm[lsm_rule].rule) {
+				      &entry->lsm[lsm_rule].rules[0]);
+	if (!ima_lsm_isset(entry->lsm[lsm_rule].rules)) {
 		pr_warn("rule for LSM \'%s\' is undefined\n",
 			entry->lsm[lsm_rule].args_p);
 
@@ -1733,7 +1751,7 @@ int ima_policy_show(struct seq_file *m, void *v)
 	}
 
 	for (i = 0; i < MAX_LSM_RULES; i++) {
-		if (entry->lsm[i].rule) {
+		if (ima_lsm_isset(entry->lsm[i].rules)) {
 			switch (i) {
 			case LSM_OBJ_USER:
 				seq_printf(m, pt(Opt_obj_user),
diff --git a/security/loadpin/loadpin.c b/security/loadpin/loadpin.c
index b12f7d986b1e..b569f3bc170b 100644
--- a/security/loadpin/loadpin.c
+++ b/security/loadpin/loadpin.c
@@ -192,6 +192,11 @@ static int loadpin_load_data(enum kernel_load_data_id id, bool contents)
 	return loadpin_read_file(NULL, (enum kernel_read_file_id) id, contents);
 }
 
+static struct lsm_id loadpin_lsmid __lsm_ro_after_init = {
+	.lsm  = "loadpin",
+	.slot = LSMBLOB_NOT_NEEDED
+};
+
 static struct security_hook_list loadpin_hooks[] __lsm_ro_after_init = {
 	LSM_HOOK_INIT(sb_free_security, loadpin_sb_free_security),
 	LSM_HOOK_INIT(kernel_read_file, loadpin_read_file),
@@ -239,7 +244,8 @@ static int __init loadpin_init(void)
 	pr_info("ready to pin (currently %senforcing)\n",
 		enforce ? "" : "not ");
 	parse_exclude();
-	security_add_hooks(loadpin_hooks, ARRAY_SIZE(loadpin_hooks), "loadpin");
+	security_add_hooks(loadpin_hooks, ARRAY_SIZE(loadpin_hooks),
+			   &loadpin_lsmid);
 	return 0;
 }
 
diff --git a/security/lockdown/lockdown.c b/security/lockdown/lockdown.c
index 87cbdc64d272..4e24ea3f7b7e 100644
--- a/security/lockdown/lockdown.c
+++ b/security/lockdown/lockdown.c
@@ -75,6 +75,11 @@ static struct security_hook_list lockdown_hooks[] __lsm_ro_after_init = {
 	LSM_HOOK_INIT(locked_down, lockdown_is_locked_down),
 };
 
+static struct lsm_id lockdown_lsmid __lsm_ro_after_init = {
+	.lsm = "lockdown",
+	.slot = LSMBLOB_NOT_NEEDED
+};
+
 static int __init lockdown_lsm_init(void)
 {
 #if defined(CONFIG_LOCK_DOWN_KERNEL_FORCE_INTEGRITY)
@@ -83,7 +88,7 @@ static int __init lockdown_lsm_init(void)
 	lock_kernel_down("Kernel configuration", LOCKDOWN_CONFIDENTIALITY_MAX);
 #endif
 	security_add_hooks(lockdown_hooks, ARRAY_SIZE(lockdown_hooks),
-			   "lockdown");
+			   &lockdown_lsmid);
 	return 0;
 }
 
diff --git a/security/safesetid/lsm.c b/security/safesetid/lsm.c
index 8a176b6adbe5..7c7ac9bfe5cd 100644
--- a/security/safesetid/lsm.c
+++ b/security/safesetid/lsm.c
@@ -244,6 +244,11 @@ static int safesetid_task_fix_setgid(struct cred *new,
 	return -EACCES;
 }
 
+static struct lsm_id safesetid_lsmid __lsm_ro_after_init = {
+	.lsm  = "safesetid",
+	.slot = LSMBLOB_NOT_NEEDED
+};
+
 static struct security_hook_list safesetid_security_hooks[] = {
 	LSM_HOOK_INIT(task_fix_setuid, safesetid_task_fix_setuid),
 	LSM_HOOK_INIT(task_fix_setgid, safesetid_task_fix_setgid),
@@ -253,7 +258,8 @@ static struct security_hook_list safesetid_security_hooks[] = {
 static int __init safesetid_security_init(void)
 {
 	security_add_hooks(safesetid_security_hooks,
-			   ARRAY_SIZE(safesetid_security_hooks), "safesetid");
+			   ARRAY_SIZE(safesetid_security_hooks),
+			   &safesetid_lsmid);
 
 	/* Report that SafeSetID successfully initialized */
 	safesetid_initialized = 1;
diff --git a/security/security.c b/security/security.c
index 5da8b3643680..d01363cb0082 100644
--- a/security/security.c
+++ b/security/security.c
@@ -341,6 +341,7 @@ static void __init ordered_lsm_init(void)
 	init_debug("msg_msg blob size  = %d\n", blob_sizes.lbs_msg_msg);
 	init_debug("sock blob size     = %d\n", blob_sizes.lbs_sock);
 	init_debug("task blob size     = %d\n", blob_sizes.lbs_task);
+	init_debug("lsmblob size       = %zu\n", sizeof(struct lsmblob));
 
 	/*
 	 * Create any kmem_caches needed for blobs
@@ -468,21 +469,36 @@ static int lsm_append(const char *new, char **result)
 	return 0;
 }
 
+/*
+ * Current index to use while initializing the lsmblob secid list.
+ */
+static int lsm_slot __lsm_ro_after_init;
+
 /**
  * security_add_hooks - Add a modules hooks to the hook lists.
  * @hooks: the hooks to add
  * @count: the number of hooks to add
- * @lsm: the name of the security module
+ * @lsmid: the the identification information for the security module
  *
  * Each LSM has to register its hooks with the infrastructure.
+ * If the LSM is using hooks that export secids allocate a slot
+ * for it in the lsmblob.
  */
 void __init security_add_hooks(struct security_hook_list *hooks, int count,
-				char *lsm)
+			       struct lsm_id *lsmid)
 {
 	int i;
 
+	if (lsmid->slot == LSMBLOB_NEEDED) {
+		if (lsm_slot >= LSMBLOB_ENTRIES)
+			panic("%s Too many LSMs registered.\n", __func__);
+		lsmid->slot = lsm_slot++;
+		init_debug("%s assigned lsmblob slot %d\n", lsmid->lsm,
+			   lsmid->slot);
+	}
+
 	for (i = 0; i < count; i++) {
-		hooks[i].lsm = lsm;
+		hooks[i].lsmid = lsmid;
 		hlist_add_tail_rcu(&hooks[i].list, hooks[i].head);
 	}
 
@@ -491,7 +507,7 @@ void __init security_add_hooks(struct security_hook_list *hooks, int count,
 	 * and fix this up afterwards.
 	 */
 	if (slab_is_available()) {
-		if (lsm_append(lsm, &lsm_names) < 0)
+		if (lsm_append(lsmid->lsm, &lsm_names) < 0)
 			panic("%s - Cannot get early memory.\n", __func__);
 	}
 }
@@ -2005,7 +2021,7 @@ int security_getprocattr(struct task_struct *p, const char *lsm, char *name,
 	struct security_hook_list *hp;
 
 	hlist_for_each_entry(hp, &security_hook_heads.getprocattr, list) {
-		if (lsm != NULL && strcmp(lsm, hp->lsm))
+		if (lsm != NULL && strcmp(lsm, hp->lsmid->lsm))
 			continue;
 		return hp->hook.getprocattr(p, name, value);
 	}
@@ -2018,7 +2034,7 @@ int security_setprocattr(const char *lsm, const char *name, void *value,
 	struct security_hook_list *hp;
 
 	hlist_for_each_entry(hp, &security_hook_heads.setprocattr, list) {
-		if (lsm != NULL && strcmp(lsm, hp->lsm))
+		if (lsm != NULL && strcmp(lsm, hp->lsmid->lsm))
 			continue;
 		return hp->hook.setprocattr(name, value, size);
 	}
@@ -2510,7 +2526,24 @@ int security_key_getsecurity(struct key *key, char **_buffer)
 
 int security_audit_rule_init(u32 field, u32 op, char *rulestr, void **lsmrule)
 {
-	return call_int_hook(audit_rule_init, 0, field, op, rulestr, lsmrule);
+	struct security_hook_list *hp;
+	bool one_is_good = false;
+	int rc = 0;
+	int trc;
+
+	hlist_for_each_entry(hp, &security_hook_heads.audit_rule_init, list) {
+		if (WARN_ON(hp->lsmid->slot < 0 || hp->lsmid->slot >= lsm_slot))
+			continue;
+		trc = hp->hook.audit_rule_init(field, op, rulestr,
+					       &lsmrule[hp->lsmid->slot]);
+		if (trc == 0)
+			one_is_good = true;
+		else
+			rc = trc;
+	}
+	if (one_is_good)
+		return 0;
+	return rc;
 }
 
 int security_audit_rule_known(struct audit_krule *krule)
@@ -2518,14 +2551,31 @@ int security_audit_rule_known(struct audit_krule *krule)
 	return call_int_hook(audit_rule_known, 0, krule);
 }
 
-void security_audit_rule_free(void *lsmrule)
+void security_audit_rule_free(void **lsmrule)
 {
-	call_void_hook(audit_rule_free, lsmrule);
+	struct security_hook_list *hp;
+
+	hlist_for_each_entry(hp, &security_hook_heads.audit_rule_free, list) {
+		if (WARN_ON(hp->lsmid->slot < 0 || hp->lsmid->slot >= lsm_slot))
+			continue;
+		hp->hook.audit_rule_free(lsmrule[hp->lsmid->slot]);
+	}
 }
 
-int security_audit_rule_match(u32 secid, u32 field, u32 op, void *lsmrule)
+int security_audit_rule_match(u32 secid, u32 field, u32 op, void **lsmrule)
 {
-	return call_int_hook(audit_rule_match, 0, secid, field, op, lsmrule);
+	struct security_hook_list *hp;
+	int rc;
+
+	hlist_for_each_entry(hp, &security_hook_heads.audit_rule_match, list) {
+		if (WARN_ON(hp->lsmid->slot < 0 || hp->lsmid->slot >= lsm_slot))
+			continue;
+		rc = hp->hook.audit_rule_match(secid, field, op,
+					       &lsmrule[hp->lsmid->slot]);
+		if (rc)
+			return rc;
+	}
+	return 0;
 }
 #endif /* CONFIG_AUDIT */
 
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 2748281a5cca..52a50d7ca534 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -6933,6 +6933,11 @@ static int selinux_perf_event_write(struct perf_event *event)
 }
 #endif
 
+static struct lsm_id selinux_lsmid __lsm_ro_after_init = {
+	.lsm  = "selinux",
+	.slot = LSMBLOB_NEEDED
+};
+
 /*
  * IMPORTANT NOTE: When adding new hooks, please be careful to keep this order:
  * 1. any hooks that don't belong to (2.) or (3.) below,
@@ -7244,7 +7249,8 @@ static __init int selinux_init(void)
 
 	hashtab_cache_init();
 
-	security_add_hooks(selinux_hooks, ARRAY_SIZE(selinux_hooks), "selinux");
+	security_add_hooks(selinux_hooks, ARRAY_SIZE(selinux_hooks),
+			   &selinux_lsmid);
 
 	if (avc_add_callback(selinux_netcache_avc_callback, AVC_CALLBACK_RESET))
 		panic("SELinux: Unable to register AVC netcache callback\n");
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index ca4a6c862732..f96be93d1a75 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -4693,6 +4693,11 @@ struct lsm_blob_sizes smack_blob_sizes __lsm_ro_after_init = {
 	.lbs_sock = sizeof(struct socket_smack),
 };
 
+static struct lsm_id smack_lsmid __lsm_ro_after_init = {
+	.lsm  = "smack",
+	.slot = LSMBLOB_NEEDED
+};
+
 static struct security_hook_list smack_hooks[] __lsm_ro_after_init = {
 	LSM_HOOK_INIT(ptrace_access_check, smack_ptrace_access_check),
 	LSM_HOOK_INIT(ptrace_traceme, smack_ptrace_traceme),
@@ -4892,7 +4897,7 @@ static __init int smack_init(void)
 	/*
 	 * Register with LSM
 	 */
-	security_add_hooks(smack_hooks, ARRAY_SIZE(smack_hooks), "smack");
+	security_add_hooks(smack_hooks, ARRAY_SIZE(smack_hooks), &smack_lsmid);
 	smack_enabled = 1;
 
 	pr_info("Smack:  Initializing.\n");
diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c
index 1f3cd432d830..22f62c67f2ec 100644
--- a/security/tomoyo/tomoyo.c
+++ b/security/tomoyo/tomoyo.c
@@ -523,6 +523,11 @@ static void tomoyo_task_free(struct task_struct *task)
 	}
 }
 
+static struct lsm_id tomoyo_lsmid __lsm_ro_after_init = {
+	.lsm  = "tomoyo",
+	.slot = LSMBLOB_NOT_NEEDED
+};
+
 /*
  * tomoyo_security_ops is a "struct security_operations" which is used for
  * registering TOMOYO.
@@ -575,7 +580,8 @@ static int __init tomoyo_init(void)
 	struct tomoyo_task *s = tomoyo_task(current);
 
 	/* register ourselves with the security framework */
-	security_add_hooks(tomoyo_hooks, ARRAY_SIZE(tomoyo_hooks), "tomoyo");
+	security_add_hooks(tomoyo_hooks, ARRAY_SIZE(tomoyo_hooks),
+			   &tomoyo_lsmid);
 	pr_info("TOMOYO Linux initialized\n");
 	s->domain_info = &tomoyo_kernel_domain;
 	atomic_inc(&tomoyo_kernel_domain.users);
diff --git a/security/yama/yama_lsm.c b/security/yama/yama_lsm.c
index 06e226166aab..a9639ea541f7 100644
--- a/security/yama/yama_lsm.c
+++ b/security/yama/yama_lsm.c
@@ -421,6 +421,11 @@ static int yama_ptrace_traceme(struct task_struct *parent)
 	return rc;
 }
 
+static struct lsm_id yama_lsmid __lsm_ro_after_init = {
+	.lsm  = "yama",
+	.slot = LSMBLOB_NOT_NEEDED
+};
+
 static struct security_hook_list yama_hooks[] __lsm_ro_after_init = {
 	LSM_HOOK_INIT(ptrace_access_check, yama_ptrace_access_check),
 	LSM_HOOK_INIT(ptrace_traceme, yama_ptrace_traceme),
@@ -477,7 +482,7 @@ static inline void yama_init_sysctl(void) { }
 static int __init yama_init(void)
 {
 	pr_info("Yama: becoming mindful.\n");
-	security_add_hooks(yama_hooks, ARRAY_SIZE(yama_hooks), "yama");
+	security_add_hooks(yama_hooks, ARRAY_SIZE(yama_hooks), &yama_lsmid);
 	yama_init_sysctl();
 	return 0;
 }
-- 
2.24.1

Re: [PATCH v23 02/23] LSM: Create and manage the lsmblob data structure.

[Mimi Zohar]

Hi Casey,

On Fri, 2020-11-20 at 12:14 -0800, Casey Schaufler wrote:
> When more than one security module is exporting data to
> audit and networking sub-systems a single 32 bit integer
> is no longer sufficient to represent the data. Add a
> structure to be used instead.
> 
> The lsmblob structure is currently an array of
> u32 "secids". There is an entry for each of the
> security modules built into the system that would
> use secids if active. The system assigns the module
> a "slot" when it registers hooks. If modules are
> compiled in but not registered there will be unused
> slots.
> 
> A new lsm_id structure, which contains the name
> of the LSM and its slot number, is created. There
> is an instance for each LSM, which assigns the name
> and passes it to the infrastructure to set the slot.
> 
> The audit rules data is expanded to use an array of
> security module data rather than a single instance.
> Because IMA uses the audit rule functions it is
> affected as well.

This patch is quite large, even without the audit rule change.  I would
limit this patch to the new lsm_id structure changes.  The audit rule
change should be broken out as a separate patch so that the audit
changes aren't hidden.

In addition, here are a few high level nits:
- The (patch description) body of the explanation, line wrapped at 75
columns, which will be copied to the permanent changelog to describe
this patch. (Refer  Documentation/process/submitting-patches.rst.)

- The brief kernel-doc descriptions should not have a trailing period. 
Nor should kernel-doc variable definitions have a trailing period. 
Example(s) inline below.  (The existing kernel-doc is mostly correct.)

- For some reason existing comments that span multiple lines aren't
formatted properly.   In those cases, where there is another change,
please fix the comment and function description.

thanks,

Mimi

> 
> Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
> Acked-by: Paul Moore <paul@paul-moore.com>
> Acked-by: John Johansen <john.johansen@canonical.com>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>

> Cc: <bpf@vger.kernel.org>
> Cc: linux-audit@redhat.com
> Cc: linux-security-module@vger.kernel.org
> Cc: selinux@vger.kernel.org
> ---

> diff --git a/include/linux/security.h b/include/linux/security.h
> index bc2725491560..fdb6e95c98e8 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -132,6 +132,65 @@ enum lockdown_reason {
> 
>  extern const char *const lockdown_reasons[LOCKDOWN_CONFIDENTIALITY_MAX+1];
> 
> +/*
> + * Data exported by the security modules
> + *
> + * Any LSM that provides secid or secctx based hooks must be included.
> + */
> +#define LSMBLOB_ENTRIES ( \
> +	(IS_ENABLED(CONFIG_SECURITY_SELINUX) ? 1 : 0) + \
> +	(IS_ENABLED(CONFIG_SECURITY_SMACK) ? 1 : 0) + \
> +	(IS_ENABLED(CONFIG_SECURITY_APPARMOR) ? 1 : 0) + \
> +	(IS_ENABLED(CONFIG_BPF_LSM) ? 1 : 0))
> +
> +struct lsmblob {
> +	u32     secid[LSMBLOB_ENTRIES];
> +};
> +
> +#define LSMBLOB_INVALID		-1	/* Not a valid LSM slot number */
> +#define LSMBLOB_NEEDED		-2	/* Slot requested on initialization */
> +#define LSMBLOB_NOT_NEEDED	-3	/* Slot not requested */
> +
> +/**
> + * lsmblob_init - initialize an lsmblob structure.

Only this kernel-doc brief description is suffixed with a period.  
Please remove.

> + * @blob: Pointer to the data to initialize
> + * @secid: The initial secid value
> + *
> + * Set all secid for all modules to the specified value.
> + */
> +static inline void lsmblob_init(struct lsmblob *blob, u32 secid)
> +{
> +	int i;
> +
> +	for (i = 0; i < LSMBLOB_ENTRIES; i++)
> +		blob->secid[i] = secid;
> +}
> +
> +/**
> + * lsmblob_is_set - report if there is an value in the lsmblob
> + * @blob: Pointer to the exported LSM data
> + *
> + * Returns true if there is a secid set, false otherwise
> + */
> +static inline bool lsmblob_is_set(struct lsmblob *blob)
> +{
> +	struct lsmblob empty = {};
> +
> +	return !!memcmp(blob, &empty, sizeof(*blob));
> +}
> +
> +/**
> + * lsmblob_equal - report if the two lsmblob's are equal
> + * @bloba: Pointer to one LSM data
> + * @blobb: Pointer to the other LSM data
> + *
> + * Returns true if all entries in the two are equal, false otherwise
> + */
> +static inline bool lsmblob_equal(struct lsmblob *bloba, struct lsmblob *blobb)
> +{
> +	return !memcmp(bloba, blobb, sizeof(*bloba));
> +}
> +
>  /* These functions are in security/commoncap.c */
>  extern int cap_capable(const struct cred *cred, struct user_namespace *ns,

> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index 9b5adeaa47fc..cd393aaa17d5 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
>  	} lsm[MAX_LSM_RULES];
> @@ -88,6 +88,22 @@ struct ima_rule_entry {
>  	struct ima_template_desc *template;
>  };
> 
> +/**
> + * ima_lsm_isset - Is a rule set for any of the active security modules
> + * @rules: The set of IMA rules to check.

Nor do kernel-doc variable definitions have a trailing period.

> + *
> + * If a rule is set for any LSM return true, otherwise return false.
> + */
> +static inline bool ima_lsm_isset(void *rules[])
> +{
> +	int i;
> +
> +	for (i = 0; i < LSMBLOB_ENTRIES; i++)
> +		if (rules[i])
> +			return true;
> +	return false;
> +}
> +
>  /*
>   * Without LSM specific knowledge, the default policy can only be
>   * written in terms of .action, .func, .mask, .fsmagic, .uid, and .fowner

Re: [PATCH v23 02/23] LSM: Create and manage the lsmblob data structure.

[Casey Schaufler]

On 12/28/2020 9:54 AM, Mimi Zohar wrote:
> Hi Casey,
>
> On Fri, 2020-11-20 at 12:14 -0800, Casey Schaufler wrote:
>> When more than one security module is exporting data to
>> audit and networking sub-systems a single 32 bit integer
>> is no longer sufficient to represent the data. Add a
>> structure to be used instead.
>>
>> The lsmblob structure is currently an array of
>> u32 "secids". There is an entry for each of the
>> security modules built into the system that would
>> use secids if active. The system assigns the module
>> a "slot" when it registers hooks. If modules are
>> compiled in but not registered there will be unused
>> slots.
>>
>> A new lsm_id structure, which contains the name
>> of the LSM and its slot number, is created. There
>> is an instance for each LSM, which assigns the name
>> and passes it to the infrastructure to set the slot.
>>
>> The audit rules data is expanded to use an array of
>> security module data rather than a single instance.
>> Because IMA uses the audit rule functions it is
>> affected as well.
> This patch is quite large, even without the audit rule change.  I would
> limit this patch to the new lsm_id structure changes.  The audit rule
> change should be broken out as a separate patch so that the audit
> changes aren't hidden.

Breaking up the patch in any meaningful way would require
scaffolding code that is as extensive and invasive as the
final change. I can do that if you really need it, but it
won't be any easier to read.

> In addition, here are a few high level nits:
> - The (patch description) body of the explanation, line wrapped at 75
> columns, which will be copied to the permanent changelog to describe
> this patch. (Refer  Documentation/process/submitting-patches.rst.)

Will fix.

> - The brief kernel-doc descriptions should not have a trailing period. 
> Nor should kernel-doc variable definitions have a trailing period. 
> Example(s) inline below.  (The existing kernel-doc is mostly correct.)

Will fix.

> - For some reason existing comments that span multiple lines aren't
> formatted properly.   In those cases, where there is another change,
> please fix the comment and function description.

Can you give an example? There are multiple comment styles
used in the various components.

> thanks,
>
> Mimi

I don't see any comments on the ima code changes. I really
don't want to spin a new patch set that does nothing but change
two periods in comments only to find out two months from now
that the code changes are completely borked. I really don't
want to go through the process of breaking up the patch that has
been widely Acked if there's no reason to expect it would require
significant work otherwise.

>
>> Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
>> Acked-by: Paul Moore <paul@paul-moore.com>
>> Acked-by: John Johansen <john.johansen@canonical.com>
>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
>> Cc: <bpf@vger.kernel.org>
>> Cc: linux-audit@redhat.com
>> Cc: linux-security-module@vger.kernel.org
>> Cc: selinux@vger.kernel.org
>> ---
>> diff --git a/include/linux/security.h b/include/linux/security.h
>> index bc2725491560..fdb6e95c98e8 100644
>> --- a/include/linux/security.h
>> +++ b/include/linux/security.h
>> @@ -132,6 +132,65 @@ enum lockdown_reason {
>>
>>  extern const char *const lockdown_reasons[LOCKDOWN_CONFIDENTIALITY_MAX+1];
>>
>> +/*
>> + * Data exported by the security modules
>> + *
>> + * Any LSM that provides secid or secctx based hooks must be included.
>> + */
>> +#define LSMBLOB_ENTRIES ( \
>> +	(IS_ENABLED(CONFIG_SECURITY_SELINUX) ? 1 : 0) + \
>> +	(IS_ENABLED(CONFIG_SECURITY_SMACK) ? 1 : 0) + \
>> +	(IS_ENABLED(CONFIG_SECURITY_APPARMOR) ? 1 : 0) + \
>> +	(IS_ENABLED(CONFIG_BPF_LSM) ? 1 : 0))
>> +
>> +struct lsmblob {
>> +	u32     secid[LSMBLOB_ENTRIES];
>> +};
>> +
>> +#define LSMBLOB_INVALID		-1	/* Not a valid LSM slot number */
>> +#define LSMBLOB_NEEDED		-2	/* Slot requested on initialization */
>> +#define LSMBLOB_NOT_NEEDED	-3	/* Slot not requested */
>> +
>> +/**
>> + * lsmblob_init - initialize an lsmblob structure.
> Only this kernel-doc brief description is suffixed with a period.  
> Please remove.
>
>> + * @blob: Pointer to the data to initialize
>> + * @secid: The initial secid value
>> + *
>> + * Set all secid for all modules to the specified value.
>> + */
>> +static inline void lsmblob_init(struct lsmblob *blob, u32 secid)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < LSMBLOB_ENTRIES; i++)
>> +		blob->secid[i] = secid;
>> +}
>> +
>> +/**
>> + * lsmblob_is_set - report if there is an value in the lsmblob
>> + * @blob: Pointer to the exported LSM data
>> + *
>> + * Returns true if there is a secid set, false otherwise
>> + */
>> +static inline bool lsmblob_is_set(struct lsmblob *blob)
>> +{
>> +	struct lsmblob empty = {};
>> +
>> +	return !!memcmp(blob, &empty, sizeof(*blob));
>> +}
>> +
>> +/**
>> + * lsmblob_equal - report if the two lsmblob's are equal
>> + * @bloba: Pointer to one LSM data
>> + * @blobb: Pointer to the other LSM data
>> + *
>> + * Returns true if all entries in the two are equal, false otherwise
>> + */
>> +static inline bool lsmblob_equal(struct lsmblob *bloba, struct lsmblob *blobb)
>> +{
>> +	return !memcmp(bloba, blobb, sizeof(*bloba));
>> +}
>> +
>>  /* These functions are in security/commoncap.c */
>>  extern int cap_capable(const struct cred *cred, struct user_namespace *ns,
>> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
>> index 9b5adeaa47fc..cd393aaa17d5 100644
>> --- a/security/integrity/ima/ima_policy.c
>> +++ b/security/integrity/ima/ima_policy.c
>>  	} lsm[MAX_LSM_RULES];
>> @@ -88,6 +88,22 @@ struct ima_rule_entry {
>>  	struct ima_template_desc *template;
>>  };
>>
>> +/**
>> + * ima_lsm_isset - Is a rule set for any of the active security modules
>> + * @rules: The set of IMA rules to check.
> Nor do kernel-doc variable definitions have a trailing period.
>
>> + *
>> + * If a rule is set for any LSM return true, otherwise return false.
>> + */
>> +static inline bool ima_lsm_isset(void *rules[])
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < LSMBLOB_ENTRIES; i++)
>> +		if (rules[i])
>> +			return true;
>> +	return false;
>> +}
>> +
>>  /*
>>   * Without LSM specific knowledge, the default policy can only be
>>   * written in terms of .action, .func, .mask, .fsmagic, .uid, and .fowner

Re: [PATCH v23 02/23] LSM: Create and manage the lsmblob data structure.

[Mimi Zohar]

On Mon, 2020-12-28 at 11:22 -0800, Casey Schaufler wrote:
> On 12/28/2020 9:54 AM, Mimi Zohar wrote:
> > Hi Casey,
> >
> > On Fri, 2020-11-20 at 12:14 -0800, Casey Schaufler wrote:
> >> When more than one security module is exporting data to
> >> audit and networking sub-systems a single 32 bit integer
> >> is no longer sufficient to represent the data. Add a
> >> structure to be used instead.
> >>
> >> The lsmblob structure is currently an array of
> >> u32 "secids". There is an entry for each of the
> >> security modules built into the system that would
> >> use secids if active. The system assigns the module
> >> a "slot" when it registers hooks. If modules are
> >> compiled in but not registered there will be unused
> >> slots.
> >>
> >> A new lsm_id structure, which contains the name
> >> of the LSM and its slot number, is created. There
> >> is an instance for each LSM, which assigns the name
> >> and passes it to the infrastructure to set the slot.
> >>
> >> The audit rules data is expanded to use an array of
> >> security module data rather than a single instance.
> >> Because IMA uses the audit rule functions it is
> >> affected as well.
> > This patch is quite large, even without the audit rule change.  I would
> > limit this patch to the new lsm_id structure changes.  The audit rule
> > change should be broken out as a separate patch so that the audit
> > changes aren't hidden.
> 
> Breaking up the patch in any meaningful way would require
> scaffolding code that is as extensive and invasive as the
> final change. I can do that if you really need it, but it
> won't be any easier to read.

Hidden in this patch is the new behavior of security_audit_rule_init(),
security_audit_rule_free(), and security_audit_rule_match().  My
concern is with label collision.  Details are in a subsequent post. 
Can an LSM prevent label collision?

> 
> > In addition, here are a few high level nits:
> > - The (patch description) body of the explanation, line wrapped at 75
> > columns, which will be copied to the permanent changelog to describe
> > this patch. (Refer  Documentation/process/submitting-patches.rst.)
> 
> Will fix.
> 
> > - The brief kernel-doc descriptions should not have a trailing period. 
> > Nor should kernel-doc variable definitions have a trailing period. 
> > Example(s) inline below.  (The existing kernel-doc is mostly correct.)
> 
> Will fix.
> 
> > - For some reason existing comments that span multiple lines aren't
> > formatted properly.   In those cases, where there is another change,
> > please fix the comment and function description.
> 
> Can you give an example? There are multiple comment styles
> used in the various components.

Never mind.  All three examples are in tomoyo.

> I don't see any comments on the ima code changes. I really
> don't want to spin a new patch set that does nothing but change
> two periods in comments only to find out two months from now
> that the code changes are completely borked. I really don't
> want to go through the process of breaking up the patch that has
> been widely Acked if there's no reason to expect it would require
> significant work otherwise.

Understood.

Re: [PATCH v23 02/23] LSM: Create and manage the lsmblob data structure.

[Mimi Zohar]

Hi Casey,

On Fri, 2020-11-20 at 12:14 -0800, Casey Schaufler wrote:
> diff --git a/security/security.c b/security/security.c
> index 5da8b3643680..d01363cb0082 100644
> --- a/security/security.c
> +++ b/security/security.c
> 
> @@ -2510,7 +2526,24 @@ int security_key_getsecurity(struct key *key, char **_buffer)
> 
>  int security_audit_rule_init(u32 field, u32 op, char *rulestr, void **lsmrule)
>  {
> -       return call_int_hook(audit_rule_init, 0, field, op, rulestr, lsmrule);
> +       struct security_hook_list *hp;
> +       bool one_is_good = false;
> +       int rc = 0;
> +       int trc;
> +
> +       hlist_for_each_entry(hp, &security_hook_heads.audit_rule_init, list) {
> +               if (WARN_ON(hp->lsmid->slot < 0 || hp->lsmid->slot >= lsm_slot))
> +                       continue;
> +               trc = hp->hook.audit_rule_init(field, op, rulestr,
> +                                              &lsmrule[hp->lsmid->slot]);
> +               if (trc == 0)
> +                       one_is_good = true;
> +               else
> +                       rc = trc;
> +       }
> +       if (one_is_good)
> +               return 0;
> +       return rc;
>  }

So the same string may be defined by multiple LSMs.
> 
>  int security_audit_rule_known(struct audit_krule *krule)
> @@ -2518,14 +2551,31 @@ int security_audit_rule_known(struct audit_krule *krule)
>         return call_int_hook(audit_rule_known, 0, krule);
>  }
> 
> -void security_audit_rule_free(void *lsmrule)
> +void security_audit_rule_free(void **lsmrule)
>  {
> -       call_void_hook(audit_rule_free, lsmrule);
> +       struct security_hook_list *hp;
> +
> +       hlist_for_each_entry(hp, &security_hook_heads.audit_rule_free, list) {
> +               if (WARN_ON(hp->lsmid->slot < 0 || hp->lsmid->slot >= lsm_slot))
> +                       continue;
> +               hp->hook.audit_rule_free(lsmrule[hp->lsmid->slot]);
> +       }
>  }
> 

If one LSM frees the string, then the string is deleted from all LSMs. 
I don't understand how this safe.

> -int security_audit_rule_match(u32 secid, u32 field, u32 op, void *lsmrule)
> +int security_audit_rule_match(u32 secid, u32 field, u32 op, void **lsmrule)
>  {
> -       return call_int_hook(audit_rule_match, 0, secid, field, op, lsmrule);
> +       struct security_hook_list *hp;
> +       int rc;
> +
> +       hlist_for_each_entry(hp, &security_hook_heads.audit_rule_match, list) {
> +               if (WARN_ON(hp->lsmid->slot < 0 || hp->lsmid->slot >= lsm_slot))
> +                       continue;
> +               rc = hp->hook.audit_rule_match(secid, field, op,
> +                                              &lsmrule[hp->lsmid->slot]);
> +               if (rc)
> +                       return rc;

Suppose that there is an IMA dont_measure or dont_appraise rule, if one
LSM matches, then this returns true, causing any measurement or
integrity verification to be skipped.  

Sample policy rules:
dont_measure obj_type=foo_log
dont_appraise obj_type=foo_log

Are there any plans to prevent label collisions or at least notify of a
label collision?

Mimi

> +       }
> +       return 0;
>  }
>  #endif /* CONFIG_AUDIT */

Re: [PATCH v23 02/23] LSM: Create and manage the lsmblob data structure.

[Casey Schaufler]

On 12/28/2020 11:24 AM, Mimi Zohar wrote:
> Hi Casey,
>
> On Fri, 2020-11-20 at 12:14 -0800, Casey Schaufler wrote:
>> diff --git a/security/security.c b/security/security.c
>> index 5da8b3643680..d01363cb0082 100644
>> --- a/security/security.c
>> +++ b/security/security.c
>>
>> @@ -2510,7 +2526,24 @@ int security_key_getsecurity(struct key *key, char **_buffer)
>>
>>  int security_audit_rule_init(u32 field, u32 op, char *rulestr, void **lsmrule)
>>  {
>> -       return call_int_hook(audit_rule_init, 0, field, op, rulestr, lsmrule);
>> +       struct security_hook_list *hp;
>> +       bool one_is_good = false;
>> +       int rc = 0;
>> +       int trc;
>> +
>> +       hlist_for_each_entry(hp, &security_hook_heads.audit_rule_init, list) {
>> +               if (WARN_ON(hp->lsmid->slot < 0 || hp->lsmid->slot >= lsm_slot))
>> +                       continue;
>> +               trc = hp->hook.audit_rule_init(field, op, rulestr,
>> +                                              &lsmrule[hp->lsmid->slot]);
>> +               if (trc == 0)
>> +                       one_is_good = true;
>> +               else
>> +                       rc = trc;
>> +       }
>> +       if (one_is_good)
>> +               return 0;
>> +       return rc;
>>  }
> So the same string may be defined by multiple LSMs.

Yes. Any legal AppArmor label would also be a legal Smack label.

>>  int security_audit_rule_known(struct audit_krule *krule)
>> @@ -2518,14 +2551,31 @@ int security_audit_rule_known(struct audit_krule *krule)
>>         return call_int_hook(audit_rule_known, 0, krule);
>>  }
>>
>> -void security_audit_rule_free(void *lsmrule)
>> +void security_audit_rule_free(void **lsmrule)
>>  {
>> -       call_void_hook(audit_rule_free, lsmrule);
>> +       struct security_hook_list *hp;
>> +
>> +       hlist_for_each_entry(hp, &security_hook_heads.audit_rule_free, list) {
>> +               if (WARN_ON(hp->lsmid->slot < 0 || hp->lsmid->slot >= lsm_slot))
>> +                       continue;
>> +               hp->hook.audit_rule_free(lsmrule[hp->lsmid->slot]);
>> +       }
>>  }
>>
> If one LSM frees the string, then the string is deleted from all LSMs. 
> I don't understand how this safe.

The audit system doesn't have a way to specify which LSM
a watched label is associated with. Even if we added one,
we'd still have to address the current behavior. Assigning
the watch to all modules means that seeing the string
in any module is sufficient to generate the event.

>
>> -int security_audit_rule_match(u32 secid, u32 field, u32 op, void *lsmrule)
>> +int security_audit_rule_match(u32 secid, u32 field, u32 op, void **lsmrule)
>>  {
>> -       return call_int_hook(audit_rule_match, 0, secid, field, op, lsmrule);
>> +       struct security_hook_list *hp;
>> +       int rc;
>> +
>> +       hlist_for_each_entry(hp, &security_hook_heads.audit_rule_match, list) {
>> +               if (WARN_ON(hp->lsmid->slot < 0 || hp->lsmid->slot >= lsm_slot))
>> +                       continue;
>> +               rc = hp->hook.audit_rule_match(secid, field, op,
>> +                                              &lsmrule[hp->lsmid->slot]);
>> +               if (rc)
>> +                       return rc;
> Suppose that there is an IMA dont_measure or dont_appraise rule, if one
> LSM matches, then this returns true, causing any measurement or
> integrity verification to be skipped.

Yes, that is correct. Like the audit system, you're doing a string based
lookup, which pretty well has to work this way. I have proposed compound
label specifications in the past, but even if we accepted something like
"apparmor=dates,selinux=figs" we'd still have to be compatible with the
old style inputs.

>
> Sample policy rules:
> dont_measure obj_type=foo_log
> dont_appraise obj_type=foo_log
>
> Are there any plans to prevent label collisions or at least notify of a
> label collision?

What would that look like? You can't say that Smack isn't allowed
to use valid AppArmor labels. How would Smack know? If the label is
valid to both, how would you decide which LSM gets to use it?

>
> Mimi
>
>> +       }
>> +       return 0;
>>  }
>>  #endif /* CONFIG_AUDIT */

Re: [PATCH v23 02/23] LSM: Create and manage the lsmblob data structure.

[Mimi Zohar]

On Mon, 2020-12-28 at 12:06 -0800, Casey Schaufler wrote:
> On 12/28/2020 11:24 AM, Mimi Zohar wrote:
> > Hi Casey,
> >
> > On Fri, 2020-11-20 at 12:14 -0800, Casey Schaufler wrote:
> >> diff --git a/security/security.c b/security/security.c
> >> index 5da8b3643680..d01363cb0082 100644
> >> --- a/security/security.c
> >> +++ b/security/security.c
> >>
> >> @@ -2510,7 +2526,24 @@ int security_key_getsecurity(struct key *key, char **_buffer)
> >>
> >>  int security_audit_rule_init(u32 field, u32 op, char *rulestr, void **lsmrule)
> >>  {
> >> -       return call_int_hook(audit_rule_init, 0, field, op, rulestr, lsmrule);
> >> +       struct security_hook_list *hp;
> >> +       bool one_is_good = false;
> >> +       int rc = 0;
> >> +       int trc;
> >> +
> >> +       hlist_for_each_entry(hp, &security_hook_heads.audit_rule_init, list) {
> >> +               if (WARN_ON(hp->lsmid->slot < 0 || hp->lsmid->slot >= lsm_slot))
> >> +                       continue;
> >> +               trc = hp->hook.audit_rule_init(field, op, rulestr,
> >> +                                              &lsmrule[hp->lsmid->slot]);
> >> +               if (trc == 0)
> >> +                       one_is_good = true;
> >> +               else
> >> +                       rc = trc;
> >> +       }
> >> +       if (one_is_good)
> >> +               return 0;
> >> +       return rc;
> >>  }
> > So the same string may be defined by multiple LSMs.
> 
> Yes. Any legal AppArmor label would also be a legal Smack label.
> 
> >>  int security_audit_rule_known(struct audit_krule *krule)
> >> @@ -2518,14 +2551,31 @@ int security_audit_rule_known(struct audit_krule *krule)
> >>         return call_int_hook(audit_rule_known, 0, krule);
> >>  }
> >>
> >> -void security_audit_rule_free(void *lsmrule)
> >> +void security_audit_rule_free(void **lsmrule)
> >>  {
> >> -       call_void_hook(audit_rule_free, lsmrule);
> >> +       struct security_hook_list *hp;
> >> +
> >> +       hlist_for_each_entry(hp, &security_hook_heads.audit_rule_free, list) {
> >> +               if (WARN_ON(hp->lsmid->slot < 0 || hp->lsmid->slot >= lsm_slot))
> >> +                       continue;
> >> +               hp->hook.audit_rule_free(lsmrule[hp->lsmid->slot]);
> >> +       }
> >>  }
> >>
> > If one LSM frees the string, then the string is deleted from all LSMs. 
> > I don't understand how this safe.
> 
> The audit system doesn't have a way to specify which LSM
> a watched label is associated with. Even if we added one,
> we'd still have to address the current behavior. Assigning
> the watch to all modules means that seeing the string
> in any module is sufficient to generate the event.

I originally thought loading a new LSM policy could not delete existing
LSM labels, but that isn't true.  If LSM labels can come and go based
on policy, with this code, could loading a new policy for one LSM
result in deleting labels of another LSM?

> 
> >
> >> -int security_audit_rule_match(u32 secid, u32 field, u32 op, void *lsmrule)
> >> +int security_audit_rule_match(u32 secid, u32 field, u32 op, void **lsmrule)
> >>  {
> >> -       return call_int_hook(audit_rule_match, 0, secid, field, op, lsmrule);
> >> +       struct security_hook_list *hp;
> >> +       int rc;
> >> +
> >> +       hlist_for_each_entry(hp, &security_hook_heads.audit_rule_match, list) {
> >> +               if (WARN_ON(hp->lsmid->slot < 0 || hp->lsmid->slot >= lsm_slot))
> >> +                       continue;
> >> +               rc = hp->hook.audit_rule_match(secid, field, op,
> >> +                                              &lsmrule[hp->lsmid->slot]);
> >> +               if (rc)
> >> +                       return rc;
> > Suppose that there is an IMA dont_measure or dont_appraise rule, if one
> > LSM matches, then this returns true, causing any measurement or
> > integrity verification to be skipped.
> 
> Yes, that is correct. Like the audit system, you're doing a string based
> lookup, which pretty well has to work this way. I have proposed compound
> label specifications in the past, but even if we accepted something like
> "apparmor=dates,selinux=figs" we'd still have to be compatible with the
> old style inputs.
> 
> >
> > Sample policy rules:
> > dont_measure obj_type=foo_log
> > dont_appraise obj_type=foo_log

IMA could extend the existing policy rules like "lsm=[selinux] |
[smack] | [apparmor]", but that assumes that the underlying
infrastructure supports it.

> >
> > Are there any plans to prevent label collisions or at least notify of a
> > label collision?
> 
> What would that look like? You can't say that Smack isn't allowed
> to use valid AppArmor labels. How would Smack know? If the label is
> valid to both, how would you decide which LSM gets to use it?

As this is a runtime issue, when loading a new policy at least flag the
collision.  When removing the label, when it is defined by multiple
LSMs, at least flag the removal.

> 
> >
> >> +       }
> >> +       return 0;
> >>  }
> >>  #endif /* CONFIG_AUDIT */
> 

Re: [PATCH v23 02/23] LSM: Create and manage the lsmblob data structure.

[Casey Schaufler]

On 12/28/2020 2:14 PM, Mimi Zohar wrote:
> On Mon, 2020-12-28 at 12:06 -0800, Casey Schaufler wrote:
>> On 12/28/2020 11:24 AM, Mimi Zohar wrote:
>>> Hi Casey,
>>>
>>> On Fri, 2020-11-20 at 12:14 -0800, Casey Schaufler wrote:
>>>> diff --git a/security/security.c b/security/security.c
>>>> index 5da8b3643680..d01363cb0082 100644
>>>> --- a/security/security.c
>>>> +++ b/security/security.c
>>>>
>>>> @@ -2510,7 +2526,24 @@ int security_key_getsecurity(struct key *key, char **_buffer)
>>>>
>>>>  int security_audit_rule_init(u32 field, u32 op, char *rulestr, void **lsmrule)
>>>>  {
>>>> -       return call_int_hook(audit_rule_init, 0, field, op, rulestr, lsmrule);
>>>> +       struct security_hook_list *hp;
>>>> +       bool one_is_good = false;
>>>> +       int rc = 0;
>>>> +       int trc;
>>>> +
>>>> +       hlist_for_each_entry(hp, &security_hook_heads.audit_rule_init, list) {
>>>> +               if (WARN_ON(hp->lsmid->slot < 0 || hp->lsmid->slot >= lsm_slot))
>>>> +                       continue;
>>>> +               trc = hp->hook.audit_rule_init(field, op, rulestr,
>>>> +                                              &lsmrule[hp->lsmid->slot]);
>>>> +               if (trc == 0)
>>>> +                       one_is_good = true;
>>>> +               else
>>>> +                       rc = trc;
>>>> +       }
>>>> +       if (one_is_good)
>>>> +               return 0;
>>>> +       return rc;
>>>>  }
>>> So the same string may be defined by multiple LSMs.
>> Yes. Any legal AppArmor label would also be a legal Smack label.
>>
>>>>  int security_audit_rule_known(struct audit_krule *krule)
>>>> @@ -2518,14 +2551,31 @@ int security_audit_rule_known(struct audit_krule *krule)
>>>>         return call_int_hook(audit_rule_known, 0, krule);
>>>>  }
>>>>
>>>> -void security_audit_rule_free(void *lsmrule)
>>>> +void security_audit_rule_free(void **lsmrule)
>>>>  {
>>>> -       call_void_hook(audit_rule_free, lsmrule);
>>>> +       struct security_hook_list *hp;
>>>> +
>>>> +       hlist_for_each_entry(hp, &security_hook_heads.audit_rule_free, list) {
>>>> +               if (WARN_ON(hp->lsmid->slot < 0 || hp->lsmid->slot >= lsm_slot))
>>>> +                       continue;
>>>> +               hp->hook.audit_rule_free(lsmrule[hp->lsmid->slot]);
>>>> +       }
>>>>  }
>>>>
>>> If one LSM frees the string, then the string is deleted from all LSMs. 
>>> I don't understand how this safe.
>> The audit system doesn't have a way to specify which LSM
>> a watched label is associated with. Even if we added one,
>> we'd still have to address the current behavior. Assigning
>> the watch to all modules means that seeing the string
>> in any module is sufficient to generate the event.
> I originally thought loading a new LSM policy could not delete existing
> LSM labels, but that isn't true.  If LSM labels can come and go based
> on policy, with this code, could loading a new policy for one LSM
> result in deleting labels of another LSM?

No. I could imagine a situation where changing policy on
a system where audit rules have been set could result in
confusion, but that would be true in the single LSM case.
It would require that secids used in the old policy be
used for different labels in the new policy. That would
not be sane behavior. I know it's impossible for Smack.

This is one of the reasons I'm switching from a single secid
to a collection of secids. You don't want unnatural behavior
of one LSM to impact the behavior of another.


>
>>>> -int security_audit_rule_match(u32 secid, u32 field, u32 op, void *lsmrule)
>>>> +int security_audit_rule_match(u32 secid, u32 field, u32 op, void **lsmrule)
>>>>  {
>>>> -       return call_int_hook(audit_rule_match, 0, secid, field, op, lsmrule);
>>>> +       struct security_hook_list *hp;
>>>> +       int rc;
>>>> +
>>>> +       hlist_for_each_entry(hp, &security_hook_heads.audit_rule_match, list) {
>>>> +               if (WARN_ON(hp->lsmid->slot < 0 || hp->lsmid->slot >= lsm_slot))
>>>> +                       continue;
>>>> +               rc = hp->hook.audit_rule_match(secid, field, op,
>>>> +                                              &lsmrule[hp->lsmid->slot]);
>>>> +               if (rc)
>>>> +                       return rc;
>>> Suppose that there is an IMA dont_measure or dont_appraise rule, if one
>>> LSM matches, then this returns true, causing any measurement or
>>> integrity verification to be skipped.
>> Yes, that is correct. Like the audit system, you're doing a string based
>> lookup, which pretty well has to work this way. I have proposed compound
>> label specifications in the past, but even if we accepted something like
>> "apparmor=dates,selinux=figs" we'd still have to be compatible with the
>> old style inputs.
>>
>>> Sample policy rules:
>>> dont_measure obj_type=foo_log
>>> dont_appraise obj_type=foo_log
> IMA could extend the existing policy rules like "lsm=[selinux] |
> [smack] | [apparmor]", but that assumes that the underlying
> infrastructure supports it.

Yes, but you would still need rational behavior in the
case where someone has old IMA policy rules.

>
>>> Are there any plans to prevent label collisions or at least notify of a
>>> label collision?
>> What would that look like? You can't say that Smack isn't allowed
>> to use valid AppArmor labels. How would Smack know? If the label is
>> valid to both, how would you decide which LSM gets to use it?
> As this is a runtime issue, when loading a new policy at least flag the
> collision.  When removing the label, when it is defined by multiple
> LSMs, at least flag the removal.

To what end would the collision be flagged? What would you do with
the information?

>
>>>> +       }
>>>> +       return 0;
>>>>  }
>>>>  #endif /* CONFIG_AUDIT */

Re: [PATCH v23 02/23] LSM: Create and manage the lsmblob data structure.

[Mimi Zohar]

On Mon, 2020-12-28 at 15:20 -0800, Casey Schaufler wrote:
> On 12/28/2020 2:14 PM, Mimi Zohar wrote:
> > On Mon, 2020-12-28 at 12:06 -0800, Casey Schaufler wrote:
> >> On 12/28/2020 11:24 AM, Mimi Zohar wrote:
> >>> Hi Casey,
> >>>
> >>> On Fri, 2020-11-20 at 12:14 -0800, Casey Schaufler wrote:
> >>>> diff --git a/security/security.c b/security/security.c
> >>>> index 5da8b3643680..d01363cb0082 100644
> >>>> --- a/security/security.c
> >>>> +++ b/security/security.c
> >>>>
> >>>> @@ -2510,7 +2526,24 @@ int security_key_getsecurity(struct key *key, char **_buffer)
> >>>>
> >>>>  int security_audit_rule_init(u32 field, u32 op, char *rulestr, void **lsmrule)
> >>>>  {
> >>>> -       return call_int_hook(audit_rule_init, 0, field, op, rulestr, lsmrule);
> >>>> +       struct security_hook_list *hp;
> >>>> +       bool one_is_good = false;
> >>>> +       int rc = 0;
> >>>> +       int trc;
> >>>> +
> >>>> +       hlist_for_each_entry(hp, &security_hook_heads.audit_rule_init, list) {
> >>>> +               if (WARN_ON(hp->lsmid->slot < 0 || hp->lsmid->slot >= lsm_slot))
> >>>> +                       continue;
> >>>> +               trc = hp->hook.audit_rule_init(field, op, rulestr,
> >>>> +                                              &lsmrule[hp->lsmid->slot]);
> >>>> +               if (trc == 0)
> >>>> +                       one_is_good = true;
> >>>> +               else
> >>>> +                       rc = trc;
> >>>> +       }
> >>>> +       if (one_is_good)
> >>>> +               return 0;
> >>>> +       return rc;
> >>>>  }
> >>> So the same string may be defined by multiple LSMs.
> >> Yes. Any legal AppArmor label would also be a legal Smack label.
> >>
> >>>>  int security_audit_rule_known(struct audit_krule *krule)
> >>>> @@ -2518,14 +2551,31 @@ int security_audit_rule_known(struct audit_krule *krule)
> >>>>         return call_int_hook(audit_rule_known, 0, krule);
> >>>>  }
> >>>>
> >>>> -void security_audit_rule_free(void *lsmrule)
> >>>> +void security_audit_rule_free(void **lsmrule)
> >>>>  {
> >>>> -       call_void_hook(audit_rule_free, lsmrule);
> >>>> +       struct security_hook_list *hp;
> >>>> +
> >>>> +       hlist_for_each_entry(hp, &security_hook_heads.audit_rule_free, list) {
> >>>> +               if (WARN_ON(hp->lsmid->slot < 0 || hp->lsmid->slot >= lsm_slot))
> >>>> +                       continue;
> >>>> +               hp->hook.audit_rule_free(lsmrule[hp->lsmid->slot]);
> >>>> +       }
> >>>>  }
> >>>>
> >>> If one LSM frees the string, then the string is deleted from all LSMs. 
> >>> I don't understand how this safe.
> >> The audit system doesn't have a way to specify which LSM
> >> a watched label is associated with. Even if we added one,
> >> we'd still have to address the current behavior. Assigning
> >> the watch to all modules means that seeing the string
> >> in any module is sufficient to generate the event.
> > I originally thought loading a new LSM policy could not delete existing
> > LSM labels, but that isn't true.  If LSM labels can come and go based
> > on policy, with this code, could loading a new policy for one LSM
> > result in deleting labels of another LSM?
> 
> No. I could imagine a situation where changing policy on
> a system where audit rules have been set could result in
> confusion, but that would be true in the single LSM case.
> It would require that secids used in the old policy be
> used for different labels in the new policy. That would
> not be sane behavior. I know it's impossible for Smack.
> 
> This is one of the reasons I'm switching from a single secid
> to a collection of secids. You don't want unnatural behavior
> of one LSM to impact the behavior of another.
> 
> 
> >
> >>>> -int security_audit_rule_match(u32 secid, u32 field, u32 op, void *lsmrule)
> >>>> +int security_audit_rule_match(u32 secid, u32 field, u32 op, void **lsmrule)
> >>>>  {
> >>>> -       return call_int_hook(audit_rule_match, 0, secid, field, op, lsmrule);
> >>>> +       struct security_hook_list *hp;
> >>>> +       int rc;
> >>>> +
> >>>> +       hlist_for_each_entry(hp, &security_hook_heads.audit_rule_match, list) {
> >>>> +               if (WARN_ON(hp->lsmid->slot < 0 || hp->lsmid->slot >= lsm_slot))
> >>>> +                       continue;
> >>>> +               rc = hp->hook.audit_rule_match(secid, field, op,
> >>>> +                                              &lsmrule[hp->lsmid->slot]);
> >>>> +               if (rc)
> >>>> +                       return rc;
> >>> Suppose that there is an IMA dont_measure or dont_appraise rule, if one
> >>> LSM matches, then this returns true, causing any measurement or
> >>> integrity verification to be skipped.
> >> Yes, that is correct. Like the audit system, you're doing a string based
> >> lookup, which pretty well has to work this way. I have proposed compound
> >> label specifications in the past, but even if we accepted something like
> >> "apparmor=dates,selinux=figs" we'd still have to be compatible with the
> >> old style inputs.
> >>
> >>> Sample policy rules:
> >>> dont_measure obj_type=foo_log
> >>> dont_appraise obj_type=foo_log
> > IMA could extend the existing policy rules like "lsm=[selinux] |
> > [smack] | [apparmor]", but that assumes that the underlying
> > infrastructure supports it.
> 
> Yes, but you would still need rational behavior in the
> case where someone has old IMA policy rules.

From an IMA perspective, allowing multiple LSMs to define the same
policy label is worse than requiring the label be constrained to a
particular LSM.

> 
> >
> >>> Are there any plans to prevent label collisions or at least notify of a
> >>> label collision?
> >> What would that look like? You can't say that Smack isn't allowed
> >> to use valid AppArmor labels. How would Smack know? If the label is
> >> valid to both, how would you decide which LSM gets to use it?

Unfortunately, unless audit supports per LSM labels, the infrastructure
needs to detect and prevent the label collision.

> > As this is a runtime issue, when loading a new policy at least flag the
> > collision.  When removing the label, when it is defined by multiple
> > LSMs, at least flag the removal.
> 
> To what end would the collision be flagged? What would you do with
> the information?

LSM label collision is probably an example of kernel integrity critical
data (yet to be upstreamed).

> 
> >
> >>>> +       }
> >>>> +       return 0;
> >>>>  }
> >>>>  #endif /* CONFIG_AUDIT */
> 

Re: [PATCH v23 02/23] LSM: Create and manage the lsmblob data structure.

[Mimi Zohar]

On Mon, 2020-12-28 at 20:53 -0500, Mimi Zohar wrote:
> On Mon, 2020-12-28 at 15:20 -0800, Casey Schaufler wrote:
> > On 12/28/2020 2:14 PM, Mimi Zohar wrote:
> > > On Mon, 2020-12-28 at 12:06 -0800, Casey Schaufler wrote:
> > >> On 12/28/2020 11:24 AM, Mimi Zohar wrote:

> > >>>> -int security_audit_rule_match(u32 secid, u32 field, u32 op, void *lsmrule)
> > >>>> +int security_audit_rule_match(u32 secid, u32 field, u32 op, void **lsmrule)
> > >>>>  {
> > >>>> -       return call_int_hook(audit_rule_match, 0, secid, field, op, lsmrule);
> > >>>> +       struct security_hook_list *hp;
> > >>>> +       int rc;
> > >>>> +
> > >>>> +       hlist_for_each_entry(hp, &security_hook_heads.audit_rule_match, list) {
> > >>>> +               if (WARN_ON(hp->lsmid->slot < 0 || hp->lsmid->slot >= lsm_slot))
> > >>>> +                       continue;
> > >>>> +               rc = hp->hook.audit_rule_match(secid, field, op,
> > >>>> +                                              &lsmrule[hp->lsmid->slot]);
> > >>>> +               if (rc)
> > >>>> +                       return rc;
> > >>> Suppose that there is an IMA dont_measure or dont_appraise rule, if one
> > >>> LSM matches, then this returns true, causing any measurement or
> > >>> integrity verification to be skipped.
> > >> Yes, that is correct. Like the audit system, you're doing a string based
> > >> lookup, which pretty well has to work this way. I have proposed compound
> > >> label specifications in the past, but even if we accepted something like
> > >> "apparmor=dates,selinux=figs" we'd still have to be compatible with the
> > >> old style inputs.
> > >>
> > >>> Sample policy rules:
> > >>> dont_measure obj_type=foo_log
> > >>> dont_appraise obj_type=foo_log
> > > IMA could extend the existing policy rules like "lsm=[selinux] |
> > > [smack] | [apparmor]", but that assumes that the underlying
> > > infrastructure supports it.
> > 
> > Yes, but you would still need rational behavior in the
> > case where someone has old IMA policy rules.
> 
> From an IMA perspective, allowing multiple LSMs to define the same
> policy label is worse than requiring the label be constrained to a
> particular LSM.

If allowing multiple LSMs to define the same label is only an IMA
issue, then have security_audit_rule_init() return the number of LSMs
which define the label.   IMA is already emitting a warning when an LSM
rule is not defined.   Emitting an additional warning would be the
first step.

In addition, ima_parse_rule() could detect policy rules containing non
LSM specific labels.  Based on policy, IMA would decide how to handle
it.

thanks,

Mimi

Re: [PATCH v23 02/23] LSM: Create and manage the lsmblob data structure.

[Casey Schaufler]

On 12/28/2020 5:53 PM, Mimi Zohar wrote:
> On Mon, 2020-12-28 at 15:20 -0800, Casey Schaufler wrote:
>> On 12/28/2020 2:14 PM, Mimi Zohar wrote:
>>> On Mon, 2020-12-28 at 12:06 -0800, Casey Schaufler wrote:
>>>> On 12/28/2020 11:24 AM, Mimi Zohar wrote:
>>>>> Hi Casey,
>>>>>
>>>>> On Fri, 2020-11-20 at 12:14 -0800, Casey Schaufler wrote:
>>>>>> diff --git a/security/security.c b/security/security.c
>>>>>> index 5da8b3643680..d01363cb0082 100644
>>>>>> --- a/security/security.c
>>>>>> +++ b/security/security.c
>>>>>>
>>>>>> @@ -2510,7 +2526,24 @@ int security_key_getsecurity(struct key *key, char **_buffer)
>>>>>>
>>>>>>  int security_audit_rule_init(u32 field, u32 op, char *rulestr, void **lsmrule)
>>>>>>  {
>>>>>> -       return call_int_hook(audit_rule_init, 0, field, op, rulestr, lsmrule);
>>>>>> +       struct security_hook_list *hp;
>>>>>> +       bool one_is_good = false;
>>>>>> +       int rc = 0;
>>>>>> +       int trc;
>>>>>> +
>>>>>> +       hlist_for_each_entry(hp, &security_hook_heads.audit_rule_init, list) {
>>>>>> +               if (WARN_ON(hp->lsmid->slot < 0 || hp->lsmid->slot >= lsm_slot))
>>>>>> +                       continue;
>>>>>> +               trc = hp->hook.audit_rule_init(field, op, rulestr,
>>>>>> +                                              &lsmrule[hp->lsmid->slot]);
>>>>>> +               if (trc == 0)
>>>>>> +                       one_is_good = true;
>>>>>> +               else
>>>>>> +                       rc = trc;
>>>>>> +       }
>>>>>> +       if (one_is_good)
>>>>>> +               return 0;
>>>>>> +       return rc;
>>>>>>  }
>>>>> So the same string may be defined by multiple LSMs.
>>>> Yes. Any legal AppArmor label would also be a legal Smack label.
>>>>
>>>>>>  int security_audit_rule_known(struct audit_krule *krule)
>>>>>> @@ -2518,14 +2551,31 @@ int security_audit_rule_known(struct audit_krule *krule)
>>>>>>         return call_int_hook(audit_rule_known, 0, krule);
>>>>>>  }
>>>>>>
>>>>>> -void security_audit_rule_free(void *lsmrule)
>>>>>> +void security_audit_rule_free(void **lsmrule)
>>>>>>  {
>>>>>> -       call_void_hook(audit_rule_free, lsmrule);
>>>>>> +       struct security_hook_list *hp;
>>>>>> +
>>>>>> +       hlist_for_each_entry(hp, &security_hook_heads.audit_rule_free, list) {
>>>>>> +               if (WARN_ON(hp->lsmid->slot < 0 || hp->lsmid->slot >= lsm_slot))
>>>>>> +                       continue;
>>>>>> +               hp->hook.audit_rule_free(lsmrule[hp->lsmid->slot]);
>>>>>> +       }
>>>>>>  }
>>>>>>
>>>>> If one LSM frees the string, then the string is deleted from all LSMs. 
>>>>> I don't understand how this safe.
>>>> The audit system doesn't have a way to specify which LSM
>>>> a watched label is associated with. Even if we added one,
>>>> we'd still have to address the current behavior. Assigning
>>>> the watch to all modules means that seeing the string
>>>> in any module is sufficient to generate the event.
>>> I originally thought loading a new LSM policy could not delete existing
>>> LSM labels, but that isn't true.  If LSM labels can come and go based
>>> on policy, with this code, could loading a new policy for one LSM
>>> result in deleting labels of another LSM?
>> No. I could imagine a situation where changing policy on
>> a system where audit rules have been set could result in
>> confusion, but that would be true in the single LSM case.
>> It would require that secids used in the old policy be
>> used for different labels in the new policy. That would
>> not be sane behavior. I know it's impossible for Smack.
>>
>> This is one of the reasons I'm switching from a single secid
>> to a collection of secids. You don't want unnatural behavior
>> of one LSM to impact the behavior of another.
>>
>>
>>>>>> -int security_audit_rule_match(u32 secid, u32 field, u32 op, void *lsmrule)
>>>>>> +int security_audit_rule_match(u32 secid, u32 field, u32 op, void **lsmrule)
>>>>>>  {
>>>>>> -       return call_int_hook(audit_rule_match, 0, secid, field, op, lsmrule);
>>>>>> +       struct security_hook_list *hp;
>>>>>> +       int rc;
>>>>>> +
>>>>>> +       hlist_for_each_entry(hp, &security_hook_heads.audit_rule_match, list) {
>>>>>> +               if (WARN_ON(hp->lsmid->slot < 0 || hp->lsmid->slot >= lsm_slot))
>>>>>> +                       continue;
>>>>>> +               rc = hp->hook.audit_rule_match(secid, field, op,
>>>>>> +                                              &lsmrule[hp->lsmid->slot]);
>>>>>> +               if (rc)
>>>>>> +                       return rc;
>>>>> Suppose that there is an IMA dont_measure or dont_appraise rule, if one
>>>>> LSM matches, then this returns true, causing any measurement or
>>>>> integrity verification to be skipped.
>>>> Yes, that is correct. Like the audit system, you're doing a string based
>>>> lookup, which pretty well has to work this way. I have proposed compound
>>>> label specifications in the past, but even if we accepted something like
>>>> "apparmor=dates,selinux=figs" we'd still have to be compatible with the
>>>> old style inputs.
>>>>
>>>>> Sample policy rules:
>>>>> dont_measure obj_type=foo_log
>>>>> dont_appraise obj_type=foo_log
>>> IMA could extend the existing policy rules like "lsm=[selinux] |
>>> [smack] | [apparmor]", but that assumes that the underlying
>>> infrastructure supports it.
>> Yes, but you would still need rational behavior in the
>> case where someone has old IMA policy rules.
> From an IMA perspective, allowing multiple LSMs to define the same
> policy label is worse than requiring the label be constrained to a
> particular LSM.

Just to be sure we're talking about the same thing,
the case I'm referring to is something like a file with
two extended attributes:

	security.apparmor MacAndCheese
	security.SMACK64 MacAndCheese

and an IMA rule that says

	dont_measure obj_type=MacAndCheese

In this case the dont_measure will be applied to both.
On the other hand,

	security.apparmor MacAndCheese
	security.SMACK64 FranksAndBeans

would also apply the rule to both, which is not
what you want. Unfortunately, there is no way to
differentiate which LSM hit the rule.

So now I'm a little confused. The case where both LSMs
use the same label looks like it works right, where the
case where they're different doesn't.

I'm beginning to think that identifying which LSMs matched
a rule (it may be none, either or both) is the right solution.
I don't think that audit is as sensitive to this.


>
>>>>> Are there any plans to prevent label collisions or at least notify of a
>>>>> label collision?
>>>> What would that look like? You can't say that Smack isn't allowed
>>>> to use valid AppArmor labels. How would Smack know? If the label is
>>>> valid to both, how would you decide which LSM gets to use it?
> Unfortunately, unless audit supports per LSM labels, the infrastructure
> needs to detect and prevent the label collision.

That would be a massive performance hit.

>
>>> As this is a runtime issue, when loading a new policy at least flag the
>>> collision.  When removing the label, when it is defined by multiple
>>> LSMs, at least flag the removal.
>> To what end would the collision be flagged? What would you do with
>> the information?
> LSM label collision is probably an example of kernel integrity critical
> data (yet to be upstreamed).
>
>>>>>> +       }
>>>>>> +       return 0;
>>>>>>  }
>>>>>>  #endif /* CONFIG_AUDIT */
>

Re: [PATCH v23 02/23] LSM: Create and manage the lsmblob data structure.

[Mimi Zohar]

On Tue, 2020-12-29 at 10:46 -0800, Casey Schaufler wrote:
> >>>>>> -int security_audit_rule_match(u32 secid, u32 field, u32 op, void *lsmrule)
> >>>>>> +int security_audit_rule_match(u32 secid, u32 field, u32 op, void **lsmrule)
> >>>>>>  {
> >>>>>> -       return call_int_hook(audit_rule_match, 0, secid, field, op, lsmrule);
> >>>>>> +       struct security_hook_list *hp;
> >>>>>> +       int rc;
> >>>>>> +
> >>>>>> +       hlist_for_each_entry(hp, &security_hook_heads.audit_rule_match, list) {
> >>>>>> +               if (WARN_ON(hp->lsmid->slot < 0 || hp->lsmid->slot >= lsm_slot))
> >>>>>> +                       continue;
> >>>>>> +               rc = hp->hook.audit_rule_match(secid, field, op,
> >>>>>> +                                              &lsmrule[hp->lsmid->slot]);
> >>>>>> +               if (rc)
> >>>>>> +                       return rc;
> >>>>> Suppose that there is an IMA dont_measure or dont_appraise rule, if one
> >>>>> LSM matches, then this returns true, causing any measurement or
> >>>>> integrity verification to be skipped.
> >>>> Yes, that is correct. Like the audit system, you're doing a string based
> >>>> lookup, which pretty well has to work this way. I have proposed compound
> >>>> label specifications in the past, but even if we accepted something like
> >>>> "apparmor=dates,selinux=figs" we'd still have to be compatible with the
> >>>> old style inputs.
> >>>>
> >>>>> Sample policy rules:
> >>>>> dont_measure obj_type=foo_log
> >>>>> dont_appraise obj_type=foo_log
> >>> IMA could extend the existing policy rules like "lsm=[selinux] |
> >>> [smack] | [apparmor]", but that assumes that the underlying
> >>> infrastructure supports it.
> >> Yes, but you would still need rational behavior in the
> >> case where someone has old IMA policy rules.
> > From an IMA perspective, allowing multiple LSMs to define the same
> > policy label is worse than requiring the label be constrained to a
> > particular LSM.
> 
> Just to be sure we're talking about the same thing,
> the case I'm referring to is something like a file with
> two extended attributes:
> 
> 	security.apparmor MacAndCheese
> 	security.SMACK64 MacAndCheese
> 
> and an IMA rule that says
> 
> 	dont_measure obj_type=MacAndCheese
> 
> In this case the dont_measure will be applied to both.
> On the other hand,
> 
> 	security.apparmor MacAndCheese
> 	security.SMACK64 FranksAndBeans
> 
> would also apply the rule to both, which is not
> what you want. Unfortunately, there is no way to
> differentiate which LSM hit the rule.
> 
> So now I'm a little confused. The case where both LSMs
> use the same label looks like it works right, where the
> case where they're different doesn't.

I'm more concerned about multiple LSMs using the same label.  The
label's meaning is LSM specific.

> 
> I'm beginning to think that identifying which LSMs matched
> a rule (it may be none, either or both) is the right solution.
> I don't think that audit is as sensitive to this.

If the label's meaning is LSM specific, then the rule needs to be LSM
specific.