Re: [PATCH v4 1/3] Enable multiple writes to the IMA policy;

[Mimi Zohar]

On Sat, 2015-10-24 at 17:04 +0300, Dmitry Kasatkin wrote:
> On Sat, Oct 24, 2015 at 3:28 PM, Petko Manolov  wrote:
> > On 15-10-23 20:13:41, Dmitry Kasatkin wrote:
> >> On Fri, Oct 23, 2015 at 3:29 PM, Petko Manolov  wrote:
> >> >
> >> > I was actually going to get rid of IMA_FS_BUSY.  It is less flexible with
> >> > respect to user-space tools.  If the flag is up then the policy upload 
> >> > will
> >> > fail.  The user script or program must check the error and repeat the
> >> > operation after some time.  Seems kind of pointless to me, unless i am
> >> > missing something.
> >> >
> >> > With a semaphore the second process will block and write the policy once 
> >> > the
> >> > first writer leave the operation.  No need to repeat it unless there's 
> >> > some
> >> > real error.
> >> >
> >> > I was trying to be careful and not break the code in case the new
> >> > functionality is not selected in Kconfig.  However, with your most recent
> >> > patch-set i guess we'll have to revisit a few things. :)
> >>
> >> Obviously in original situation it will be only a "single" policy writer -
> >> which IMA init script or binary. If some other script tries to write 
> >> policy at
> >> the same time I would consider it as someone exploit would trying to do 
> >> nasty
> >> things.
> >
> > You've got a point here.  If we get rid of the busy flag we'll have to 
> > block at
> > open() instead of write() in order to keep the "original" semantics.
> >
> 
> No, that was not my point.
> The point was that there is no another/simultaneous  policy writer in reality.
> 
> >> With possibility to update policy I also do not see any need for multiple
> >> writers, when second waits first to finish update and it is not aware about
> >> coming another update. It would be some integrity manager doing policy 
> >> updates
> >> sequentially from time to time.
> >
> > Nope, that's not the only scenario.  Imagine a machine with multiple 
> > tenants and
> > huge uptime.  Imagine also that these tenants want to run their own 
> > software on
> > this machine.  Policy write may occur at any time.
> >
> 
> No, I cannot imagine that unrelated processes can write/update policy
> simultaneously at any time.
> I can imagine that some device/system management software does policy update.
> 
> May be you could give more exact use case.

His usage of the term "multi-tenant" is not the traditional one of
having to protect one tenant from another tenant, but where all tenants
are equally trusted.  Each tenant is allowed to update their own keys on
the .ima_mok/.ima keyrings system and append new rules to the policy.
The tenants are protecting against a file of unknown origin from being
executed, or possibly accessed.

Think of a system owned by a company/university with different
departments all wanting to run code on it.  Not only does the system
owner not want to get involved each time a new piece of code has to be
signed, but wants to easily be able to associate the code on the system
with a dept.  So the system owner delegates authority to the different
departments by signing their certificate.

Mimi

--
To unsubscribe from this list: send the line "unsubscribe 
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 1/3] Enable multiple writes to the IMA policy;

[Mimi Zohar]

On Tue, 2015-10-27 at 00:03 +0200, Petko Manolov wrote:
> On 15-10-26 22:39:28, Dmitry Kasatkin wrote:

> > Can you please still explain when multiple policy writers can content? I 
> > 100% 
> > understand the role of mutex
> 
> Ignore the high level requirements for the moment.  Every time you have a 
> contended resource you need to protect it from concurrent writers.  IMA 
> policy 
> is read way more frequently than it is been written.  Just once in the past, 
> now 
> a few times more.

Right.  We all agree that only one process can append new rules at a
time.  The open currently fails with -EBUSY.  If the policy isn't being
updated frequently and there isn't any contention for writing the
policy, the question is why change the existing behavior (by defining a
new mutex)?

Mimi

--
To unsubscribe from this list: send the line "unsubscribe 
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 1/3] Enable multiple writes to the IMA policy;

[Mimi Zohar]

On Mon, 2015-10-26 at 16:01 +0200, Petko Manolov wrote:
> On 15-10-25 07:50:32, Mimi Zohar wrote:
> > On Sat, 2015-10-24 at 17:06 +0300, Dmitry Kasatkin wrote:
> > 
> > > > @@ -171,9 +172,8 @@ static int __init 
> > > > default_appraise_policy_setup(char *str)
> > > >  __setup("ima_appraise_tcb", default_appraise_policy_setup);
> > > >
> > > >  /*
> > > > - * Although the IMA policy does not change, the LSM policy can be
> > > > - * reloaded, leaving the IMA LSM based rules referring to the old,
> > > > - * stale LSM policy.
> > 
> > Please don't remove this comment.
> 
> OK, i will leave it, but the beginning of the first sentence should be 
> changed 
> to reflect that the IMA policy _may_ change.

Right, please start the sentence with "The LSM policy can be ...".

> 
> > > > + * Blocking here is not legal as we hold an RCU lock.  
> > > > ima_update_policy()
> > > > + * should make it safe to walk the list at any time.
> > > >   
> > 
> > I'm not sure this comment is needed.  If it is needed, this should be moved 
> > to 
> > below the function description.
> 
> From personal experience, i like it when i see such comments, especially at 
> critical code section.  I'll move it down below if you don't like it there.

I prefer to start with the reason for the function.

> > > >   * 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.
> > > > @@ -184,7 +184,6 @@ static void ima_lsm_update_rules(void)
> > > > int result;
> > > > int i;
> > > >
> > > > -   mutex_lock(_rules_mutex);
> > > > list_for_each_entry_safe(entry, tmp, _policy_rules, list) {
> > > 
> > > 
> > > Use of "list_for_each_entry_safe" makes me having a doubt.
> > > 
> > > "safe" versions are used when entries are removed while walk.
> > > 
> > > If it is so, then entire RCU case is impossible?
> > 
> > Taking the lock here was to prevent modifying the LSM rule number multiple 
> > times.  Using the "safe" version was never needed.  The worst that can 
> > happen 
> > here is that the LSM rule number is updated multiple times.
> 
> "safe" is a remnant from the original version of the code.  I don't think it 
> is 
> necessary, but didn't want to make too many changes in one go.  In the worst 
> case the list traversal is a bit slower, which should not happen too often 
> anyway.

Right, this was my mistake.  Please feel free to change it.

Mimi

--
To unsubscribe from this list: send the line "unsubscribe 
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 1/3] Enable multiple writes to the IMA policy;

[Petko Manolov]

On 15-10-25 07:50:32, Mimi Zohar wrote:
> On Sat, 2015-10-24 at 17:06 +0300, Dmitry Kasatkin wrote:
> 
> > > @@ -171,9 +172,8 @@ static int __init default_appraise_policy_setup(char 
> > > *str)
> > >  __setup("ima_appraise_tcb", default_appraise_policy_setup);
> > >
> > >  /*
> > > - * Although the IMA policy does not change, the LSM policy can be
> > > - * reloaded, leaving the IMA LSM based rules referring to the old,
> > > - * stale LSM policy.
> 
> Please don't remove this comment.

OK, i will leave it, but the beginning of the first sentence should be changed 
to reflect that the IMA policy _may_ change.

> > > + * Blocking here is not legal as we hold an RCU lock.  
> > > ima_update_policy()
> > > + * should make it safe to walk the list at any time.
> > >   
> 
> I'm not sure this comment is needed.  If it is needed, this should be moved 
> to 
> below the function description.

>From personal experience, i like it when i see such comments, especially at 
critical code section.  I'll move it down below if you don't like it there.

> > >   * 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.
> > > @@ -184,7 +184,6 @@ static void ima_lsm_update_rules(void)
> > > int result;
> > > int i;
> > >
> > > -   mutex_lock(_rules_mutex);
> > > list_for_each_entry_safe(entry, tmp, _policy_rules, list) {
> > 
> > 
> > Use of "list_for_each_entry_safe" makes me having a doubt.
> > 
> > "safe" versions are used when entries are removed while walk.
> > 
> > If it is so, then entire RCU case is impossible?
> 
> Taking the lock here was to prevent modifying the LSM rule number multiple 
> times.  Using the "safe" version was never needed.  The worst that can happen 
> here is that the LSM rule number is updated multiple times.

"safe" is a remnant from the original version of the code.  I don't think it is 
necessary, but didn't want to make too many changes in one go.  In the worst 
case the list traversal is a bit slower, which should not happen too often 
anyway.



Petko

--
To unsubscribe from this list: send the line "unsubscribe 
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 1/3] Enable multiple writes to the IMA policy;

[Dmitry Kasatkin]

Hi,

On Fri, Oct 16, 2015 at 10:31 PM, Petko Manolov  wrote:
> IMA policy can now be updated multiple times.  The new rules get appended
> to the original policy.  Have in mind that the rules are scanned in FIFO
> order so be careful when you add new ones.
>
> The mutex locks are replaced with RCU, which should lead to faster policy
> traversals.  The new rules are first appended to a temporary list, which
> on error gets released without disturbing the normal IMA operations.
>
> Signed-off-by: Petko Manolov 
> ---
>  security/integrity/ima/Kconfig  | 14 
>  security/integrity/ima/ima_fs.c | 13 +++
>  security/integrity/ima/ima_policy.c | 70 
> +
>  3 files changed, 74 insertions(+), 23 deletions(-)
>
> diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
> index df30334..15264b7 100644
> --- a/security/integrity/ima/Kconfig
> +++ b/security/integrity/ima/Kconfig
> @@ -107,6 +107,20 @@ config IMA_DEFAULT_HASH
> default "sha512" if IMA_DEFAULT_HASH_SHA512
> default "wp512" if IMA_DEFAULT_HASH_WP512
>
> +config IMA_WRITE_POLICY
> +   bool "Enable multiple writes to the IMA policy"
> +   depends on IMA
> +   default n
> +   help
> + IMA policy can now be updated multiple times.  The new rules get
> + appended to the original policy.  Have in mind that the rules are
> + scanned in FIFO order so be careful when you add new ones.
> +
> + WARNING: Potential security hole - should be used with care in
> + production-grade kernels!
> +
> + If unsure, say N.
> +
>  config IMA_APPRAISE
> bool "Appraise integrity measurements"
> depends on IMA
> diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
> index 816d175..a3cf5c0 100644
> --- a/security/integrity/ima/ima_fs.c
> +++ b/security/integrity/ima/ima_fs.c
> @@ -25,6 +25,8 @@
>
>  #include "ima.h"
>
> +static DEFINE_MUTEX(ima_write_mutex);
> +
>  static int valid_policy = 1;
>  #define TMPBUFLEN 12
>  static ssize_t ima_show_htable_value(char __user *buf, size_t count,
> @@ -261,6 +263,11 @@ static ssize_t ima_write_policy(struct file *file, const 
> char __user *buf,
>  {
> char *data = NULL;
> ssize_t result;
> +   int res;
> +
> +   res = mutex_lock_interruptible(_write_mutex);
> +   if (res)
> +   return res;
>
> if (datalen >= PAGE_SIZE)
> datalen = PAGE_SIZE - 1;
> @@ -286,6 +293,8 @@ out:
> if (result < 0)
> valid_policy = 0;
> kfree(data);
> +   mutex_unlock(_write_mutex);
> +
> return result;
>  }
>
> @@ -337,8 +346,12 @@ static int ima_release_policy(struct inode *inode, 
> struct file *file)
> return 0;
> }
> ima_update_policy();
> +#ifndefCONFIG_IMA_WRITE_POLICY
> securityfs_remove(ima_policy);
> ima_policy = NULL;
> +#else
> +   clear_bit(IMA_FS_BUSY, _fs_flags);
> +#endif
> return 0;
>  }
>
> diff --git a/security/integrity/ima/ima_policy.c 
> b/security/integrity/ima/ima_policy.c
> index 3997e20..7ace4e4 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -16,6 +16,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>
>  #include "ima.h"
> @@ -135,11 +136,11 @@ static struct ima_rule_entry default_appraise_rules[] = 
> {
>
>  static LIST_HEAD(ima_default_rules);
>  static LIST_HEAD(ima_policy_rules);
> +static LIST_HEAD(ima_temp_rules);
>  static struct list_head *ima_rules;
>
> -static DEFINE_MUTEX(ima_rules_mutex);
> -
>  static int ima_policy __initdata;
> +
>  static int __init default_measure_policy_setup(char *str)
>  {
> if (ima_policy)
> @@ -171,9 +172,8 @@ static int __init default_appraise_policy_setup(char *str)
>  __setup("ima_appraise_tcb", default_appraise_policy_setup);
>
>  /*
> - * Although the IMA policy does not change, the LSM policy can be
> - * reloaded, leaving the IMA LSM based rules referring to the old,
> - * stale LSM policy.
> + * Blocking here is not legal as we hold an RCU lock.  ima_update_policy()
> + * should make it safe to walk the list at any time.
>   *
>   * 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.
> @@ -184,7 +184,6 @@ static void ima_lsm_update_rules(void)
> int result;
> int i;
>
> -   mutex_lock(_rules_mutex);
> list_for_each_entry_safe(entry, tmp, _policy_rules, list) {


Use of "list_for_each_entry_safe" makes me having a doubt.

"safe" versions are used when entries are removed while walk.

If it is so, then entire RCU case is impossible?


Dmitry


> for (i = 0; i < MAX_LSM_RULES; i++) {
> if (!entry->lsm[i].rule)
> @@ -196,7 +195,6 @@ static void 

Re: [PATCH v4 1/3] Enable multiple writes to the IMA policy;

[Petko Manolov]

On 15-10-23 20:13:41, Dmitry Kasatkin wrote:
> On Fri, Oct 23, 2015 at 3:29 PM, Petko Manolov  wrote:
> >
> > I was actually going to get rid of IMA_FS_BUSY.  It is less flexible with 
> > respect to user-space tools.  If the flag is up then the policy upload will 
> > fail.  The user script or program must check the error and repeat the 
> > operation after some time.  Seems kind of pointless to me, unless i am 
> > missing something.
> >
> > With a semaphore the second process will block and write the policy once 
> > the 
> > first writer leave the operation.  No need to repeat it unless there's some 
> > real error.
> >
> > I was trying to be careful and not break the code in case the new 
> > functionality is not selected in Kconfig.  However, with your most recent 
> > patch-set i guess we'll have to revisit a few things. :)
> 
> Obviously in original situation it will be only a "single" policy writer - 
> which IMA init script or binary. If some other script tries to write policy 
> at 
> the same time I would consider it as someone exploit would trying to do nasty 
> things.

You've got a point here.  If we get rid of the busy flag we'll have to block at 
open() instead of write() in order to keep the "original" semantics.

> With possibility to update policy I also do not see any need for multiple 
> writers, when second waits first to finish update and it is not aware about 
> coming another update. It would be some integrity manager doing policy 
> updates 
> sequentially from time to time.

Nope, that's not the only scenario.  Imagine a machine with multiple tenants 
and 
huge uptime.  Imagine also that these tenants want to run their own software on 
this machine.  Policy write may occur at any time.

> But with "reading" the policy file, I could imaging process blocking to wait 
> when update/read completes.

We don't need mutexes to safely read the policy.  The code that does list 
splicing is taking care of the reader either seeing the original policy or the 
new one.  Not a disjointed version of it.

Whether one will see the old or the new policy is a matter of timing and 
semaphores are not going to change this.


Petko
--
To unsubscribe from this list: send the line "unsubscribe 
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 1/3] Enable multiple writes to the IMA policy;

[Petko Manolov]

On 15-10-22 22:15:30, Dmitry Kasatkin wrote:
> Hi Petko,
> 
> I have a question
> 
> On Fri, Oct 16, 2015 at 10:31 PM, Petko Manolov  wrote:
> > IMA policy can now be updated multiple times.  The new rules get appended
> > to the original policy.  Have in mind that the rules are scanned in FIFO
> > order so be careful when you add new ones.
> >
> > The mutex locks are replaced with RCU, which should lead to faster policy
> > traversals.  The new rules are first appended to a temporary list, which
> > on error gets released without disturbing the normal IMA operations.
> >
> > Signed-off-by: Petko Manolov 
> > ---
> >  security/integrity/ima/Kconfig  | 14 
> >  security/integrity/ima/ima_fs.c | 13 +++
> >  security/integrity/ima/ima_policy.c | 70 
> > +
> >  3 files changed, 74 insertions(+), 23 deletions(-)
> >
> > diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
> > index df30334..15264b7 100644
> > --- a/security/integrity/ima/Kconfig
> > +++ b/security/integrity/ima/Kconfig
> > @@ -107,6 +107,20 @@ config IMA_DEFAULT_HASH
> > default "sha512" if IMA_DEFAULT_HASH_SHA512
> > default "wp512" if IMA_DEFAULT_HASH_WP512
> >
> > +config IMA_WRITE_POLICY
> > +   bool "Enable multiple writes to the IMA policy"
> > +   depends on IMA
> > +   default n
> > +   help
> > + IMA policy can now be updated multiple times.  The new rules get
> > + appended to the original policy.  Have in mind that the rules are
> > + scanned in FIFO order so be careful when you add new ones.
> > +
> > + WARNING: Potential security hole - should be used with care in
> > + production-grade kernels!
> > +
> > + If unsure, say N.
> > +
> >  config IMA_APPRAISE
> > bool "Appraise integrity measurements"
> > depends on IMA
> > diff --git a/security/integrity/ima/ima_fs.c 
> > b/security/integrity/ima/ima_fs.c
> > index 816d175..a3cf5c0 100644
> > --- a/security/integrity/ima/ima_fs.c
> > +++ b/security/integrity/ima/ima_fs.c
> > @@ -25,6 +25,8 @@
> >
> >  #include "ima.h"
> >
> > +static DEFINE_MUTEX(ima_write_mutex);
> > +
> >  static int valid_policy = 1;
> >  #define TMPBUFLEN 12
> >  static ssize_t ima_show_htable_value(char __user *buf, size_t count,
> > @@ -261,6 +263,11 @@ static ssize_t ima_write_policy(struct file *file, 
> > const char __user *buf,
> >  {
> > char *data = NULL;
> > ssize_t result;
> > +   int res;
> > +
> > +   res = mutex_lock_interruptible(_write_mutex);
> > +   if (res)
> > +   return res;
> >
> 
> You got rid of "ima_rules_mutex" in favor of RCU.
> 
> I wonder why 'ima_write_mutex" is needed here?
> I do not see any other uses.
> 
> ima_open_policy() provide "exclusive" access
> 
> if (test_and_set_bit(IMA_FS_BUSY, _fs_flags))
>return -EBUSY;

I was actually going to get rid of IMA_FS_BUSY.  It is less flexible with 
respect to user-space tools.  If the flag is up then the policy upload will 
fail.  The user script or program must check the error and repeat the operation 
after some time.  Seems kind of pointless to me, unless i am missing something.

With a semaphore the second process will block and write the policy once the 
first writer leave the operation.  No need to repeat it unless there's some 
real 
error.

I was trying to be careful and not break the code in case the new functionality 
is not selected in Kconfig.  However, with your most recent patch-set i guess 
we'll have to revisit a few things. :)

> > if (datalen >= PAGE_SIZE)
> > datalen = PAGE_SIZE - 1;
> > @@ -286,6 +293,8 @@ out:
> > if (result < 0)
> > valid_policy = 0;
> > kfree(data);
> > +   mutex_unlock(_write_mutex);
> > +
> > return result;
> >  }
> >
> > @@ -337,8 +346,12 @@ static int ima_release_policy(struct inode *inode, 
> > struct file *file)
> > return 0;
> > }
> > ima_update_policy();
> > +#ifndefCONFIG_IMA_WRITE_POLICY
> > securityfs_remove(ima_policy);
> > ima_policy = NULL;
> > +#else
> > +   clear_bit(IMA_FS_BUSY, _fs_flags);
> > +#endif
> > return 0;
> >  }
> >
> 
> May be could actually just clear a flag and leave sysfs entry anyway
> 
> #ifdefCONFIG_IMA_WRITE_POLICY
>clear_bit(IMA_FS_BUSY, _fs_flags);
> #endif
> 
> otherwise flag will be busy and next open fails as well...

As noted above, i'd rather be rid of this flag.


Petko
--
To unsubscribe from this list: send the line "unsubscribe 
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 1/3] Enable multiple writes to the IMA policy;

[Dmitry Kasatkin]

On Fri, Oct 23, 2015 at 3:29 PM, Petko Manolov  wrote:
> On 15-10-22 22:15:30, Dmitry Kasatkin wrote:
>> Hi Petko,
>>
>> I have a question
>>
>> On Fri, Oct 16, 2015 at 10:31 PM, Petko Manolov  wrote:
>> > IMA policy can now be updated multiple times.  The new rules get appended
>> > to the original policy.  Have in mind that the rules are scanned in FIFO
>> > order so be careful when you add new ones.
>> >
>> > The mutex locks are replaced with RCU, which should lead to faster policy
>> > traversals.  The new rules are first appended to a temporary list, which
>> > on error gets released without disturbing the normal IMA operations.
>> >
>> > Signed-off-by: Petko Manolov 
>> > ---
>> >  security/integrity/ima/Kconfig  | 14 
>> >  security/integrity/ima/ima_fs.c | 13 +++
>> >  security/integrity/ima/ima_policy.c | 70 
>> > +
>> >  3 files changed, 74 insertions(+), 23 deletions(-)
>> >
>> > diff --git a/security/integrity/ima/Kconfig 
>> > b/security/integrity/ima/Kconfig
>> > index df30334..15264b7 100644
>> > --- a/security/integrity/ima/Kconfig
>> > +++ b/security/integrity/ima/Kconfig
>> > @@ -107,6 +107,20 @@ config IMA_DEFAULT_HASH
>> > default "sha512" if IMA_DEFAULT_HASH_SHA512
>> > default "wp512" if IMA_DEFAULT_HASH_WP512
>> >
>> > +config IMA_WRITE_POLICY
>> > +   bool "Enable multiple writes to the IMA policy"
>> > +   depends on IMA
>> > +   default n
>> > +   help
>> > + IMA policy can now be updated multiple times.  The new rules get
>> > + appended to the original policy.  Have in mind that the rules are
>> > + scanned in FIFO order so be careful when you add new ones.
>> > +
>> > + WARNING: Potential security hole - should be used with care in
>> > + production-grade kernels!
>> > +
>> > + If unsure, say N.
>> > +
>> >  config IMA_APPRAISE
>> > bool "Appraise integrity measurements"
>> > depends on IMA
>> > diff --git a/security/integrity/ima/ima_fs.c 
>> > b/security/integrity/ima/ima_fs.c
>> > index 816d175..a3cf5c0 100644
>> > --- a/security/integrity/ima/ima_fs.c
>> > +++ b/security/integrity/ima/ima_fs.c
>> > @@ -25,6 +25,8 @@
>> >
>> >  #include "ima.h"
>> >
>> > +static DEFINE_MUTEX(ima_write_mutex);
>> > +
>> >  static int valid_policy = 1;
>> >  #define TMPBUFLEN 12
>> >  static ssize_t ima_show_htable_value(char __user *buf, size_t count,
>> > @@ -261,6 +263,11 @@ static ssize_t ima_write_policy(struct file *file, 
>> > const char __user *buf,
>> >  {
>> > char *data = NULL;
>> > ssize_t result;
>> > +   int res;
>> > +
>> > +   res = mutex_lock_interruptible(_write_mutex);
>> > +   if (res)
>> > +   return res;
>> >
>>
>> You got rid of "ima_rules_mutex" in favor of RCU.
>>
>> I wonder why 'ima_write_mutex" is needed here?
>> I do not see any other uses.
>>
>> ima_open_policy() provide "exclusive" access
>>
>> if (test_and_set_bit(IMA_FS_BUSY, _fs_flags))
>>return -EBUSY;
>
> I was actually going to get rid of IMA_FS_BUSY.  It is less flexible with
> respect to user-space tools.  If the flag is up then the policy upload will
> fail.  The user script or program must check the error and repeat the 
> operation
> after some time.  Seems kind of pointless to me, unless i am missing 
> something.
>
> With a semaphore the second process will block and write the policy once the
> first writer leave the operation.  No need to repeat it unless there's some 
> real
> error.
>
> I was trying to be careful and not break the code in case the new 
> functionality
> is not selected in Kconfig.  However, with your most recent patch-set i guess
> we'll have to revisit a few things. :)

Obviously in original situation it will be only a "single" policy
writer - which IMA init script or binary.
If some other script tries to write policy at the same time I would
consider it as someone exploit would trying to do nasty things.

With possibility to update policy I also do not see any need for
multiple writers, when second waits first to finish update and it is
not aware about coming another update. It would be some integrity
manager doing policy updates sequentially from time to time.

But with "reading" the policy file, I could imaging process blocking
to wait when update/read completes.

Dmtry


>
>> > if (datalen >= PAGE_SIZE)
>> > datalen = PAGE_SIZE - 1;
>> > @@ -286,6 +293,8 @@ out:
>> > if (result < 0)
>> > valid_policy = 0;
>> > kfree(data);
>> > +   mutex_unlock(_write_mutex);
>> > +
>> > return result;
>> >  }
>> >
>> > @@ -337,8 +346,12 @@ static int ima_release_policy(struct inode *inode, 
>> > struct file *file)
>> > return 0;
>> > }
>> > ima_update_policy();
>> > +#ifndefCONFIG_IMA_WRITE_POLICY
>> > 

Re: [PATCH v4 1/3] Enable multiple writes to the IMA policy;

[Dmitry Kasatkin]

Hi Petko,

I have a question

On Fri, Oct 16, 2015 at 10:31 PM, Petko Manolov  wrote:
> IMA policy can now be updated multiple times.  The new rules get appended
> to the original policy.  Have in mind that the rules are scanned in FIFO
> order so be careful when you add new ones.
>
> The mutex locks are replaced with RCU, which should lead to faster policy
> traversals.  The new rules are first appended to a temporary list, which
> on error gets released without disturbing the normal IMA operations.
>
> Signed-off-by: Petko Manolov 
> ---
>  security/integrity/ima/Kconfig  | 14 
>  security/integrity/ima/ima_fs.c | 13 +++
>  security/integrity/ima/ima_policy.c | 70 
> +
>  3 files changed, 74 insertions(+), 23 deletions(-)
>
> diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
> index df30334..15264b7 100644
> --- a/security/integrity/ima/Kconfig
> +++ b/security/integrity/ima/Kconfig
> @@ -107,6 +107,20 @@ config IMA_DEFAULT_HASH
> default "sha512" if IMA_DEFAULT_HASH_SHA512
> default "wp512" if IMA_DEFAULT_HASH_WP512
>
> +config IMA_WRITE_POLICY
> +   bool "Enable multiple writes to the IMA policy"
> +   depends on IMA
> +   default n
> +   help
> + IMA policy can now be updated multiple times.  The new rules get
> + appended to the original policy.  Have in mind that the rules are
> + scanned in FIFO order so be careful when you add new ones.
> +
> + WARNING: Potential security hole - should be used with care in
> + production-grade kernels!
> +
> + If unsure, say N.
> +
>  config IMA_APPRAISE
> bool "Appraise integrity measurements"
> depends on IMA
> diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
> index 816d175..a3cf5c0 100644
> --- a/security/integrity/ima/ima_fs.c
> +++ b/security/integrity/ima/ima_fs.c
> @@ -25,6 +25,8 @@
>
>  #include "ima.h"
>
> +static DEFINE_MUTEX(ima_write_mutex);
> +
>  static int valid_policy = 1;
>  #define TMPBUFLEN 12
>  static ssize_t ima_show_htable_value(char __user *buf, size_t count,
> @@ -261,6 +263,11 @@ static ssize_t ima_write_policy(struct file *file, const 
> char __user *buf,
>  {
> char *data = NULL;
> ssize_t result;
> +   int res;
> +
> +   res = mutex_lock_interruptible(_write_mutex);
> +   if (res)
> +   return res;
>

You got rid of "ima_rules_mutex" in favor of RCU.

I wonder why 'ima_write_mutex" is needed here?
I do not see any other uses.

ima_open_policy() provide "exclusive" access

if (test_and_set_bit(IMA_FS_BUSY, _fs_flags))
   return -EBUSY;



> if (datalen >= PAGE_SIZE)
> datalen = PAGE_SIZE - 1;
> @@ -286,6 +293,8 @@ out:
> if (result < 0)
> valid_policy = 0;
> kfree(data);
> +   mutex_unlock(_write_mutex);
> +
> return result;
>  }
>
> @@ -337,8 +346,12 @@ static int ima_release_policy(struct inode *inode, 
> struct file *file)
> return 0;
> }
> ima_update_policy();
> +#ifndefCONFIG_IMA_WRITE_POLICY
> securityfs_remove(ima_policy);
> ima_policy = NULL;
> +#else
> +   clear_bit(IMA_FS_BUSY, _fs_flags);
> +#endif
> return 0;
>  }
>

May be could actually just clear a flag and leave sysfs entry anyway

#ifdefCONFIG_IMA_WRITE_POLICY
   clear_bit(IMA_FS_BUSY, _fs_flags);
#endif

otherwise flag will be busy and next open fails as well...


Dmitry

> diff --git a/security/integrity/ima/ima_policy.c 
> b/security/integrity/ima/ima_policy.c
> index 3997e20..7ace4e4 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -16,6 +16,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>
>  #include "ima.h"
> @@ -135,11 +136,11 @@ static struct ima_rule_entry default_appraise_rules[] = 
> {
>
>  static LIST_HEAD(ima_default_rules);
>  static LIST_HEAD(ima_policy_rules);
> +static LIST_HEAD(ima_temp_rules);
>  static struct list_head *ima_rules;
>
> -static DEFINE_MUTEX(ima_rules_mutex);
> -
>  static int ima_policy __initdata;
> +
>  static int __init default_measure_policy_setup(char *str)
>  {
> if (ima_policy)
> @@ -171,9 +172,8 @@ static int __init default_appraise_policy_setup(char *str)
>  __setup("ima_appraise_tcb", default_appraise_policy_setup);
>
>  /*
> - * Although the IMA policy does not change, the LSM policy can be
> - * reloaded, leaving the IMA LSM based rules referring to the old,
> - * stale LSM policy.
> + * Blocking here is not legal as we hold an RCU lock.  ima_update_policy()
> + * should make it safe to walk the list at any time.
>   *
>   * 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.
> @@ -184,7 +184,6 @@ static void 

Re: [PATCH v4 1/3] Enable multiple writes to the IMA policy;

[Petko Manolov]

On 15-10-19 14:21:03, Mimi Zohar wrote:
> On Fri, 2015-10-16 at 22:31 +0300, Petko Manolov wrote:
> > IMA policy can now be updated multiple times.  The new rules get appended
> > to the original policy.  Have in mind that the rules are scanned in FIFO
> > order so be careful when you add new ones.
> > 
> > The mutex locks are replaced with RCU, which should lead to faster policy
> > traversals.  The new rules are first appended to a temporary list, which
> > on error gets released without disturbing the normal IMA operations.
> 
> There was no need for locking in the original version.  This comment should 
> be 
> included in a change log to reflect different versions of the patch.

Yep, a message change is in order.


Petko


> > Signed-off-by: Petko Manolov  ---
> >  security/integrity/ima/Kconfig  | 14 
> >  security/integrity/ima/ima_fs.c | 13 +++
> >  security/integrity/ima/ima_policy.c | 70 
> > +
> >  3 files changed, 74 insertions(+), 23 deletions(-)
> > 
> > diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
> > index df30334..15264b7 100644
> > --- a/security/integrity/ima/Kconfig
> > +++ b/security/integrity/ima/Kconfig
> > @@ -107,6 +107,20 @@ config IMA_DEFAULT_HASH
> > default "sha512" if IMA_DEFAULT_HASH_SHA512
> > default "wp512" if IMA_DEFAULT_HASH_WP512
> > 
> > +config IMA_WRITE_POLICY
> > +   bool "Enable multiple writes to the IMA policy"
> > +   depends on IMA
> > +   default n
> > +   help
> > + IMA policy can now be updated multiple times.  The new rules get
> > + appended to the original policy.  Have in mind that the rules are
> > + scanned in FIFO order so be careful when you add new ones.
> > +
> > + WARNING: Potential security hole - should be used with care in
> > + production-grade kernels!
> > +
> > + If unsure, say N.
> > +
> >  config IMA_APPRAISE
> > bool "Appraise integrity measurements"
> > depends on IMA
> > diff --git a/security/integrity/ima/ima_fs.c 
> > b/security/integrity/ima/ima_fs.c
> > index 816d175..a3cf5c0 100644
> > --- a/security/integrity/ima/ima_fs.c
> > +++ b/security/integrity/ima/ima_fs.c
> > @@ -25,6 +25,8 @@
> > 
> >  #include "ima.h"
> > 
> > +static DEFINE_MUTEX(ima_write_mutex);
> > +
> >  static int valid_policy = 1;
> >  #define TMPBUFLEN 12
> >  static ssize_t ima_show_htable_value(char __user *buf, size_t count,
> > @@ -261,6 +263,11 @@ static ssize_t ima_write_policy(struct file *file, 
> > const char __user *buf,
> >  {
> > char *data = NULL;
> > ssize_t result;
> > +   int res;
> > +
> > +   res = mutex_lock_interruptible(_write_mutex);
> > +   if (res)
> > +   return res;
> > 
> > if (datalen >= PAGE_SIZE)
> > datalen = PAGE_SIZE - 1;
> > @@ -286,6 +293,8 @@ out:
> > if (result < 0)
> > valid_policy = 0;
> > kfree(data);
> > +   mutex_unlock(_write_mutex);
> > +
> > return result;
> >  }
> > 
> > @@ -337,8 +346,12 @@ static int ima_release_policy(struct inode *inode, 
> > struct file *file)
> > return 0;
> > }
> > ima_update_policy();
> > +#ifndefCONFIG_IMA_WRITE_POLICY
> > securityfs_remove(ima_policy);
> > ima_policy = NULL;
> > +#else
> > +   clear_bit(IMA_FS_BUSY, _fs_flags);
> > +#endif
> > return 0;
> >  }
> > 
> > diff --git a/security/integrity/ima/ima_policy.c 
> > b/security/integrity/ima/ima_policy.c
> > index 3997e20..7ace4e4 100644
> > --- a/security/integrity/ima/ima_policy.c
> > +++ b/security/integrity/ima/ima_policy.c
> > @@ -16,6 +16,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  #include 
> > 
> >  #include "ima.h"
> > @@ -135,11 +136,11 @@ static struct ima_rule_entry default_appraise_rules[] 
> > = {
> > 
> >  static LIST_HEAD(ima_default_rules);
> >  static LIST_HEAD(ima_policy_rules);
> > +static LIST_HEAD(ima_temp_rules);
> >  static struct list_head *ima_rules;
> > 
> > -static DEFINE_MUTEX(ima_rules_mutex);
> > -
> >  static int ima_policy __initdata;
> > +
> >  static int __init default_measure_policy_setup(char *str)
> >  {
> > if (ima_policy)
> > @@ -171,9 +172,8 @@ static int __init default_appraise_policy_setup(char 
> > *str)
> >  __setup("ima_appraise_tcb", default_appraise_policy_setup);
> > 
> >  /*
> > - * Although the IMA policy does not change, the LSM policy can be
> > - * reloaded, leaving the IMA LSM based rules referring to the old,
> > - * stale LSM policy.
> > + * Blocking here is not legal as we hold an RCU lock.  ima_update_policy()
> > + * should make it safe to walk the list at any time.
> >   *
> >   * 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.
> > @@ -184,7 +184,6 @@ static void ima_lsm_update_rules(void)
> > int result;
> > int i;
> > 
> > -   mutex_lock(_rules_mutex);
> > list_for_each_entry_safe(entry, tmp, 

Re: [PATCH v4 1/3] Enable multiple writes to the IMA policy;

[Petko Manolov]

On 15-10-19 18:28:22, Mimi Zohar wrote:
> On Mon, 2015-10-19 at 14:21 -0400, Mimi Zohar wrote:
> > On Fri, 2015-10-16 at 22:31 +0300, Petko Manolov wrote:
> 
> > > diff --git a/security/integrity/ima/ima_fs.c 
> > > b/security/integrity/ima/ima_fs.c
> > > index 816d175..a3cf5c0 100644
> > > --- a/security/integrity/ima/ima_fs.c
> > > +++ b/security/integrity/ima/ima_fs.c
> > > @@ -25,6 +25,8 @@
> > > 
> > >  #include "ima.h"
> > > 
> > > +static DEFINE_MUTEX(ima_write_mutex);
> > > +
> > >  static int valid_policy = 1;
> > >  #define TMPBUFLEN 12
> > >  static ssize_t ima_show_htable_value(char __user *buf, size_t count,
> > > @@ -261,6 +263,11 @@ static ssize_t ima_write_policy(struct file *file, 
> > > const char __user *buf,
> > >  {
> > >   char *data = NULL;
> > >   ssize_t result;
> > > + int res;
> > > +
> > > + res = mutex_lock_interruptible(_write_mutex);
> > > + if (res)
> > > + return res;
> > > 
> > >   if (datalen >= PAGE_SIZE)
> > >   datalen = PAGE_SIZE - 1;
> > > @@ -286,6 +293,8 @@ out:
> > >   if (result < 0)
> > >   valid_policy = 0;
> > >   kfree(data);
> > > + mutex_unlock(_write_mutex);
> > > +
> > >   return result;
> > >  }
> > > 
> > > @@ -337,8 +346,12 @@ static int ima_release_policy(struct inode *inode, 
> > > struct file *file)
> > >   return 0;
> > >   }
> > >   ima_update_policy();
> > > +#ifndef  CONFIG_IMA_WRITE_POLICY
> > >   securityfs_remove(ima_policy);
> > >   ima_policy = NULL;
> > > +#else
> > > + clear_bit(IMA_FS_BUSY, _fs_flags);
> > > +#endif
> > >   return 0;
> > >  }
> > > 
> 
> The IMA_FS_BUSY flag needs to be cleared, like here, above for !
> valid_policy.

Good catch.  Fixed.


Petko
--
To unsubscribe from this list: send the line "unsubscribe 
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 1/3] Enable multiple writes to the IMA policy;

[Mimi Zohar]

On Mon, 2015-10-19 at 14:21 -0400, Mimi Zohar wrote:
> On Fri, 2015-10-16 at 22:31 +0300, Petko Manolov wrote:

> > diff --git a/security/integrity/ima/ima_fs.c 
> > b/security/integrity/ima/ima_fs.c
> > index 816d175..a3cf5c0 100644
> > --- a/security/integrity/ima/ima_fs.c
> > +++ b/security/integrity/ima/ima_fs.c
> > @@ -25,6 +25,8 @@
> > 
> >  #include "ima.h"
> > 
> > +static DEFINE_MUTEX(ima_write_mutex);
> > +
> >  static int valid_policy = 1;
> >  #define TMPBUFLEN 12
> >  static ssize_t ima_show_htable_value(char __user *buf, size_t count,
> > @@ -261,6 +263,11 @@ static ssize_t ima_write_policy(struct file *file, 
> > const char __user *buf,
> >  {
> > char *data = NULL;
> > ssize_t result;
> > +   int res;
> > +
> > +   res = mutex_lock_interruptible(_write_mutex);
> > +   if (res)
> > +   return res;
> > 
> > if (datalen >= PAGE_SIZE)
> > datalen = PAGE_SIZE - 1;
> > @@ -286,6 +293,8 @@ out:
> > if (result < 0)
> > valid_policy = 0;
> > kfree(data);
> > +   mutex_unlock(_write_mutex);
> > +
> > return result;
> >  }
> > 
> > @@ -337,8 +346,12 @@ static int ima_release_policy(struct inode *inode, 
> > struct file *file)
> > return 0;
> > }
> > ima_update_policy();
> > +#ifndefCONFIG_IMA_WRITE_POLICY
> > securityfs_remove(ima_policy);
> > ima_policy = NULL;
> > +#else
> > +   clear_bit(IMA_FS_BUSY, _fs_flags);
> > +#endif
> > return 0;
> >  }
> > 

The IMA_FS_BUSY flag needs to be cleared, like here, above for !
valid_policy.

Mimi

--
To unsubscribe from this list: send the line "unsubscribe 
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 1/3] Enable multiple writes to the IMA policy;

[Petko Manolov]

IMA policy can now be updated multiple times.  The new rules get appended
to the original policy.  Have in mind that the rules are scanned in FIFO
order so be careful when you add new ones.

The mutex locks are replaced with RCU, which should lead to faster policy
traversals.  The new rules are first appended to a temporary list, which
on error gets released without disturbing the normal IMA operations.

Signed-off-by: Petko Manolov 
---
 security/integrity/ima/Kconfig  | 14 
 security/integrity/ima/ima_fs.c | 13 +++
 security/integrity/ima/ima_policy.c | 70 +
 3 files changed, 74 insertions(+), 23 deletions(-)

diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
index df30334..15264b7 100644
--- a/security/integrity/ima/Kconfig
+++ b/security/integrity/ima/Kconfig
@@ -107,6 +107,20 @@ config IMA_DEFAULT_HASH
default "sha512" if IMA_DEFAULT_HASH_SHA512
default "wp512" if IMA_DEFAULT_HASH_WP512
 
+config IMA_WRITE_POLICY
+   bool "Enable multiple writes to the IMA policy"
+   depends on IMA
+   default n
+   help
+ IMA policy can now be updated multiple times.  The new rules get
+ appended to the original policy.  Have in mind that the rules are
+ scanned in FIFO order so be careful when you add new ones.
+
+ WARNING: Potential security hole - should be used with care in
+ production-grade kernels!
+
+ If unsure, say N.
+
 config IMA_APPRAISE
bool "Appraise integrity measurements"
depends on IMA
diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
index 816d175..a3cf5c0 100644
--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c
@@ -25,6 +25,8 @@
 
 #include "ima.h"
 
+static DEFINE_MUTEX(ima_write_mutex);
+
 static int valid_policy = 1;
 #define TMPBUFLEN 12
 static ssize_t ima_show_htable_value(char __user *buf, size_t count,
@@ -261,6 +263,11 @@ static ssize_t ima_write_policy(struct file *file, const 
char __user *buf,
 {
char *data = NULL;
ssize_t result;
+   int res;
+
+   res = mutex_lock_interruptible(_write_mutex);
+   if (res)
+   return res;
 
if (datalen >= PAGE_SIZE)
datalen = PAGE_SIZE - 1;
@@ -286,6 +293,8 @@ out:
if (result < 0)
valid_policy = 0;
kfree(data);
+   mutex_unlock(_write_mutex);
+
return result;
 }
 
@@ -337,8 +346,12 @@ static int ima_release_policy(struct inode *inode, struct 
file *file)
return 0;
}
ima_update_policy();
+#ifndefCONFIG_IMA_WRITE_POLICY
securityfs_remove(ima_policy);
ima_policy = NULL;
+#else
+   clear_bit(IMA_FS_BUSY, _fs_flags);
+#endif
return 0;
 }
 
diff --git a/security/integrity/ima/ima_policy.c 
b/security/integrity/ima/ima_policy.c
index 3997e20..7ace4e4 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -16,6 +16,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 
 #include "ima.h"
@@ -135,11 +136,11 @@ static struct ima_rule_entry default_appraise_rules[] = {
 
 static LIST_HEAD(ima_default_rules);
 static LIST_HEAD(ima_policy_rules);
+static LIST_HEAD(ima_temp_rules);
 static struct list_head *ima_rules;
 
-static DEFINE_MUTEX(ima_rules_mutex);
-
 static int ima_policy __initdata;
+
 static int __init default_measure_policy_setup(char *str)
 {
if (ima_policy)
@@ -171,9 +172,8 @@ static int __init default_appraise_policy_setup(char *str)
 __setup("ima_appraise_tcb", default_appraise_policy_setup);
 
 /*
- * Although the IMA policy does not change, the LSM policy can be
- * reloaded, leaving the IMA LSM based rules referring to the old,
- * stale LSM policy.
+ * Blocking here is not legal as we hold an RCU lock.  ima_update_policy()
+ * should make it safe to walk the list at any time.
  *
  * 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.
@@ -184,7 +184,6 @@ static void ima_lsm_update_rules(void)
int result;
int i;
 
-   mutex_lock(_rules_mutex);
list_for_each_entry_safe(entry, tmp, _policy_rules, list) {
for (i = 0; i < MAX_LSM_RULES; i++) {
if (!entry->lsm[i].rule)
@@ -196,7 +195,6 @@ static void ima_lsm_update_rules(void)
BUG_ON(!entry->lsm[i].rule);
}
}
-   mutex_unlock(_rules_mutex);
 }
 
 /**
@@ -319,9 +317,9 @@ static int get_subaction(struct ima_rule_entry *rule, int 
func)
  * Measure decision based on func/mask/fsmagic and LSM(subj/obj/type)
  * conditions.
  *
- * (There is no need for locking when walking the policy list,
- * as elements in the list are never deleted, nor does the list
- * change.)
+ * Since the IMA policy may be updated multiple times we need to