[PATCH v4 0/2] ima: Handle -ESTALE returned by ima_filter_rule_match()

[PATCH v4 0/2] ima: Handle -ESTALE returned by ima_filter_rule_match()

[GUO Zihua]

IMA happens to measure extra files if LSM based rules are specified and
the corresponding LSM is updating its policy.

The root cause is explained in the second patch.

GUO Zihua (2):
  ima: Simplify ima_lsm_copy_rule
  ima: Handle -ESTALE returned by ima_filter_rule_match()

 security/integrity/ima/ima_policy.c | 38 +++++++++++++++++++++--------
 1 file changed, 28 insertions(+), 10 deletions(-)

---

v4:
  Use a tempory rule instead of updating the rule in place. To do that,
also update ima_lsm_copy_rule so we can make use of it.

v3:
  Update current rule instead of just retrying, as suggested by Mimi

v2:
  Fixes message errors pointed out by Mimi

-- 
2.17.1

[PATCH v4 1/2] ima: Simplify ima_lsm_copy_rule

[GUO Zihua]

Currently ima_lsm_copy_rule() set the arg_p field of the source rule to
NULL, so that the source rule could be freed afterward. It does not make
sense for this behavior to be inside a "copy" function. So move it
outside and let the caller handle this field.

As of now, the only user of ima_lsm_copy_rule() is
ima_lsm_update_rule(). In ima_lsm_update_rule(), we would like to free
only the lsm.rule in the old rule entry, and leave arg_p untouch. In
this case, it's better to use ima_filter_rule_free() directly and not
introduce another for loop to set arg_p to NULL.

Signed-off-by: GUO Zihua <guozihua@huawei.com>
---
 security/integrity/ima/ima_policy.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index a8802b8da946..8040215c0252 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -398,12 +398,6 @@ static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry)
 
 		nentry->lsm[i].type = entry->lsm[i].type;
 		nentry->lsm[i].args_p = entry->lsm[i].args_p;
-		/*
-		 * Remove the reference from entry so that the associated
-		 * memory will not be freed during a later call to
-		 * ima_lsm_free_rule(entry).
-		 */
-		entry->lsm[i].args_p = NULL;
 
 		ima_filter_rule_init(nentry->lsm[i].type, Audit_equal,
 				     nentry->lsm[i].args_p,
@@ -417,6 +411,7 @@ static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry)
 
 static int ima_lsm_update_rule(struct ima_rule_entry *entry)
 {
+	int i;
 	struct ima_rule_entry *nentry;
 
 	nentry = ima_lsm_copy_rule(entry);
@@ -431,7 +426,8 @@ static int ima_lsm_update_rule(struct ima_rule_entry *entry)
 	 * references and the entry itself. All other memory references will now
 	 * be owned by nentry.
 	 */
-	ima_lsm_free_rule(entry);
+	for (i = 0; i < MAX_LSM_RULES; i++)
+		ima_filter_rule_free(entry->lsm[i].rule);
 	kfree(entry);
 
 	return 0;
-- 
2.17.1

[PATCH v4 2/2] ima: Handle -ESTALE returned by ima_filter_rule_match()

[GUO Zihua]

IMA relies on the blocking LSM policy notifier callback to update the
LSM based IMA policy rules.

When SELinux update its policies, IMA would be notified and starts
updating all its lsm rules one-by-one. During this time, -ESTALE would
be returned by ima_filter_rule_match() if it is called with a LSM rule
that has not yet been updated. In ima_match_rules(), -ESTALE is not
handled, and the LSM rule is considered a match, causing extra files
to be measured by IMA.

Fix it by re-initializing a temporary rule if -ESTALE is returned by
ima_filter_rule_match(). The origin rule in the rule list would be
updated by the LSM policy notifier callback.

Fixes: b16942455193 ("ima: use the lsm policy update notifier")
Signed-off-by: GUO Zihua <guozihua@huawei.com>
---
 security/integrity/ima/ima_policy.c | 28 +++++++++++++++++++++++++---
 1 file changed, 25 insertions(+), 3 deletions(-)

diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 8040215c0252..cb672df9b888 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -545,6 +545,8 @@ static bool ima_match_rules(struct ima_rule_entry *rule,
 			    const char *func_data)
 {
 	int i;
+	bool result = false;
+	bool rule_reinitialized = false;
 
 	if ((rule->flags & IMA_FUNC) &&
 	    (rule->func != func && func != POST_SETATTR))
@@ -612,6 +614,8 @@ static bool ima_match_rules(struct ima_rule_entry *rule,
 			else
 				return false;
 		}
+
+retry:
 		switch (i) {
 		case LSM_OBJ_USER:
 		case LSM_OBJ_ROLE:
@@ -631,10 +635,28 @@ static bool ima_match_rules(struct ima_rule_entry *rule,
 		default:
 			break;
 		}
-		if (!rc)
-			return false;
+
+		if (rc == -ESTALE) {
+			rule = ima_lsm_copy_rule(rule);
+			if (rule) {
+				rule_reinitialized = true;
+				goto retry;
+			}
+		}
+		if (!rc) {
+			result = false;
+			goto out;
+		}
 	}
-	return true;
+	result = true;
+
+out:
+	if (rule_reinitialized) {
+		for (i = 0; i < MAX_LSM_RULES; i++)
+			ima_filter_rule_free(rule->lsm[i].rule);
+		kfree(rule);
+	}
+	return result;
 }
 
 /*
-- 
2.17.1

Re: [PATCH v4 2/2] ima: Handle -ESTALE returned by ima_filter_rule_match()

[Mimi Zohar]

Hi Scott,

> @@ -612,6 +614,8 @@ static bool ima_match_rules(struct ima_rule_entry *rule,
>  			else
>  				return false;
>  		}
> +
> +retry:
>  		switch (i) {
>  		case LSM_OBJ_USER:
>  		case LSM_OBJ_ROLE:
> @@ -631,10 +635,28 @@ static bool ima_match_rules(struct ima_rule_entry *rule,
>  		default:
>  			break;
>  		}
> -		if (!rc)
> -			return false;
> +
> +		if (rc == -ESTALE) {
> +			rule = ima_lsm_copy_rule(rule);

Re-using rule here

> +			if (rule) {

and here doesn't look right.

> +				rule_reinitialized = true;
> +				goto retry;
> +			}
> +		}
> +		if (!rc) {
> +			result = false;
> +			goto out;
> +		}
>  	}
> -	return true;
> +	result = true;
> +
> +out:
> +	if (rule_reinitialized) {
> +		for (i = 0; i < MAX_LSM_RULES; i++)
> +			ima_filter_rule_free(rule->lsm[i].rule);
> +		kfree(rule);
> +	}

Shouldn't freeing the memory be immediately after the retry? 
Otherwise, only the last instance of processing -ESTALE would be freed.

> +	return result;
>  }
>  
>  /*

-- 
thanks,

Mimi

Re: [PATCH v4 1/2] ima: Simplify ima_lsm_copy_rule

[Mimi Zohar]

Hi Scott,

On Fri, 2022-09-09 at 09:15 +0800, GUO Zihua wrote:
> Currently ima_lsm_copy_rule() set the arg_p field of the source rule to
> NULL, so that the source rule could be freed afterward. It does not make
> sense for this behavior to be inside a "copy" function. So move it
> outside and let the caller handle this field.
> 
> As of now, the only user of ima_lsm_copy_rule() is
> ima_lsm_update_rule(). In ima_lsm_update_rule(), we would like to free
> only the lsm.rule in the old rule entry, and leave arg_p untouch. In
> this case, it's better to use ima_filter_rule_free() directly and not
> introduce another for loop to set arg_p to NULL.
> 

This needs to be re-written from a higher perspective.  Perhaps base it
on the existing comment in ima_lsm_update_rule().
 
ima_lsm_copy_rule() shallow copied all references, except for the LSM
references.  Only the LSM references and the entry itself need to be
freed.  Instead of calling ima_lsm_free_rule() to free the LSM
references and also the args_p field, directly free the LSM references.


> Signed-off-by: GUO Zihua <guozihua@huawei.com>
> ---
>  security/integrity/ima/ima_policy.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index a8802b8da946..8040215c0252 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -398,12 +398,6 @@ static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry)
>  
>  		nentry->lsm[i].type = entry->lsm[i].type;
>  		nentry->lsm[i].args_p = entry->lsm[i].args_p;
> -		/*
> -		 * Remove the reference from entry so that the associated
> -		 * memory will not be freed during a later call to
> -		 * ima_lsm_free_rule(entry).
> -		 */
> -		entry->lsm[i].args_p = NULL;
>  
>  		ima_filter_rule_init(nentry->lsm[i].type, Audit_equal,
>  				     nentry->lsm[i].args_p,
> @@ -417,6 +411,7 @@ static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry)
>  
>  static int ima_lsm_update_rule(struct ima_rule_entry *entry)
>  {
> +	int i;
>  	struct ima_rule_entry *nentry;
>  
>  	nentry = ima_lsm_copy_rule(entry);
> @@ -431,7 +426,8 @@ static int ima_lsm_update_rule(struct ima_rule_entry *entry)
>  	 * references and the entry itself. All other memory references will now
>  	 * be owned by nentry.
>  	 */
> -	ima_lsm_free_rule(entry);
> +	for (i = 0; i < MAX_LSM_RULES; i++)
> +		ima_filter_rule_free(entry->lsm[i].rule);
>  	kfree(entry);
>  
>  	return 0;

Re: [PATCH v4 2/2] ima: Handle -ESTALE returned by ima_filter_rule_match()

[Guozihua (Scott)]

On 2022/9/20 5:35, Mimi Zohar wrote:
> Hi Scott,
> 
>> @@ -612,6 +614,8 @@ static bool ima_match_rules(struct ima_rule_entry *rule,
>>   			else
>>   				return false;
>>   		}
>> +
>> +retry:
>>   		switch (i) {
>>   		case LSM_OBJ_USER:
>>   		case LSM_OBJ_ROLE:
>> @@ -631,10 +635,28 @@ static bool ima_match_rules(struct ima_rule_entry *rule,
>>   		default:
>>   			break;
>>   		}
>> -		if (!rc)
>> -			return false;
>> +
>> +		if (rc == -ESTALE) {
>> +			rule = ima_lsm_copy_rule(rule);
> 
> Re-using rule here

I'll use another variable here.
> 
>> +			if (rule) {
> 
> and here doesn't look right.

What seems to be wrong here? ima_lsm_copy_rule() returns a shallow copy 
of the rule, and NULL if the copy fails. Only if the returned rule is 
not NULL should we proceed with the retry. I used rule_reinitialized to 
memorize whether the current rule is copied so that we should free it 
later on.
> 
>> +				rule_reinitialized = true;
>> +				goto retry;
>> +			}
>> +		}
>> +		if (!rc) {
>> +			result = false;
>> +			goto out;
>> +		}
>>   	}
>> -	return true;
>> +	result = true;
>> +
>> +out:
>> +	if (rule_reinitialized) {
>> +		for (i = 0; i < MAX_LSM_RULES; i++)
>> +			ima_filter_rule_free(rule->lsm[i].rule);
>> +		kfree(rule);
>> +	}
> 
> Shouldn't freeing the memory be immediately after the retry?
> Otherwise, only the last instance of processing -ESTALE would be freed.

ima_lsm_copy_rule() would update every member of rule->lsm, and the 
retry is within the for loop on members of rule->lsm. We'd better keep 
the copied rule till the loop ends. To avoid race condition if the LSM 
rule has been updated again during the loop, I can add a guard here.
> 
>> +	return result;
>>   }
>>   
>>   /*
> 


-- 
Best
GUO Zihua