sleep in selinux_audit_rule_init

[Stephen Smalley]

On 5/30/19 6:39 AM, Janne Karhunen wrote:
> On Wed, May 22, 2019 at 6:27 PM Stephen Smalley <sds at tycho.nsa.gov> 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
>