On 5/30/19 6:39 AM, Janne Karhunen wrote:
On Wed, May 22, 2019 at 6:27 PM Stephen Smalley <sds@xxxxxxxxxxxxx> wrote:
Ok. The question is then how should IMA handle missing domains/types.
Just dropping IMA policy rules doesn't sound safe, nor does skipping
rules in case the domains/types are restored.
You can just do what audit_dupe_lsm_field() does. It effectively
disables the rule upon the invalidation (which makes sense, since it can
no longer match anything since nothing can have that domain/type) but
retains the string value so it can later re-activate the rule if the
domain/type becomes valid again later.
Finally got a moment to look into this. It looks to me there is
already a notifier? Could something like this work?
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index d213e835c498..2203451862d4 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -154,6 +154,8 @@ unsigned long ima_get_binary_runtime_size(void);
int ima_init_template(void);
void ima_init_template_list(void);
int __init ima_init_digests(void);
+int ima_lsm_policy_change(struct notifier_block *nb, unsigned long event,
+ void *lsm_data);
/*
* used to protect h_table and sha_table
diff --git a/security/integrity/ima/ima_main.c
b/security/integrity/ima/ima_main.c
index 5749ec92516f..449502f5c3dc 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -52,6 +52,10 @@ int ima_hash_algo = HASH_ALGO_SHA1;
static int hash_setup_done;
static struct workqueue_struct *ima_update_wq;
+static struct notifier_block ima_lsm_policy_notifier = {
+ .notifier_call = ima_lsm_policy_change,
+};
+
static int __init hash_setup(char *str)
{
struct ima_template_desc *template_desc = ima_template_desc_current();
@@ -691,6 +695,10 @@ static int __init init_ima(void)
error = ima_init();
}
+ error = register_lsm_notifier(&ima_lsm_policy_notifier);
+ if (error)
+ pr_warn("Couldn't register LSM notifier, error %d\n", error);
+
if (!error)
ima_update_policy_flag();
else
diff --git a/security/integrity/ima/ima_policy.c
b/security/integrity/ima/ima_policy.c
index e0cc323f948f..c3983d24279a 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -252,8 +252,8 @@ __setup("ima_appraise_tcb", default_appraise_policy_setup);
/*
* The LSM policy can be reloaded, leaving the IMA LSM based rules referring
* to the old, stale LSM policy. Update the IMA LSM based rules to reflect
- * the reloaded LSM policy. We assume the rules still exist; and BUG_ON() if
- * they don't.
+ * the reloaded LSM policy. Keep currently invalid fields around in case
+ * they become valid after a policy reload.
*/
static void ima_lsm_update_rules(void)
{
@@ -269,11 +269,23 @@ static void ima_lsm_update_rules(void)
Audit_equal,
entry->lsm[i].args_p,
&entry->lsm[i].rule);
- BUG_ON(!entry->lsm[i].rule);
+ if (result == -EINVAL)
+ pr_warn("ima: rule for LSM \'%d\' is invalid\n",
+ entry->lsm[i].type);
I could be wrong, but I think there is still a problem here in that you
are modifying entry->lsm[i].rule in-place, but it is protected under RCU
and therefore needs to be duplicated and then modified? Also you are
leaking the old rule? Both of those issues also exist prior to your
patch but you aren't fixing them here. And lastly, it looks like lsm
notifiers are atomic notifiers (not clear to me why) so you can't block
in the callback, thereby requiring scheduling the work as is done in
infiniband. I'm not sure though why we can't make the lsm notifiers
blocking notifiers. The only callers of call_lsm_notifier() are
sel_write_enforce() and selinux_lsm_notifier_avc_callback(), called from
avc_ss_reset(), called from sel_write_enforce(), security_load_policy()
and security_set_bools(), all outside of locks and in process context
AFAICS.
}
}
}
+int ima_lsm_policy_change(struct notifier_block *nb, unsigned long event,
+ void *lsm_data)
+{
+ if (event != LSM_POLICY_CHANGE)
+ return NOTIFY_DONE;
+
+ ima_lsm_update_rules();
+ return NOTIFY_DONE;
+}
+
/**
* ima_match_rules - determine whether an inode matches the measure rule.
* @rule: a pointer to a rule
@@ -327,11 +339,10 @@ static bool ima_match_rules(struct
ima_rule_entry *rule, struct inode *inode,
for (i = 0; i < MAX_LSM_RULES; i++) {
int rc = 0;
u32 osid;
- int retried = 0;
if (!rule->lsm[i].rule)
continue;
-retry:
+
switch (i) {
case LSM_OBJ_USER:
case LSM_OBJ_ROLE:
@@ -352,11 +363,6 @@ static bool ima_match_rules(struct ima_rule_entry
*rule, struct inode *inode,
default:
break;
}
- if ((rc < 0) && (!retried)) {
- retried = 1;
- ima_lsm_update_rules();
- goto retry;
- }
if (!rc)
return false;
}
--
Janne
