On Mon, 2025-11-10 at 11:06 -0800, Tahera Fahimi wrote:
> 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.
Why would an administrator attempt to load the same rule twice?
How likely is the case where one would like to combine multiple signed
IMA policies with common rules?
Unless there is a realistic use case, it would be better to patch user
space first.
Thanks
Roberto
> 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
