[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