Re: [PATCH v35 05/29] IMA: avoid label collisions with stacked LSMs

[Casey Schaufler <casey@schaufler-ca.com>]

On 4/20/2022 12:23 PM, Mimi Zohar wrote:
> Hi Casey,
>
> Below are a few initial comments/questions from a high level...
>
> On Tue, 2022-04-19 at 09:50 -0700, Casey Schaufler wrote:
>> On 4/18/2022 7:59 AM, Casey Schaufler wrote:
>>> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
>>> index eea6e92500b8..97470354c8ae 100644
>>> --- a/security/integrity/ima/ima_policy.c
>>> +++ b/security/integrity/ima/ima_policy.c
>>> @@ -89,6 +89,7 @@ struct ima_rule_entry {
>>>    	bool (*fgroup_op)(kgid_t cred_gid, kgid_t rule_gid); /* gid_eq(), gid_gt(), gid_lt() */
>>>    	int pcr;
>>>    	unsigned int allowed_algos; /* bitfield of allowed hash algorithms */
>>> +	int which;		/* which LSM rule applies to */
> If "which" was defined in the lsm[] structure, it would be clear
> reading the code that "which" refers to an LSM (e.g. entry-
>> lsm[i].which).  Perhaps rename "which" to "which_lsm", "lsm_slot", or
> "rules_lsm".

Both fine suggestions. I will incorporate them.

>
>>>    	struct {
>>>    		void *rule;	/* LSM file metadata specific */
>>>    		char *args_p;	/* audit value */
>>> @@ -285,6 +286,20 @@ static int __init default_appraise_policy_setup(char *str)
>>>    }
>>>    __setup("ima_appraise_tcb", default_appraise_policy_setup);
>>>    
>>> +static int ima_rules_lsm __ro_after_init;
>>> +
>>> +static int __init ima_rules_lsm_init(char *str)
>>> +{
>>> +	ima_rules_lsm = lsm_name_to_slot(str);
>>> +	if (ima_rules_lsm < 0) {
>>> +		ima_rules_lsm = 0;
>>> +		pr_err("rule lsm \"%s\" not registered", str);
>>> +	}
> Specific IMA policy rules could be independent of the default one being
> initialized here.  Probably "ima_rules_lsm" should be renamed
> "default_rules_lsm" or "default_ima_rules_lsm".

Sure. No problem to change.

>    The pr_err() message
> should indicate setting the default rule LSM failed with an indication
> of which LSM is set as the default.
>
> Assuming 0 is guaranteed to be a valid LSM,

Unfortunately, it's possible for there to be no LSMs,
in which case 0 won't match any LSM when the hooks are
being invoked.

>   then something like:
>   "default rule lsm \"%s\" not registered, using \"%s"\", str,
> lsm_slot_to_name(0));
>
>>> +
>>> +	return 1;
>>> +}
>>> +__setup("ima_rules_lsm=", ima_rules_lsm_init);
>>> +
>>>    static struct ima_rule_opt_list *ima_alloc_rule_opt_list(const substring_t *src)
>>>    {
>>>    	struct ima_rule_opt_list *opt_list;
>>> @@ -356,7 +371,7 @@ static void ima_lsm_free_rule(struct ima_rule_entry *entry)
>>>    	int i;
>>>    
>>>    	for (i = 0; i < MAX_LSM_RULES; i++) {
>>> -		ima_filter_rule_free(entry->lsm[i].rule);
>>> +		ima_filter_rule_free(entry->lsm[i].rule, entry->which);
>>>    		kfree(entry->lsm[i].args_p);
>>>    	}
>>>    }
> ima_rules_lsm is initialized to 0,  If it isn't guranteed to be a valid
> LSM, then ima_rules_lsm_init() needs to be called from ima_init.c:
> ima_init(), so that it can be reset to an invalid value.  Then
> ima_filter_rule_init()/free() could check it.

If there is no LSM in slot 0 that implies there are no LSMs
suppling the hooks. Since the list of hooks to invoke will be
empty it doesn't matter what value is in default_rules_lsm.

>
> thanks,
>
> Mimi
>