Race conditioned discovered between ima_match_rules and ima_update_lsm_update_rules

["Guozihua (Scott)" <guozihua@huawei.com>]

Hi Community,

Recently we discovered a race condition while updating SELinux policy 
with IMA lsm rule enabled. Which would lead to extra files being measured.

While SELinux policy is updated, the IDs for object types and such would 
be changed, and ima_lsm_update_rules would be called.

There are no lock applied in ima_lsm_update_rules. If user accesses a 
file during this time, ima_match_rules will be matching rules based on 
old SELinux au_seqno resulting in selinux_audit_rule_match returning 
-ESTALE.

However, in ima_match_rules, this error number is not handled, causing 
IMA to think the LSM rule is also a match, leading to measuring extra files.

Relevant codes are as follows:

> static void ima_lsm_update_rules(void)
> {
> 	struct ima_rule_entry *entry, *e;
> 	int result;
> 
> 	list_for_each_entry_safe(entry, e, &ima_policy_rules, list) {
> 		if (!ima_rule_contains_lsm_cond(entry))
> 			continue;
> 
> 		result = ima_lsm_update_rule(entry);

A RCU style update is used with no lock applied. Reading to rules would 
return rules with staled au_seqno.

> 		if (result) {
> 			pr_err("lsm rule update error %d\n", result);
> 			return;
> 		}
> 	}
> }


> int selinux_audit_rule_match(u32 sid, u32 field, u32 op, void *vrule)
> {
> 	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;
> 
> 	if (unlikely(!rule)) {
> 		WARN_ONCE(1, "selinux_audit_rule_match: missing rule\n");
> 		return -ENOENT;
> 	}
> 
> 	if (!selinux_initialized(state))
> 		return 0;
> 
> 	rcu_read_lock();
> 
> 	policy = rcu_dereference(state->policy);
> 
> 	if (rule->au_seqno < policy->latest_granting) {
> 		match = -ESTALE;
> 		goto out;
> 	}

SELinux would return -ESTALE here.

> static bool ima_match_rules(struct ima_rule_entry *rule,
> 			    struct user_namespace *mnt_userns,
> 			    struct inode *inode, const struct cred *cred,
> 			    u32 secid, enum ima_hooks func, int mask,
> 			    const char *func_data)
> {
> ...
> 
> 	for (i = 0; i < MAX_LSM_RULES; i++) {
> 		int rc = 0;
> 		u32 osid;
> 
> 		if (!rule->lsm[i].rule) {
> 			if (!rule->lsm[i].args_p)
> 				continue;
> 			else
> 				return false;
> 		}
> 		switch (i) {
> 		case LSM_OBJ_USER:
> 		case LSM_OBJ_ROLE:
> 		case LSM_OBJ_TYPE:
> 			security_inode_getsecid(inode, &osid);
> 			rc = ima_filter_rule_match(osid, rule->lsm[i].type,
> 						   Audit_equal,
> 						   rule->lsm[i].rule);

rc here will be -ESTALE.

> 			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);
> 			break;
> 		default:
> 			break;
> 		}
> 		if (!rc)
> 			return false;

-ERRNO is not handled, this func will return true.

> 	}
> 	return true;
> }

It seems that IMA would not "leak" any files, but widening the 
measurement range.

Is this the intended behavior? Or is it a good idea to add a lock for 
LSM rules during update?

-- 
Best
GUO Zihua