On 11/6/2025 12:32 PM, Anirudh Venkataramanan wrote:
> 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.
Hi Ahirudh, Thanks for your feedback.
>>
>> Signed-off-by: Tahera Fahimi <[email protected]>
>> ---
>> 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.
The declaration of j is at the beginning to adhere proper kernel style and
ancient compile support.
>> +
>> + 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?
The purpose of this patch is to prohibit two exact same rule.
We can have other approaches like merging the new rule to the previously
existing rule, ignore
new rule if a similar rule exists. However, this approaches would add more
complexity to the code
and are not the purpose of this patch.
> 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:
> ...
> ...
I don't think having two goto in the code will improve its readability.
>> +
>> + 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;
>> }
I
