On 2022/8/8 11:02, Guozihua (Scott) wrote:
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?
Including SELinux in the loop.
--
Best
GUO Zihua
