sleep in selinux_audit_rule_init

sleep in selinux_audit_rule_init

[Janne Karhunen]

Hi,

I managed to hit a following BUG, looks like ima can call
selinux_audit_rule_init that can sleep in rcu critical section in
ima_match_policy():

__might_sleep
kmem_cache_alloc_trace
selinux_audit_rule_init <<< kzalloc (.. GFP_KERNEL)
security_audit_rule_init
ima_match_policy <<< list_for_each_entry_rcu
ima_get_action
process_measurement
ima_file_check
path_openat
do_filp_open
..

I guess this is the ima_match_rules() calling ima_lsm_update_rules()
when it concludes that the selinux policy may have been reloaded.

The easy way for me to fix my own butt in this regard is to change the
selinux allocation not to wait, but Paul would you be OK with such
change? The alternative looks like a pretty big change in the ima?


--
Janne

Re: sleep in selinux_audit_rule_init

[Stephen Smalley]

On 5/22/19 7:49 AM, Janne Karhunen wrote:
> Hi,
> 
> I managed to hit a following BUG, looks like ima can call
> selinux_audit_rule_init that can sleep in rcu critical section in
> ima_match_policy():
> 
> __might_sleep
> kmem_cache_alloc_trace
> selinux_audit_rule_init <<< kzalloc (.. GFP_KERNEL)
> security_audit_rule_init
> ima_match_policy <<< list_for_each_entry_rcu
> ima_get_action
> process_measurement
> ima_file_check
> path_openat
> do_filp_open
> ..
> 
> I guess this is the ima_match_rules() calling ima_lsm_update_rules()
> when it concludes that the selinux policy may have been reloaded.
> 
> The easy way for me to fix my own butt in this regard is to change the
> selinux allocation not to wait, but Paul would you be OK with such
> change? The alternative looks like a pretty big change in the ima?

This is perhaps a sign of a deeper bug in IMA; if they are in the middle 
of matching against their policy rules, then they shouldn't be 
updating/modifying those rules in the middle of match processing?  How 
is that safe under RCU?

If you look at how the audit subsystem deals with the same problem, they 
have a callback (audit_update_lsm_rules) that is called upon an AVC 
reset (hence upon a policy reload) and can update all of their rules at 
that time, not lazily during matching.  Since that time, a more general 
notifier mechanism was added, register_lsm_notifier(), and is used by 
infiniband to update its state upon policy changes.

Re: sleep in selinux_audit_rule_init

[Stephen Smalley]

On 5/22/19 8:20 AM, Stephen Smalley wrote:
> On 5/22/19 7:49 AM, Janne Karhunen wrote:
>> Hi,
>>
>> I managed to hit a following BUG, looks like ima can call
>> selinux_audit_rule_init that can sleep in rcu critical section in
>> ima_match_policy():
>>
>> __might_sleep
>> kmem_cache_alloc_trace
>> selinux_audit_rule_init <<< kzalloc (.. GFP_KERNEL)
>> security_audit_rule_init
>> ima_match_policy <<< list_for_each_entry_rcu
>> ima_get_action
>> process_measurement
>> ima_file_check
>> path_openat
>> do_filp_open
>> ..
>>
>> I guess this is the ima_match_rules() calling ima_lsm_update_rules()
>> when it concludes that the selinux policy may have been reloaded.
>>
>> The easy way for me to fix my own butt in this regard is to change the
>> selinux allocation not to wait, but Paul would you be OK with such
>> change? The alternative looks like a pretty big change in the ima?
> 
> This is perhaps a sign of a deeper bug in IMA; if they are in the middle 
> of matching against their policy rules, then they shouldn't be 
> updating/modifying those rules in the middle of match processing?  How 
> is that safe under RCU?
> 
> If you look at how the audit subsystem deals with the same problem, they 
> have a callback (audit_update_lsm_rules) that is called upon an AVC 
> reset (hence upon a policy reload) and can update all of their rules at 
> that time, not lazily during matching.  Since that time, a more general 
> notifier mechanism was added, register_lsm_notifier(), and is used by 
> infiniband to update its state upon policy changes.

Another potentially worrisome aspect of the current 
ima_lsm_update_rules() logic is that it does a BUG_ON() if the attempt 
to update the rule fails, which could occur if e.g. one had an IMA 
policy rule based on a given domain/type and that domain/type were 
removed from policy (e.g. via policy module removal).  Contrast with the 
handling in audit_dupe_lsm_field().  The existing ima_lsm_update_rules() 
logic could also yield a BUG_ON upon transient memory allocation failure.

Re: sleep in selinux_audit_rule_init

[Mimi Zohar]

On Wed, 2019-05-22 at 08:41 -0400, Stephen Smalley wrote:
> Another potentially worrisome aspect of the current 
> ima_lsm_update_rules() logic is that it does a BUG_ON() if the attempt 
> to update the rule fails, which could occur if e.g. one had an IMA 
> policy rule based on a given domain/type and that domain/type were 
> removed from policy (e.g. via policy module removal).  Contrast with the 
> handling in audit_dupe_lsm_field().  The existing ima_lsm_update_rules() 
> logic could also yield a BUG_ON upon transient memory allocation failure.

The original design was based on the assumption that SELinux labels
could not be removed, only new ones could be added.  Sounds like that
isn't the case any longer.

Mimi

Re: sleep in selinux_audit_rule_init

[Stephen Smalley]

On 5/22/19 9:00 AM, Mimi Zohar wrote:
> On Wed, 2019-05-22 at 08:41 -0400, Stephen Smalley wrote:
>> Another potentially worrisome aspect of the current
>> ima_lsm_update_rules() logic is that it does a BUG_ON() if the attempt
>> to update the rule fails, which could occur if e.g. one had an IMA
>> policy rule based on a given domain/type and that domain/type were
>> removed from policy (e.g. via policy module removal).  Contrast with the
>> handling in audit_dupe_lsm_field().  The existing ima_lsm_update_rules()
>> logic could also yield a BUG_ON upon transient memory allocation failure.
> 
> The original design was based on the assumption that SELinux labels
> could not be removed, only new ones could be added.  Sounds like that
> isn't the case any longer.

That's never really been the case for SELinux; it has always been 
possible to reload with a policy that renders previously valid security 
contexts invalid.  What has changed over time is the ability of SELinux 
to gracefully handle the situation where a security context is rendered 
invalid upon a policy reload and then later restored to validity via a 
subsequent policy reload (e.g. removing a policy module and then 
re-adding it), but even that deferred mapping of contexts support has 
been around since 2008.

What you are likely thinking of is the conventional practice of 
distributions, which is generally to not remove domains/types from their 
policy or to at least retain a type alias for compatibility reasons. 
But that's just a convention, not guaranteed by any mechanism, and users 
are free to remove policy modules.

Re: sleep in selinux_audit_rule_init

[Mimi Zohar]

On Wed, 2019-05-22 at 09:16 -0400, Stephen Smalley wrote:
> On 5/22/19 9:00 AM, Mimi Zohar wrote:
> > On Wed, 2019-05-22 at 08:41 -0400, Stephen Smalley wrote:
> >> Another potentially worrisome aspect of the current
> >> ima_lsm_update_rules() logic is that it does a BUG_ON() if the attempt
> >> to update the rule fails, which could occur if e.g. one had an IMA
> >> policy rule based on a given domain/type and that domain/type were
> >> removed from policy (e.g. via policy module removal).  Contrast with the
> >> handling in audit_dupe_lsm_field().  The existing ima_lsm_update_rules()
> >> logic could also yield a BUG_ON upon transient memory allocation failure.
> > 
> > The original design was based on the assumption that SELinux labels
> > could not be removed, only new ones could be added.  Sounds like that
> > isn't the case any longer.
> 
> That's never really been the case for SELinux; it has always been 
> possible to reload with a policy that renders previously valid security 
> contexts invalid.  What has changed over time is the ability of SELinux 
> to gracefully handle the situation where a security context is rendered 
> invalid upon a policy reload and then later restored to validity via a 
> subsequent policy reload (e.g. removing a policy module and then 
> re-adding it), but even that deferred mapping of contexts support has 
> been around since 2008.
> 
> What you are likely thinking of is the conventional practice of 
> distributions, which is generally to not remove domains/types from their 
> policy or to at least retain a type alias for compatibility reasons. 
> But that's just a convention, not guaranteed by any mechanism, and users 
> are free to remove policy modules.

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.

Mimi  

Re: sleep in selinux_audit_rule_init

[Casey Schaufler]

On 5/22/2019 6:57 AM, Mimi Zohar wrote:
> On Wed, 2019-05-22 at 09:16 -0400, Stephen Smalley wrote:
>> On 5/22/19 9:00 AM, Mimi Zohar wrote:
>>> On Wed, 2019-05-22 at 08:41 -0400, Stephen Smalley wrote:
>>>> Another potentially worrisome aspect of the current
>>>> ima_lsm_update_rules() logic is that it does a BUG_ON() if the attempt
>>>> to update the rule fails, which could occur if e.g. one had an IMA
>>>> policy rule based on a given domain/type and that domain/type were
>>>> removed from policy (e.g. via policy module removal).  Contrast with the
>>>> handling in audit_dupe_lsm_field().  The existing ima_lsm_update_rules()
>>>> logic could also yield a BUG_ON upon transient memory allocation failure.
>>> The original design was based on the assumption that SELinux labels
>>> could not be removed, only new ones could be added. ??Sounds like that
>>> isn't the case any longer.
>> That's never really been the case for SELinux; it has always been 
>> possible to reload with a policy that renders previously valid security 
>> contexts invalid.  What has changed over time is the ability of SELinux 
>> to gracefully handle the situation where a security context is rendered 
>> invalid upon a policy reload and then later restored to validity via a 
>> subsequent policy reload (e.g. removing a policy module and then 
>> re-adding it), but even that deferred mapping of contexts support has 
>> been around since 2008.
>>
>> What you are likely thinking of is the conventional practice of 
>> distributions, which is generally to not remove domains/types from their 
>> policy or to at least retain a type alias for compatibility reasons. 
>> But that's just a convention, not guaranteed by any mechanism, and users 
>> are free to remove policy modules.
> 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.

Smack has a case where the subject label might never have been
seen by the system before, and hence can't be in any rules. This
can occur when a labeled packet comes from another host. Because
a subject with the star ("*") label is never allowed access to
anything, that is a convenient value to use. It is never used as
the subject label otherwise.

You could do something similar if there is a SELinux domain/type
that you can rely on being present. I fear that there may not be
any such element, but it wouldn't hurt (too much) too look.

>
> Mimi ??
>
>

Re: sleep in selinux_audit_rule_init

[Stephen Smalley]

On 5/22/19 9:57 AM, Mimi Zohar wrote:
> On Wed, 2019-05-22 at 09:16 -0400, Stephen Smalley wrote:
>> On 5/22/19 9:00 AM, Mimi Zohar wrote:
>>> On Wed, 2019-05-22 at 08:41 -0400, Stephen Smalley wrote:
>>>> Another potentially worrisome aspect of the current
>>>> ima_lsm_update_rules() logic is that it does a BUG_ON() if the attempt
>>>> to update the rule fails, which could occur if e.g. one had an IMA
>>>> policy rule based on a given domain/type and that domain/type were
>>>> removed from policy (e.g. via policy module removal).  Contrast with the
>>>> handling in audit_dupe_lsm_field().  The existing ima_lsm_update_rules()
>>>> logic could also yield a BUG_ON upon transient memory allocation failure.
>>>
>>> The original design was based on the assumption that SELinux labels
>>> could not be removed, only new ones could be added.  Sounds like that
>>> isn't the case any longer.
>>
>> That's never really been the case for SELinux; it has always been
>> possible to reload with a policy that renders previously valid security
>> contexts invalid.  What has changed over time is the ability of SELinux
>> to gracefully handle the situation where a security context is rendered
>> invalid upon a policy reload and then later restored to validity via a
>> subsequent policy reload (e.g. removing a policy module and then
>> re-adding it), but even that deferred mapping of contexts support has
>> been around since 2008.
>>
>> What you are likely thinking of is the conventional practice of
>> distributions, which is generally to not remove domains/types from their
>> policy or to at least retain a type alias for compatibility reasons.
>> But that's just a convention, not guaranteed by any mechanism, and users
>> are free to remove policy modules.
> 
> 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.

Re: sleep in selinux_audit_rule_init

[Janne Karhunen]

On Wed, May 22, 2019 at 6:27 PM Stephen Smalley <sds@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);
                }
        }
 }

+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

Re: 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@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
> 

Re: sleep in selinux_audit_rule_init

[Paul Moore]

On Thu, May 30, 2019 at 8:07 AM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> ... 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.

Off the top of my head I don't recall why the atomic notifiers were
chosen over the blocking notifiers; it may simply be an artifact of an
interim patch that was changed.  Regardless, I have no problem if we
switch to using blocking notifiers.  However, if we are changing it
now it might be a good idea to also add a "block"/"blocking" somewhere
in the lsm_notifier functions' name to make the change obvious and to
help make it easier if we ever need to add atomic notifier support in
the future.

-- 
paul moore
www.paul-moore.com

Re: sleep in selinux_audit_rule_init

[Janne Karhunen]

On Thu, May 30, 2019 at 3:08 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:

> > @@ -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?

Right. Bit too fast tapping the keyboard without thinking, will fix
and post in the proper form. But I guess the original point was to
verify if that 'notifier_block' is indeed the right way to get the
update notification?


>  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.

Great catch, thank you. That's an easy fix if no-one objects pushing
these through the system-wq for example.


--
Janne

Re: sleep in selinux_audit_rule_init

[Stephen Smalley]

On 5/30/19 9:27 AM, Janne Karhunen wrote:
> On Thu, May 30, 2019 at 3:08 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> 
>>> @@ -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?
> 
> Right. Bit too fast tapping the keyboard without thinking, will fix
> and post in the proper form. But I guess the original point was to
> verify if that 'notifier_block' is indeed the right way to get the
> update notification?

Yes.

> 
>>   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.
> 
> Great catch, thank you. That's an easy fix if no-one objects pushing
> these through the system-wq for example.

I think you can switch the lsm notifier over to using blocking notifiers 
instead; there seems to be no valid reason for making it atomic.

Re: sleep in selinux_audit_rule_init

[Janne Karhunen]

On Thu, May 30, 2019 at 5:17 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:

> >>   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.
> >
> > Great catch, thank you. That's an easy fix if no-one objects pushing
> > these through the system-wq for example.
>
> I think you can switch the lsm notifier over to using blocking notifiers
> instead; there seems to be no valid reason for making it atomic.

Further drafting. I certainly feel uneasy with the rcu update side of
it, but the string is not used in the matching so.. ?


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..6776dc2b9664 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -252,12 +252,14 @@ __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.
  */
 static void ima_lsm_update_rules(void)
 {
        struct ima_rule_entry *entry;
+       void *rule_new;
+       char *lsm_new;
+       char *lsm_old;
        int result;
        int i;
@@ -265,13 +267,37 @@ static void ima_lsm_update_rules(void)
                for (i = 0; i < MAX_LSM_RULES; i++) {
                        if (!entry->lsm[i].rule)
                                continue;
+
+                       lsm_old = entry->lsm[i].args_p;
+                       lsm_new = kstrdup(lsm_old, GFP_KERNEL);
+                       if (unlikely(!lsm_new))
+                               return;
+
                        result = security_filter_rule_init(entry->lsm[i].type,
                                                           Audit_equal,
-                                                          entry->lsm[i].args_p,
-                                                          &entry->lsm[i].rule);
-                       BUG_ON(!entry->lsm[i].rule);
-               }
-       }
+                                                          lsm_new,
+                                                          &rule_new);
+                       if (result == -EINVAL)
+                               pr_warn("ima: rule for LSM \'%d\' is invalid\n",
+                                       entry->lsm[i].type);
+
+                       entry->lsm[i].rule = rule_new;
+                       entry->lsm[i].args_p = lsm_new;
+                       synchronize_rcu();
+
+                       kfree(lsm_old);
+                }
+        }
+}
+
+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_OK;
 }

 /**
@@ -327,11 +353,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 +377,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;
        }
diff --git a/security/security.c b/security/security.c
index 23cbb1a295a3..c5e69ce81521 100644
--- a/security/security.c
+++ b/security/security.c
@@ -39,7 +39,7 @@
 #define LSM_COUNT (__end_lsm_info - __start_lsm_info)

 struct security_hook_heads security_hook_heads __lsm_ro_after_init;
-static ATOMIC_NOTIFIER_HEAD(lsm_notifier_chain);
+static BLOCKING_NOTIFIER_HEAD(lsm_notifier_chain);

 static struct kmem_cache *lsm_file_cache;
 static struct kmem_cache *lsm_inode_cache;
@@ -432,19 +432,19 @@ void __init security_add_hooks(struct
security_hook_list *hooks, int count,

 int call_lsm_notifier(enum lsm_event event, void *data)
 {
-       return atomic_notifier_call_chain(&lsm_notifier_chain, event, data);
+       return blocking_notifier_call_chain(&lsm_notifier_chain, event, data);
 }
 EXPORT_SYMBOL(call_lsm_notifier);

 int register_lsm_notifier(struct notifier_block *nb)
 {
-       return atomic_notifier_chain_register(&lsm_notifier_chain, nb);
+       return blocking_notifier_chain_register(&lsm_notifier_chain, nb);
 }
 EXPORT_SYMBOL(register_lsm_notifier);

 int unregister_lsm_notifier(struct notifier_block *nb)
 {
-       return atomic_notifier_chain_unregister(&lsm_notifier_chain, nb);
+       return blocking_notifier_chain_unregister(&lsm_notifier_chain, nb);
 }
 EXPORT_SYMBOL(unregister_lsm_notifier);


--
Janne

Re: sleep in selinux_audit_rule_init

[Janne Karhunen]

On Wed, May 22, 2019 at 3:20 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:

> > I managed to hit a following BUG, looks like ima can call
> > selinux_audit_rule_init that can sleep in rcu critical section in
> > ima_match_policy():
> >
> > __might_sleep
> > kmem_cache_alloc_trace
> > selinux_audit_rule_init <<< kzalloc (.. GFP_KERNEL)
> > security_audit_rule_init
> > ima_match_policy <<< list_for_each_entry_rcu
> > ima_get_action
> > process_measurement
> > ima_file_check
> > path_openat
> > do_filp_open
> > ..
> >
> > I guess this is the ima_match_rules() calling ima_lsm_update_rules()
> > when it concludes that the selinux policy may have been reloaded.
> >
> > The easy way for me to fix my own butt in this regard is to change the
> > selinux allocation not to wait, but Paul would you be OK with such
> > change? The alternative looks like a pretty big change in the ima?
>
> This is perhaps a sign of a deeper bug in IMA; if they are in the middle
> of matching against their policy rules, then they shouldn't be
> updating/modifying those rules in the middle of match processing?  How
> is that safe under RCU?

Heh indeed...


> If you look at how the audit subsystem deals with the same problem, they
> have a callback (audit_update_lsm_rules) that is called upon an AVC
> reset (hence upon a policy reload) and can update all of their rules at
> that time, not lazily during matching.  Since that time, a more general
> notifier mechanism was added, register_lsm_notifier(), and is used by
> infiniband to update its state upon policy changes.

I guess the same approach could work here. I'll see how that would
look like exactly..


--
Janne