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".
> > 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". 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, 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.
thanks,
Mimi
