Re: [Patch V1] ima: avoid duplicate policy rules insertions

[Anirudh Venkataramanan <anirudhve@linux.microsoft.com>]

On 11/6/2025 10:14 AM, Tahera Fahimi wrote:
> Prevent redundant IMA policy rules by checking for duplicates before insertion. This ensures that
> rules are not re-added when userspace is restarted (using systemd-soft-reboot) without a full system
> reboot. ima_rule_exists() detects duplicates in both temporary and active rule lists.

I have run into this too. Thanks for proposing a patch!

FWIW - I am fairly new to the IMA subsystem, so feedback below is mostly 
structural, with some IMA specific comments.

> 
> Signed-off-by: Tahera Fahimi <taherafahimi@linux.microsoft.com>
> ---
>   security/integrity/ima/ima_policy.c | 157 +++++++++++++++++++++++++++-
>   1 file changed, 156 insertions(+), 1 deletion(-)
> 
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index 164d62832f8ec..3dd902101dbda 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -1953,6 +1953,153 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
>   	return result;
>   }
>   
> +static bool template_has_field(const char *field_id, const struct ima_template_desc *template2)
> +{
> +	int j;

j is declared in the loop header below too, which is more correct 
because it keeps the scope of j to be within the loop. So I'd say get 
rid of the above declaration.

> +
> +	for (int j = 0; j < template2->num_fields; j++)
> +		if (strcmp(field_id, template2->fields[j]->field_id) == 0)
> +			return true;
I believe the preferred kernel style is to use if (!strcmp(...)).

> +
> +	return false;
> +}
> +
> +static bool keyring_has_item(const char *item, const struct ima_rule_opt_list *keyrings)
> +{
> +	int j;
> +
> +	for (j = 0; j < keyrings->count; j++) {
> +		if (strcmp(item, keyrings->items[j]) == 0)
> +			return true;
> +	}
> +	return false;
> +}
> +
> +static bool labels_has_item(const char *item, const struct ima_rule_opt_list *labels)
> +{
> +	int j;
> +
> +	for (j = 0; j < labels->count; j++) {
> +		if (strcmp(item, labels->items[j]) == 0)
> +			return true;
> +	}
> +	return false;
> +}
> +
> +static bool ima_rules_equal(const struct ima_rule_entry *rule1, const struct ima_rule_entry *rule2)
> +{
> +	int i;

i is used further down in this function, and even in all those cases, 
the scope of i can be limited to the loop body where it's used.

If you didn't know this already - you can use cppcheck to identify and 
reduce the scope of variables.

> +
> +	if (rule1->flags != rule2->flags)
> +		return false;
> +
> +	if (rule1->action != rule2->action)
> +		return false;
> +
> +	if (((rule1->flags & IMA_FUNC) && rule1->func != rule2->func) ||
> +	    ((rule1->flags & (IMA_MASK | IMA_INMASK)) && rule1->mask != rule2->mask) ||
> +	    ((rule1->flags & IMA_FSMAGIC) && rule1->fsmagic != rule2->fsmagic) ||
> +	    ((rule1->flags & IMA_FSUUID) && !uuid_equal(&rule1->fsuuid, &rule2->fsuuid)) ||
> +	    ((rule1->flags & IMA_UID) && !uid_eq(rule1->uid, rule2->uid)) ||
> +	    ((rule1->flags & IMA_GID) && !gid_eq(rule1->gid, rule2->gid)) ||
> +	    ((rule1->flags & IMA_FOWNER) && !uid_eq(rule1->fowner, rule2->fowner)) ||
> +	    ((rule1->flags & IMA_FGROUP) && !gid_eq(rule1->fgroup, rule2->fgroup)) ||
> +	    ((rule1->flags & IMA_FSNAME) && (strcmp(rule1->fsname, rule2->fsname) != 0)) ||
> +	    ((rule1->flags & IMA_PCR) && rule1->pcr != rule2->pcr) ||
> +	    ((rule1->flags & IMA_VALIDATE_ALGOS) &&
> +	      rule1->allowed_algos != rule2->allowed_algos) ||
> +	    ((rule1->flags & IMA_EUID) && !uid_eq(rule1->uid, rule2->uid)) ||
> +	    ((rule1->flags & IMA_EGID) && !gid_eq(rule1->gid, rule2->gid)))
> +		return false;

So the goal is to prevent the exact same policy rule from being added, 
not to update an existing rule, correct? IOW, you could end up with two 
very similar rules, because the new rule has one thing that's different 
compared to the existing rule?

I feel that a little bit of commentary around what makes two rules the 
same would be useful.

> +
> +	if (!rule1->template && !rule2->template) {
> +		;
You're trying to do nothing and continue on. A goto statement would 
communicate intent better. There are other places below with the same 
noop structure.

To be fair, I also don't completely understand what you're trying to 
achieve here, Regardless, this "do nothing inside a conditional" looks 
weird and I feel like there should be a way to structure your logic 
without resorting to this.

> +	} else if (!rule1->template || !rule2->template) {
> +		return false;
> +	} else if (rule1->template->num_fields != rule2->template->num_fields) {
> +		return false;
> +	} else if (rule1->template->num_fields != 0) {
> +		for (i = 0; i < rule1->template->num_fields; i++) {
> +			if (!template_has_field(rule1->template->fields[i]->field_id,
> +						rule2->template))
> +				return false;
> +		}
> +	}

if + return will achieve the same end goals as else if + return, with 
lesser clutter. I have seen some static analyzers flag this pattern, but 
I can't remember which one at the moment.

So something like this:

if (!rule1->template && !rule2->template)
     goto some_target;

if (!rule1->template || !rule2->template)
     return false;

if (rule1->template->num_fields != rule2->template->num_fields)
     return false;

if (rule1->template->num_fields != 0) {
     for (i = 0; i < rule1->template->num_fields; i++) {
         if (!template_has_field(rule1->template->fields[i]->field_id,
                                 rule2->template))
               return false;
         }
}

some_target:
...
...

> +
> +	if (rule1->flags & IMA_KEYRINGS) {
> +		if (!rule1->keyrings && !rule2->keyrings) {
> +			;

Another if block no-op

> +		} else if (!rule1->keyrings || !rule2->keyrings) {
> +			return false;
> +		} else if (rule1->keyrings->count != rule2->keyrings->count) {
> +			return false;
> +		} else if (rule1->keyrings->count != 0) {

if (rule1->keyrings->count)

> +			for (i = 0; i < rule1->keyrings->count; i++) {

for (int i,

> +				if (!keyring_has_item(rule1->keyrings->items[i], rule2->keyrings))
> +					return false;
> +			}
> +		}
> +	}
> +
> +	if (rule1->flags & IMA_LABEL) {
> +		if (!rule1->label && !rule2->label) {
> +			;

Another if block no-op

> +		} else if (!rule1->label || !rule2->label) {
> +			return false;
> +		} else if (rule1->label->count != rule2->label->count) {
> +			return false;
> +		} else if (rule1->label->count != 0) {
> +			for (i = 0; i < rule1->label->count; i++) {
> +				if (!labels_has_item(rule1->label->items[i], rule2->label))
> +					return false;
> +			}
> +		}
> +	}
> +
> +	for (i = 0; i < MAX_LSM_RULES; i++) {

for (int i,

> +		if (!rule1->lsm[i].rule && !rule2->lsm[i].rule)
> +			continue;
> +
> +		if (!rule1->lsm[i].rule || !rule2->lsm[i].rule)
> +			return false;
> +
> +		if (strcmp(rule1->lsm[i].args_p, rule2->lsm[i].args_p) != 0)
> +			return false;
> +	}
> +
> +	return true;
> +}
> +
> +/**
> + * ima_rule_exists - check if a rule already exists in the policy
> + *
> + * Checking both the active policy and the temporary rules list.
> + */
> +static bool ima_rule_exists(struct ima_rule_entry *new_rule)
> +{
> +	struct ima_rule_entry *entry;
> +	struct list_head *ima_rules_tmp;
> +
> +	if (!list_empty(&ima_temp_rules)) {
> +		list_for_each_entry(entry, &ima_temp_rules, list) {
> +			if (ima_rules_equal(entry, new_rule))
> +				return true;
> +		}
> +	}
> +
> +	rcu_read_lock();
> +	ima_rules_tmp = rcu_dereference(ima_rules);
> +	list_for_each_entry_rcu(entry, ima_rules_tmp, list) {
> +		if (ima_rules_equal(entry, new_rule)) {
> +			rcu_read_unlock();
> +			return true;
> +		}
> +	}
> +	rcu_read_unlock();
> +
> +	return false;
> +}
> +
>   /**
>    * ima_parse_add_rule - add a rule to ima_policy_rules
>    * @rule: ima measurement policy rule
> @@ -1993,7 +2140,15 @@ ssize_t ima_parse_add_rule(char *rule)
>   		return result;
>   	}
>   
> -	list_add_tail(&entry->list, &ima_temp_rules);
> +	if (!ima_rule_exists(entry)) {
> +		list_add_tail(&entry->list, &ima_temp_rules);
> +	} else {
> +		result = -EEXIST;
Is it necessary to set result? Or can you just pass -EEXIST to the audit 
call below?

> +		ima_free_rule(entry);
> +		integrity_audit_msg(AUDIT_INTEGRITY_STATUS, NULL,
> +				    NULL, op, "duplicate-policy", result,
> +				    audit_info);
> +	}
>   
>   	return len;
>   }