[Patch V1] ima: avoid duplicate policy rules insertions

[Patch V1] ima: avoid duplicate policy rules insertions

[Tahera Fahimi]

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.

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;
+
+	for (int j = 0; j < template2->num_fields; j++)
+		if (strcmp(field_id, template2->fields[j]->field_id) == 0)
+			return true;
+
+	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;
+
+	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;
+
+	if (!rule1->template && !rule2->template) {
+		;
+	} 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 (rule1->flags & IMA_KEYRINGS) {
+		if (!rule1->keyrings && !rule2->keyrings) {
+			;
+		} else if (!rule1->keyrings || !rule2->keyrings) {
+			return false;
+		} else if (rule1->keyrings->count != rule2->keyrings->count) {
+			return false;
+		} else if (rule1->keyrings->count != 0) {
+			for (i = 0; i < rule1->keyrings->count; i++) {
+				if (!keyring_has_item(rule1->keyrings->items[i], rule2->keyrings))
+					return false;
+			}
+		}
+	}
+
+	if (rule1->flags & IMA_LABEL) {
+		if (!rule1->label && !rule2->label) {
+			;
+		} 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++) {
+		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;
+		ima_free_rule(entry);
+		integrity_audit_msg(AUDIT_INTEGRITY_STATUS, NULL,
+				    NULL, op, "duplicate-policy", result,
+				    audit_info);
+	}
 
 	return len;
 }
-- 
2.43.0

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

[Anirudh Venkataramanan]

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;
>   }

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

[kernel test robot]

Hi Tahera,

kernel test robot noticed the following build warnings:

[auto build test WARNING on zohar-integrity/next-integrity]
[also build test WARNING on linus/master v6.18-rc4 next-20251107]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Tahera-Fahimi/ima-avoid-duplicate-policy-rules-insertions/20251107-021615
base:   https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git next-integrity
patch link:    https://lore.kernel.org/r/20251106181404.3429710-1-taherafahimi%40linux.microsoft.com
patch subject: [Patch V1] ima: avoid duplicate policy rules insertions
config: alpha-allyesconfig (https://download.01.org/0day-ci/archive/20251107/202511071406.hU1UdCKh-lkp@intel.com/config)
compiler: alpha-linux-gcc (GCC) 15.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20251107/202511071406.hU1UdCKh-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202511071406.hU1UdCKh-lkp@intel.com/

All warnings (new ones prefixed by >>):

   security/integrity/ima/ima_policy.c: In function 'template_has_field':
>> security/integrity/ima/ima_policy.c:1958:13: warning: unused variable 'j' [-Wunused-variable]
    1958 |         int j;
         |             ^
--
>> Warning: security/integrity/ima/ima_policy.c:2078 function parameter 'new_rule' not described in 'ima_rule_exists'


vim +/j +1958 security/integrity/ima/ima_policy.c

  1955	
  1956	static bool template_has_field(const char *field_id, const struct ima_template_desc *template2)
  1957	{
> 1958		int j;
  1959	
  1960		for (int j = 0; j < template2->num_fields; j++)
  1961			if (strcmp(field_id, template2->fields[j]->field_id) == 0)
  1962				return true;
  1963	
  1964		return false;
  1965	}
  1966	
  1967	static bool keyring_has_item(const char *item, const struct ima_rule_opt_list *keyrings)
  1968	{
  1969		int j;
  1970	
  1971		for (j = 0; j < keyrings->count; j++) {
  1972			if (strcmp(item, keyrings->items[j]) == 0)
  1973				return true;
  1974		}
  1975		return false;
  1976	}
  1977	
  1978	static bool labels_has_item(const char *item, const struct ima_rule_opt_list *labels)
  1979	{
  1980		int j;
  1981	
  1982		for (j = 0; j < labels->count; j++) {
  1983			if (strcmp(item, labels->items[j]) == 0)
  1984				return true;
  1985		}
  1986		return false;
  1987	}
  1988	
  1989	static bool ima_rules_equal(const struct ima_rule_entry *rule1, const struct ima_rule_entry *rule2)
  1990	{
  1991		int i;
  1992	
  1993		if (rule1->flags != rule2->flags)
  1994			return false;
  1995	
  1996		if (rule1->action != rule2->action)
  1997			return false;
  1998	
  1999		if (((rule1->flags & IMA_FUNC) && rule1->func != rule2->func) ||
  2000		    ((rule1->flags & (IMA_MASK | IMA_INMASK)) && rule1->mask != rule2->mask) ||
  2001		    ((rule1->flags & IMA_FSMAGIC) && rule1->fsmagic != rule2->fsmagic) ||
  2002		    ((rule1->flags & IMA_FSUUID) && !uuid_equal(&rule1->fsuuid, &rule2->fsuuid)) ||
  2003		    ((rule1->flags & IMA_UID) && !uid_eq(rule1->uid, rule2->uid)) ||
  2004		    ((rule1->flags & IMA_GID) && !gid_eq(rule1->gid, rule2->gid)) ||
  2005		    ((rule1->flags & IMA_FOWNER) && !uid_eq(rule1->fowner, rule2->fowner)) ||
  2006		    ((rule1->flags & IMA_FGROUP) && !gid_eq(rule1->fgroup, rule2->fgroup)) ||
  2007		    ((rule1->flags & IMA_FSNAME) && (strcmp(rule1->fsname, rule2->fsname) != 0)) ||
  2008		    ((rule1->flags & IMA_PCR) && rule1->pcr != rule2->pcr) ||
  2009		    ((rule1->flags & IMA_VALIDATE_ALGOS) &&
  2010		      rule1->allowed_algos != rule2->allowed_algos) ||
  2011		    ((rule1->flags & IMA_EUID) && !uid_eq(rule1->uid, rule2->uid)) ||
  2012		    ((rule1->flags & IMA_EGID) && !gid_eq(rule1->gid, rule2->gid)))
  2013			return false;
  2014	
  2015		if (!rule1->template && !rule2->template) {
  2016			;
  2017		} else if (!rule1->template || !rule2->template) {
  2018			return false;
  2019		} else if (rule1->template->num_fields != rule2->template->num_fields) {
  2020			return false;
  2021		} else if (rule1->template->num_fields != 0) {
  2022			for (i = 0; i < rule1->template->num_fields; i++) {
  2023				if (!template_has_field(rule1->template->fields[i]->field_id,
  2024							rule2->template))
  2025					return false;
  2026			}
  2027		}
  2028	
  2029		if (rule1->flags & IMA_KEYRINGS) {
  2030			if (!rule1->keyrings && !rule2->keyrings) {
  2031				;
  2032			} else if (!rule1->keyrings || !rule2->keyrings) {
  2033				return false;
  2034			} else if (rule1->keyrings->count != rule2->keyrings->count) {
  2035				return false;
  2036			} else if (rule1->keyrings->count != 0) {
  2037				for (i = 0; i < rule1->keyrings->count; i++) {
  2038					if (!keyring_has_item(rule1->keyrings->items[i], rule2->keyrings))
  2039						return false;
  2040				}
  2041			}
  2042		}
  2043	
  2044		if (rule1->flags & IMA_LABEL) {
  2045			if (!rule1->label && !rule2->label) {
  2046				;
  2047			} else if (!rule1->label || !rule2->label) {
  2048				return false;
  2049			} else if (rule1->label->count != rule2->label->count) {
  2050				return false;
  2051			} else if (rule1->label->count != 0) {
  2052				for (i = 0; i < rule1->label->count; i++) {
  2053					if (!labels_has_item(rule1->label->items[i], rule2->label))
  2054						return false;
  2055				}
  2056			}
  2057		}
  2058	
  2059		for (i = 0; i < MAX_LSM_RULES; i++) {
  2060			if (!rule1->lsm[i].rule && !rule2->lsm[i].rule)
  2061				continue;
  2062	
  2063			if (!rule1->lsm[i].rule || !rule2->lsm[i].rule)
  2064				return false;
  2065	
  2066			if (strcmp(rule1->lsm[i].args_p, rule2->lsm[i].args_p) != 0)
  2067				return false;
  2068		}
  2069	
  2070		return true;
  2071	}
  2072	
  2073	/**
  2074	 * ima_rule_exists - check if a rule already exists in the policy
  2075	 *
  2076	 * Checking both the active policy and the temporary rules list.
  2077	 */
> 2078	static bool ima_rule_exists(struct ima_rule_entry *new_rule)
  2079	{
  2080		struct ima_rule_entry *entry;
  2081		struct list_head *ima_rules_tmp;
  2082	
  2083		if (!list_empty(&ima_temp_rules)) {
  2084			list_for_each_entry(entry, &ima_temp_rules, list) {
  2085				if (ima_rules_equal(entry, new_rule))
  2086					return true;
  2087			}
  2088		}
  2089	
  2090		rcu_read_lock();
  2091		ima_rules_tmp = rcu_dereference(ima_rules);
  2092		list_for_each_entry_rcu(entry, ima_rules_tmp, list) {
  2093			if (ima_rules_equal(entry, new_rule)) {
  2094				rcu_read_unlock();
  2095				return true;
  2096			}
  2097		}
  2098		rcu_read_unlock();
  2099	
  2100		return false;
  2101	}
  2102	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[Roberto Sassu]

On Thu, 2025-11-06 at 18:14 +0000, 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.

+ Lennart

Hi Tahera

thanks for the patch!


Wouldn't be better to enhance systemd-soft-reboot to not send the same
IMA policy again?

Thanks

Roberto

> 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;
> +
> +	for (int j = 0; j < template2->num_fields; j++)
> +		if (strcmp(field_id, template2->fields[j]->field_id) == 0)
> +			return true;
> +
> +	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;
> +
> +	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;
> +
> +	if (!rule1->template && !rule2->template) {
> +		;
> +	} 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 (rule1->flags & IMA_KEYRINGS) {
> +		if (!rule1->keyrings && !rule2->keyrings) {
> +			;
> +		} else if (!rule1->keyrings || !rule2->keyrings) {
> +			return false;
> +		} else if (rule1->keyrings->count != rule2->keyrings->count) {
> +			return false;
> +		} else if (rule1->keyrings->count != 0) {
> +			for (i = 0; i < rule1->keyrings->count; i++) {
> +				if (!keyring_has_item(rule1->keyrings->items[i], rule2->keyrings))
> +					return false;
> +			}
> +		}
> +	}
> +
> +	if (rule1->flags & IMA_LABEL) {
> +		if (!rule1->label && !rule2->label) {
> +			;
> +		} 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++) {
> +		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;
> +		ima_free_rule(entry);
> +		integrity_audit_msg(AUDIT_INTEGRITY_STATUS, NULL,
> +				    NULL, op, "duplicate-policy", result,
> +				    audit_info);
> +	}
>  
>  	return len;
>  }

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

[Lennart Poettering]

On Fr, 07.11.25 10:44, Roberto Sassu (roberto.sassu@huaweicloud.com) wrote:

> On Thu, 2025-11-06 at 18:14 +0000, 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.
>
> + Lennart
>
> Hi Tahera
>
> thanks for the patch!
>
> Wouldn't be better to enhance systemd-soft-reboot to not send the same
> IMA policy again?

the soft-reboot logic doesn't load the IMA policy. It's just that
soft-reboot means we reexec PID1: the old pid1 gets replaced by the
new one. And that new PID1 then initializes as it usually would, and
loads security policies again. It currently has support for selinux
policies, ima, ipe, smack.

These policies are supposed to *replace* whatever was loaded
before. Looking at our IMA logic, this doesn't happen right now
though, it just adds stuff:

https://github.com/systemd/systemd/blob/main/src/core/ima-setup.c

Is there a way to replace the old IMA policy with the new, with the
current IMA userspace interface? If so, we should probably make use of
that in systemd, and replace the policy that way. Or in other words:
under the assumption that one can flush out the old IMA policy and
replace it with a new one, I think this should be fixed in systemd,
not the kernel. (of there's no api for flushing out the old
policy/replacing it with the new, then of course we need something
like that in the kernel first).

My understanding of IMA is kinda limited though. I just know what we
do in our codebase.

Lennart

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

[Roberto Sassu]

On Fri, 2025-11-07 at 11:56 +0100, Lennart Poettering wrote:
> On Fr, 07.11.25 10:44, Roberto Sassu (roberto.sassu@huaweicloud.com) wrote:
> 
> > On Thu, 2025-11-06 at 18:14 +0000, 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.
> > 
> > + Lennart
> > 
> > Hi Tahera
> > 
> > thanks for the patch!
> > 
> > Wouldn't be better to enhance systemd-soft-reboot to not send the same
> > IMA policy again?
> 
> the soft-reboot logic doesn't load the IMA policy. It's just that
> soft-reboot means we reexec PID1: the old pid1 gets replaced by the
> new one. And that new PID1 then initializes as it usually would, and
> loads security policies again. It currently has support for selinux
> policies, ima, ipe, smack.
> 
> These policies are supposed to *replace* whatever was loaded
> before. Looking at our IMA logic, this doesn't happen right now
> though, it just adds stuff:

From a functional perspective. As far as integrity is concerned, you
would probably agree that just replacing PID 1 does not mean resetting
the integrity of the system to a known state (all the other processes
are still running). Due to that, I think the concept of soft-reset
should not be applied to the kernel.

> https://github.com/systemd/systemd/blob/main/src/core/ima-setup.c
> 
> Is there a way to replace the old IMA policy with the new, with the
> current IMA userspace interface? If so, we should probably make use of

No, only the IMA boot policies specified in the kernel command line can
be reset (only once, and not completely, secure boot rules still
persist despite user space loads a new policy). New rules are append-
only.

> that in systemd, and replace the policy that way. Or in other words:
> under the assumption that one can flush out the old IMA policy and
> replace it with a new one, I think this should be fixed in systemd,
> not the kernel. (of there's no api for flushing out the old
> policy/replacing it with the new, then of course we need something
> like that in the kernel first).

Assuming that technically it is feasible to flush the old IMA policy
(except for the permanent secure boot rules). What it would be the
additional value of changing the policy on the fly on the same system
as before, but with a different PID 1?

Regarding the duplicate IMA policy load, I guess you could probably
store the digest of the currently loaded policy in the systemd state
and not doing it again after soft-reboot, if the digest matches.

Roberto

> My understanding of IMA is kinda limited though. I just know what we
> do in our codebase.
> 
> Lennart

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

[Tahera Fahimi]

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 <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.
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