Linux-Integrity Archive on lore.kernel.org
 help / color / Atom feed
* sleep in selinux_audit_rule_init
@ 2019-05-22 11:49 Janne Karhunen
  2019-05-22 12:20 ` Stephen Smalley
  0 siblings, 1 reply; 15+ messages in thread
From: Janne Karhunen @ 2019-05-22 11:49 UTC (permalink / raw)
  To: Mimi Zohar, paul; +Cc: linux-integrity, linux-security-module

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: sleep in selinux_audit_rule_init
  2019-05-22 11:49 sleep in selinux_audit_rule_init Janne Karhunen
@ 2019-05-22 12:20 ` Stephen Smalley
  2019-05-22 12:41   ` Stephen Smalley
  2019-05-22 12:47   ` Janne Karhunen
  0 siblings, 2 replies; 15+ messages in thread
From: Stephen Smalley @ 2019-05-22 12:20 UTC (permalink / raw)
  To: Janne Karhunen, Mimi Zohar, paul; +Cc: linux-integrity, linux-security-module

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.




^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: sleep in selinux_audit_rule_init
  2019-05-22 12:20 ` Stephen Smalley
@ 2019-05-22 12:41   ` Stephen Smalley
  2019-05-22 13:00     ` Mimi Zohar
  2019-05-22 12:47   ` Janne Karhunen
  1 sibling, 1 reply; 15+ messages in thread
From: Stephen Smalley @ 2019-05-22 12:41 UTC (permalink / raw)
  To: Janne Karhunen, Mimi Zohar, paul; +Cc: linux-integrity, linux-security-module

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.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: sleep in selinux_audit_rule_init
  2019-05-22 12:20 ` Stephen Smalley
  2019-05-22 12:41   ` Stephen Smalley
@ 2019-05-22 12:47   ` Janne Karhunen
  1 sibling, 0 replies; 15+ messages in thread
From: Janne Karhunen @ 2019-05-22 12:47 UTC (permalink / raw)
  To: Stephen Smalley; +Cc: Mimi Zohar, paul, linux-integrity, linux-security-module

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: sleep in selinux_audit_rule_init
  2019-05-22 12:41   ` Stephen Smalley
@ 2019-05-22 13:00     ` Mimi Zohar
  2019-05-22 13:16       ` Stephen Smalley
  0 siblings, 1 reply; 15+ messages in thread
From: Mimi Zohar @ 2019-05-22 13:00 UTC (permalink / raw)
  To: Stephen Smalley, Janne Karhunen, paul
  Cc: linux-integrity, linux-security-module

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


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: sleep in selinux_audit_rule_init
  2019-05-22 13:00     ` Mimi Zohar
@ 2019-05-22 13:16       ` Stephen Smalley
  2019-05-22 13:57         ` Mimi Zohar
  0 siblings, 1 reply; 15+ messages in thread
From: Stephen Smalley @ 2019-05-22 13:16 UTC (permalink / raw)
  To: Mimi Zohar, Janne Karhunen, paul; +Cc: linux-integrity, linux-security-module

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.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: sleep in selinux_audit_rule_init
  2019-05-22 13:16       ` Stephen Smalley
@ 2019-05-22 13:57         ` Mimi Zohar
  2019-05-22 15:10           ` Casey Schaufler
  2019-05-22 15:27           ` Stephen Smalley
  0 siblings, 2 replies; 15+ messages in thread
From: Mimi Zohar @ 2019-05-22 13:57 UTC (permalink / raw)
  To: Stephen Smalley, Janne Karhunen, paul
  Cc: linux-integrity, linux-security-module

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  


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: sleep in selinux_audit_rule_init
  2019-05-22 13:57         ` Mimi Zohar
@ 2019-05-22 15:10           ` Casey Schaufler
  2019-05-22 15:27           ` Stephen Smalley
  1 sibling, 0 replies; 15+ messages in thread
From: Casey Schaufler @ 2019-05-22 15:10 UTC (permalink / raw)
  To: Mimi Zohar, Stephen Smalley, Janne Karhunen, paul
  Cc: linux-integrity, linux-security-module

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: sleep in selinux_audit_rule_init
  2019-05-22 13:57         ` Mimi Zohar
  2019-05-22 15:10           ` Casey Schaufler
@ 2019-05-22 15:27           ` Stephen Smalley
  2019-05-30 10:39             ` Janne Karhunen
  1 sibling, 1 reply; 15+ messages in thread
From: Stephen Smalley @ 2019-05-22 15:27 UTC (permalink / raw)
  To: Mimi Zohar, Janne Karhunen, paul; +Cc: linux-integrity, linux-security-module

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.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: sleep in selinux_audit_rule_init
  2019-05-22 15:27           ` Stephen Smalley
@ 2019-05-30 10:39             ` Janne Karhunen
  2019-05-30 12:07               ` Stephen Smalley
  0 siblings, 1 reply; 15+ messages in thread
From: Janne Karhunen @ 2019-05-30 10:39 UTC (permalink / raw)
  To: Stephen Smalley
  Cc: Mimi Zohar, Paul Moore, linux-integrity, linux-security-module

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: sleep in selinux_audit_rule_init
  2019-05-30 10:39             ` Janne Karhunen
@ 2019-05-30 12:07               ` Stephen Smalley
  2019-05-30 12:29                 ` Paul Moore
  2019-05-30 13:27                 ` Janne Karhunen
  0 siblings, 2 replies; 15+ messages in thread
From: Stephen Smalley @ 2019-05-30 12:07 UTC (permalink / raw)
  To: Janne Karhunen
  Cc: Mimi Zohar, Paul Moore, linux-integrity, linux-security-module,
	Dan Jurgens

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
> 


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: sleep in selinux_audit_rule_init
  2019-05-30 12:07               ` Stephen Smalley
@ 2019-05-30 12:29                 ` Paul Moore
  2019-05-30 13:27                 ` Janne Karhunen
  1 sibling, 0 replies; 15+ messages in thread
From: Paul Moore @ 2019-05-30 12:29 UTC (permalink / raw)
  To: Stephen Smalley
  Cc: Janne Karhunen, Mimi Zohar, linux-integrity,
	linux-security-module, Dan Jurgens

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: sleep in selinux_audit_rule_init
  2019-05-30 12:07               ` Stephen Smalley
  2019-05-30 12:29                 ` Paul Moore
@ 2019-05-30 13:27                 ` Janne Karhunen
  2019-05-30 14:17                   ` Stephen Smalley
  1 sibling, 1 reply; 15+ messages in thread
From: Janne Karhunen @ 2019-05-30 13:27 UTC (permalink / raw)
  To: Stephen Smalley
  Cc: Mimi Zohar, Paul Moore, linux-integrity, linux-security-module,
	Dan Jurgens

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: sleep in selinux_audit_rule_init
  2019-05-30 13:27                 ` Janne Karhunen
@ 2019-05-30 14:17                   ` Stephen Smalley
  2019-05-31 11:22                     ` Janne Karhunen
  0 siblings, 1 reply; 15+ messages in thread
From: Stephen Smalley @ 2019-05-30 14:17 UTC (permalink / raw)
  To: Janne Karhunen
  Cc: Mimi Zohar, Paul Moore, linux-integrity, linux-security-module,
	Dan Jurgens

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.



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: sleep in selinux_audit_rule_init
  2019-05-30 14:17                   ` Stephen Smalley
@ 2019-05-31 11:22                     ` Janne Karhunen
  0 siblings, 0 replies; 15+ messages in thread
From: Janne Karhunen @ 2019-05-31 11:22 UTC (permalink / raw)
  To: Stephen Smalley
  Cc: Mimi Zohar, Paul Moore, linux-integrity, linux-security-module,
	Dan Jurgens

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, back to index

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-22 11:49 sleep in selinux_audit_rule_init Janne Karhunen
2019-05-22 12:20 ` Stephen Smalley
2019-05-22 12:41   ` Stephen Smalley
2019-05-22 13:00     ` Mimi Zohar
2019-05-22 13:16       ` Stephen Smalley
2019-05-22 13:57         ` Mimi Zohar
2019-05-22 15:10           ` Casey Schaufler
2019-05-22 15:27           ` Stephen Smalley
2019-05-30 10:39             ` Janne Karhunen
2019-05-30 12:07               ` Stephen Smalley
2019-05-30 12:29                 ` Paul Moore
2019-05-30 13:27                 ` Janne Karhunen
2019-05-30 14:17                   ` Stephen Smalley
2019-05-31 11:22                     ` Janne Karhunen
2019-05-22 12:47   ` Janne Karhunen

Linux-Integrity Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-integrity/0 linux-integrity/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-integrity linux-integrity/ https://lore.kernel.org/linux-integrity \
		linux-integrity@vger.kernel.org linux-integrity@archiver.kernel.org
	public-inbox-index linux-integrity


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-integrity


AGPL code for this site: git clone https://public-inbox.org/ public-inbox