diff mbox series

[bpf-next,v4,10/20] lsm: Refactor return value of LSM hook audit_rule_match

Message ID 20240711111908.3817636-11-xukuohai@huaweicloud.com (mailing list archive)
State Changes Requested
Delegated to: Paul Moore
Headers show
Series Add return value range check for BPF LSM | expand

Commit Message

Xu Kuohai July 11, 2024, 11:18 a.m. UTC 
From: Xu Kuohai <xukuohai@huawei.com>

To be consistent with most LSM hooks, convert the return value of
hook audit_rule_match to 0 or a negative error code.

Before:
- Hook audit_rule_match returns 1 if the rule matches, 0 if it not,
  and negative error code otherwise.

After:
- Hook audit_rule_match returns 0 on success or a negative error
  code on failure. An output parameter @match is introduced to hold
  the match result on success.

Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
---
 include/linux/lsm_hook_defs.h     |  3 +-
 security/apparmor/audit.c         | 22 ++++++-------
 security/apparmor/include/audit.h |  2 +-
 security/security.c               | 15 ++++++++-
 security/selinux/include/audit.h  |  8 +++--
 security/selinux/ss/services.c    | 54 +++++++++++++++++--------------
 security/smack/smack_lsm.c        | 19 +++++++----
 7 files changed, 75 insertions(+), 48 deletions(-)

Comments

Paul Moore July 19, 2024, 2:08 a.m. UTC | #1 
On Jul 11, 2024 Xu Kuohai <xukuohai@huaweicloud.com> wrote:
> 
> To be consistent with most LSM hooks, convert the return value of
> hook audit_rule_match to 0 or a negative error code.
> 
> Before:
> - Hook audit_rule_match returns 1 if the rule matches, 0 if it not,
>   and negative error code otherwise.
> 
> After:
> - Hook audit_rule_match returns 0 on success or a negative error
>   code on failure. An output parameter @match is introduced to hold
>   the match result on success.
> 
> Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
> ---
>  include/linux/lsm_hook_defs.h     |  3 +-
>  security/apparmor/audit.c         | 22 ++++++-------
>  security/apparmor/include/audit.h |  2 +-
>  security/security.c               | 15 ++++++++-
>  security/selinux/include/audit.h  |  8 +++--
>  security/selinux/ss/services.c    | 54 +++++++++++++++++--------------
>  security/smack/smack_lsm.c        | 19 +++++++----
>  7 files changed, 75 insertions(+), 48 deletions(-)

This is another odd hook, and similar to some of the others in this
patchset, I'm not sure how applicable this would be to a BPF-based
LSM.  I suspect you could safely block this from a BPF LSM and no one
would notice or be upset.

However, if we did want to keep this hook for a BPF LSM, I think it
might be better to encode the "match" results in the return value, just
sticking with a more conventional 0/errno approach.  What do you think
about 0:found/ok, -ENOENT:missing/ok, -ERRNO:other/error?  Yes, some
of the existing LSM audit_match code uses -ENOENT but looking quickly
at those error conditions it seems that we could consider them
equivalent to a "missing" or "failed match" result and use -ENOENT for
both.  If you're really not happy with that overloading, we could use
something like -ENOMSG:missing/ok instead.

Thoughts?

--
paul-moore.com
Xu Kuohai July 20, 2024, 9:31 a.m. UTC | #2 
On 7/19/2024 10:08 AM, Paul Moore wrote:
> On Jul 11, 2024 Xu Kuohai <xukuohai@huaweicloud.com> wrote:
>>
>> To be consistent with most LSM hooks, convert the return value of
>> hook audit_rule_match to 0 or a negative error code.
>>
>> Before:
>> - Hook audit_rule_match returns 1 if the rule matches, 0 if it not,
>>    and negative error code otherwise.
>>
>> After:
>> - Hook audit_rule_match returns 0 on success or a negative error
>>    code on failure. An output parameter @match is introduced to hold
>>    the match result on success.
>>
>> Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
>> ---
>>   include/linux/lsm_hook_defs.h     |  3 +-
>>   security/apparmor/audit.c         | 22 ++++++-------
>>   security/apparmor/include/audit.h |  2 +-
>>   security/security.c               | 15 ++++++++-
>>   security/selinux/include/audit.h  |  8 +++--
>>   security/selinux/ss/services.c    | 54 +++++++++++++++++--------------
>>   security/smack/smack_lsm.c        | 19 +++++++----
>>   7 files changed, 75 insertions(+), 48 deletions(-)
> 
> This is another odd hook, and similar to some of the others in this
> patchset, I'm not sure how applicable this would be to a BPF-based
> LSM.  I suspect you could safely block this from a BPF LSM and no one
> would notice or be upset.
> 
> However, if we did want to keep this hook for a BPF LSM, I think it
> might be better to encode the "match" results in the return value, just
> sticking with a more conventional 0/errno approach.  What do you think
> about 0:found/ok, -ENOENT:missing/ok, -ERRNO:other/error?  Yes, some
> of the existing LSM audit_match code uses -ENOENT but looking quickly
> at those error conditions it seems that we could consider them
> equivalent to a "missing" or "failed match" result and use -ENOENT for
> both.  If you're really not happy with that overloading, we could use
> something like -ENOMSG:missing/ok instead.
> 
> Thoughts?
>

I think we could just block it and see what happens.

> --
> paul-moore.com
>
diff mbox series

Patch

diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 54fec360947c..6b521744a23b 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -418,7 +418,8 @@  LSM_HOOK(void, LSM_RET_VOID, key_post_create_or_update, struct key *keyring,
 LSM_HOOK(int, 0, audit_rule_init, u32 field, u32 op, char *rulestr,
 	 void **lsmrule, gfp_t gfp)
 LSM_HOOK(int, 0, audit_rule_known, struct audit_krule *krule)
-LSM_HOOK(int, 0, audit_rule_match, u32 secid, u32 field, u32 op, void *lsmrule)
+LSM_HOOK(int, 0, audit_rule_match, u32 secid, u32 field, u32 op, void *lsmrule,
+	 bool *match)
 LSM_HOOK(void, LSM_RET_VOID, audit_rule_free, void *lsmrule)
 #endif /* CONFIG_AUDIT */
 
diff --git a/security/apparmor/audit.c b/security/apparmor/audit.c
index 6b5181c668b5..352a183b3325 100644
--- a/security/apparmor/audit.c
+++ b/security/apparmor/audit.c
@@ -264,11 +264,11 @@  int aa_audit_rule_known(struct audit_krule *rule)
 	return 0;
 }
 
-int aa_audit_rule_match(u32 sid, u32 field, u32 op, void *vrule)
+int aa_audit_rule_match(u32 sid, u32 field, u32 op, void *vrule, bool *match)
 {
 	struct aa_audit_rule *rule = vrule;
 	struct aa_label *label;
-	int found = 0;
+	bool found = false;
 
 	label = aa_secid_to_label(sid);
 
@@ -276,16 +276,14 @@  int aa_audit_rule_match(u32 sid, u32 field, u32 op, void *vrule)
 		return -ENOENT;
 
 	if (aa_label_is_subset(label, rule->label))
-		found = 1;
+		found = true;
+
+	if (field == AUDIT_SUBJ_ROLE && op == Audit_equal)
+		*match = found;
+	else if (field == AUDIT_SUBJ_ROLE && op == Audit_not_equal)
+		*match = !found;
+	else
+		*match = false;
 
-	switch (field) {
-	case AUDIT_SUBJ_ROLE:
-		switch (op) {
-		case Audit_equal:
-			return found;
-		case Audit_not_equal:
-			return !found;
-		}
-	}
 	return 0;
 }
diff --git a/security/apparmor/include/audit.h b/security/apparmor/include/audit.h
index 0c8cc86b417b..a227741f33c8 100644
--- a/security/apparmor/include/audit.h
+++ b/security/apparmor/include/audit.h
@@ -202,6 +202,6 @@  static inline int complain_error(int error)
 void aa_audit_rule_free(void *vrule);
 int aa_audit_rule_init(u32 field, u32 op, char *rulestr, void **vrule, gfp_t gfp);
 int aa_audit_rule_known(struct audit_krule *rule);
-int aa_audit_rule_match(u32 sid, u32 field, u32 op, void *vrule);
+int aa_audit_rule_match(u32 sid, u32 field, u32 op, void *vrule, bool *match);
 
 #endif /* __AA_AUDIT_H */
diff --git a/security/security.c b/security/security.c
index 2c161101074d..5e9de8d0cdde 100644
--- a/security/security.c
+++ b/security/security.c
@@ -5450,7 +5450,20 @@  void security_audit_rule_free(void *lsmrule)
  */
 int security_audit_rule_match(u32 secid, u32 field, u32 op, void *lsmrule)
 {
-	return call_int_hook(audit_rule_match, secid, field, op, lsmrule);
+	int rc;
+	bool match = false;
+	struct security_hook_list *hp;
+
+	hlist_for_each_entry(hp, &security_hook_heads.audit_rule_match, list) {
+		rc = hp->hook.audit_rule_match(secid, field, op, lsmrule,
+					       &match);
+		if (rc < 0)
+			return rc;
+		if (match)
+			break;
+	}
+
+	return match;
 }
 #endif /* CONFIG_AUDIT */
 
diff --git a/security/selinux/include/audit.h b/security/selinux/include/audit.h
index 29c7d4c86f6d..2d0799270426 100644
--- a/security/selinux/include/audit.h
+++ b/security/selinux/include/audit.h
@@ -45,11 +45,13 @@  void selinux_audit_rule_free(void *rule);
  *	@field: the field this rule refers to
  *	@op: the operator the rule uses
  *	@rule: pointer to the audit rule to check against
+ *	@match: if the context id matches the rule
  *
- *	Returns 1 if the context id matches the rule, 0 if it does not, and
- *	-errno on failure.
+ *	Returns 0 on success and -errno on failure. @match holds the match
+ *	result.
  */
-int selinux_audit_rule_match(u32 sid, u32 field, u32 op, void *rule);
+int selinux_audit_rule_match(u32 sid, u32 field, u32 op, void *rule,
+			     bool *match);
 
 /**
  *	selinux_audit_rule_known - check to see if rule contains selinux fields.
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index e33e55384b75..2946d28a25b1 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -3633,29 +3633,32 @@  int selinux_audit_rule_known(struct audit_krule *rule)
 	return 0;
 }
 
-int selinux_audit_rule_match(u32 sid, u32 field, u32 op, void *vrule)
+int selinux_audit_rule_match(u32 sid, u32 field, u32 op, void *vrule,
+			     bool *match)
 {
 	struct selinux_state *state = &selinux_state;
 	struct selinux_policy *policy;
 	struct context *ctxt;
 	struct mls_level *level;
 	struct selinux_audit_rule *rule = vrule;
-	int match = 0;
+	int rc = 0;
 
 	if (unlikely(!rule)) {
 		WARN_ONCE(1, "selinux_audit_rule_match: missing rule\n");
 		return -ENOENT;
 	}
 
-	if (!selinux_initialized())
+	if (!selinux_initialized()) {
+		*match = false;
 		return 0;
+	}
 
 	rcu_read_lock();
 
 	policy = rcu_dereference(state->policy);
 
 	if (rule->au_seqno < policy->latest_granting) {
-		match = -ESTALE;
+		rc = -ESTALE;
 		goto out;
 	}
 
@@ -3663,7 +3666,7 @@  int selinux_audit_rule_match(u32 sid, u32 field, u32 op, void *vrule)
 	if (unlikely(!ctxt)) {
 		WARN_ONCE(1, "selinux_audit_rule_match: unrecognized SID %d\n",
 			  sid);
-		match = -ENOENT;
+		rc = -ENOENT;
 		goto out;
 	}
 
@@ -3674,10 +3677,10 @@  int selinux_audit_rule_match(u32 sid, u32 field, u32 op, void *vrule)
 	case AUDIT_OBJ_USER:
 		switch (op) {
 		case Audit_equal:
-			match = (ctxt->user == rule->au_ctxt.user);
+			rc = (ctxt->user == rule->au_ctxt.user);
 			break;
 		case Audit_not_equal:
-			match = (ctxt->user != rule->au_ctxt.user);
+			rc = (ctxt->user != rule->au_ctxt.user);
 			break;
 		}
 		break;
@@ -3685,10 +3688,10 @@  int selinux_audit_rule_match(u32 sid, u32 field, u32 op, void *vrule)
 	case AUDIT_OBJ_ROLE:
 		switch (op) {
 		case Audit_equal:
-			match = (ctxt->role == rule->au_ctxt.role);
+			rc = (ctxt->role == rule->au_ctxt.role);
 			break;
 		case Audit_not_equal:
-			match = (ctxt->role != rule->au_ctxt.role);
+			rc = (ctxt->role != rule->au_ctxt.role);
 			break;
 		}
 		break;
@@ -3696,10 +3699,10 @@  int selinux_audit_rule_match(u32 sid, u32 field, u32 op, void *vrule)
 	case AUDIT_OBJ_TYPE:
 		switch (op) {
 		case Audit_equal:
-			match = (ctxt->type == rule->au_ctxt.type);
+			rc = (ctxt->type == rule->au_ctxt.type);
 			break;
 		case Audit_not_equal:
-			match = (ctxt->type != rule->au_ctxt.type);
+			rc = (ctxt->type != rule->au_ctxt.type);
 			break;
 		}
 		break;
@@ -3712,39 +3715,42 @@  int selinux_audit_rule_match(u32 sid, u32 field, u32 op, void *vrule)
 			 &ctxt->range.level[0] : &ctxt->range.level[1]);
 		switch (op) {
 		case Audit_equal:
-			match = mls_level_eq(&rule->au_ctxt.range.level[0],
-					     level);
+			rc = mls_level_eq(&rule->au_ctxt.range.level[0],
+					  level);
 			break;
 		case Audit_not_equal:
-			match = !mls_level_eq(&rule->au_ctxt.range.level[0],
-					      level);
+			rc = !mls_level_eq(&rule->au_ctxt.range.level[0],
+					   level);
 			break;
 		case Audit_lt:
-			match = (mls_level_dom(&rule->au_ctxt.range.level[0],
-					       level) &&
+			rc = (mls_level_dom(&rule->au_ctxt.range.level[0],
+					    level) &&
 				 !mls_level_eq(&rule->au_ctxt.range.level[0],
 					       level));
 			break;
 		case Audit_le:
-			match = mls_level_dom(&rule->au_ctxt.range.level[0],
-					      level);
+			rc = mls_level_dom(&rule->au_ctxt.range.level[0],
+					   level);
 			break;
 		case Audit_gt:
-			match = (mls_level_dom(level,
-					      &rule->au_ctxt.range.level[0]) &&
+			rc = (mls_level_dom(level,
+					    &rule->au_ctxt.range.level[0]) &&
 				 !mls_level_eq(level,
 					       &rule->au_ctxt.range.level[0]));
 			break;
 		case Audit_ge:
-			match = mls_level_dom(level,
-					      &rule->au_ctxt.range.level[0]);
+			rc = mls_level_dom(level,
+					   &rule->au_ctxt.range.level[0]);
 			break;
 		}
 	}
 
 out:
 	rcu_read_unlock();
-	return match;
+	if (rc < 0)
+		return rc;
+	*match = !!rc;
+	return 0;
 }
 
 static int aurule_avc_callback(u32 event)
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 9a121ad53b16..ea0f0cf11ff3 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -4764,11 +4764,15 @@  static int smack_audit_rule_known(struct audit_krule *krule)
  * @field: audit rule flags given from user-space
  * @op: required testing operator
  * @vrule: smack internal rule presentation
+ * @match: the match result
  *
  * The core Audit hook. It's used to take the decision of
  * whether to audit or not to audit a given object.
+ *
+ * Returns 0 on success or negative error code on failure.
  */
-static int smack_audit_rule_match(u32 secid, u32 field, u32 op, void *vrule)
+static int smack_audit_rule_match(u32 secid, u32 field, u32 op, void *vrule,
+				  bool *match)
 {
 	struct smack_known *skp;
 	char *rule = vrule;
@@ -4778,8 +4782,10 @@  static int smack_audit_rule_match(u32 secid, u32 field, u32 op, void *vrule)
 		return -ENOENT;
 	}
 
-	if (field != AUDIT_SUBJ_USER && field != AUDIT_OBJ_USER)
+	if (field != AUDIT_SUBJ_USER && field != AUDIT_OBJ_USER) {
+		*match = false;
 		return 0;
+	}
 
 	skp = smack_from_secid(secid);
 
@@ -4789,10 +4795,11 @@  static int smack_audit_rule_match(u32 secid, u32 field, u32 op, void *vrule)
 	 * label.
 	 */
 	if (op == Audit_equal)
-		return (rule == skp->smk_known);
-	if (op == Audit_not_equal)
-		return (rule != skp->smk_known);
-
+		*match = (rule == skp->smk_known);
+	else if (op == Audit_not_equal)
+		*match = (rule != skp->smk_known);
+	else
+		*match = false;
 	return 0;
 }