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 <pet...@mip-labs.com> 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 <pet...@mip-labs.com>
> > ---
> > 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(&ima_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, &ima_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(&ima_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, &ima_fs_flags);
> > +#endif
> > return 0;
> > }
> >
>
> May be could actually just clear a flag and leave sysfs entry anyway....
>
> #ifdef CONFIG_IMA_WRITE_POLICY
> clear_bit(IMA_FS_BUSY, &ima_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
