diff mbox series

[v5,22/23] integrity: Move integrity functions to the LSM infrastructure

Message ID 20231107134012.682009-23-roberto.sassu@huaweicloud.com
State New
Headers
Series security: Move IMA and EVM to the LSM infrastructure |

Commit Message

Roberto Sassu Nov. 7, 2023, 1:40 p.m. UTC 
  From: Roberto Sassu <roberto.sassu@huawei.com>

Remove hardcoded calls to integrity functions from the LSM infrastructure
and, instead, register them in integrity_lsm_init() with the IMA or EVM
LSM ID (the first non-NULL returned by ima_get_lsm_id() and
evm_get_lsm_id()).

Also move the global declaration of integrity_inode_get() to
security/integrity/integrity.h, so that the function can be still called by
IMA.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 include/linux/integrity.h      | 26 --------------------------
 security/integrity/iint.c      | 30 +++++++++++++++++++++++++++++-
 security/integrity/integrity.h |  7 +++++++
 security/security.c            |  9 +--------
 4 files changed, 37 insertions(+), 35 deletions(-)

Comments

Casey Schaufler Nov. 7, 2023, 6:46 p.m. UTC | #1 
On 11/7/2023 5:40 AM, Roberto Sassu wrote:
> From: Roberto Sassu <roberto.sassu@huawei.com>
>
> Remove hardcoded calls to integrity functions from the LSM infrastructure
> and, instead, register them in integrity_lsm_init() with the IMA or EVM
> LSM ID (the first non-NULL returned by ima_get_lsm_id() and
> evm_get_lsm_id()).
>
> Also move the global declaration of integrity_inode_get() to
> security/integrity/integrity.h, so that the function can be still called by
> IMA.
>
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>

Reviewed-by: Casey Schaufler <casey@schaufler-ca.com>


> ---
>  include/linux/integrity.h      | 26 --------------------------
>  security/integrity/iint.c      | 30 +++++++++++++++++++++++++++++-
>  security/integrity/integrity.h |  7 +++++++
>  security/security.c            |  9 +--------
>  4 files changed, 37 insertions(+), 35 deletions(-)
>
> diff --git a/include/linux/integrity.h b/include/linux/integrity.h
> index 2ea0f2f65ab6..afaae7ad26f4 100644
> --- a/include/linux/integrity.h
> +++ b/include/linux/integrity.h
> @@ -21,38 +21,12 @@ enum integrity_status {
>  
>  /* List of EVM protected security xattrs */
>  #ifdef CONFIG_INTEGRITY
> -extern struct integrity_iint_cache *integrity_inode_get(struct inode *inode);
> -extern void integrity_inode_free(struct inode *inode);
>  extern void __init integrity_load_keys(void);
>  
>  #else
> -static inline struct integrity_iint_cache *
> -				integrity_inode_get(struct inode *inode)
> -{
> -	return NULL;
> -}
> -
> -static inline void integrity_inode_free(struct inode *inode)
> -{
> -	return;
> -}
> -
>  static inline void integrity_load_keys(void)
>  {
>  }
>  #endif /* CONFIG_INTEGRITY */
>  
> -#ifdef CONFIG_INTEGRITY_ASYMMETRIC_KEYS
> -
> -extern int integrity_kernel_module_request(char *kmod_name);
> -
> -#else
> -
> -static inline int integrity_kernel_module_request(char *kmod_name)
> -{
> -	return 0;
> -}
> -
> -#endif /* CONFIG_INTEGRITY_ASYMMETRIC_KEYS */
> -
>  #endif /* _LINUX_INTEGRITY_H */
> diff --git a/security/integrity/iint.c b/security/integrity/iint.c
> index 0b0ac71142e8..882fde2a2607 100644
> --- a/security/integrity/iint.c
> +++ b/security/integrity/iint.c
> @@ -171,7 +171,7 @@ struct integrity_iint_cache *integrity_inode_get(struct inode *inode)
>   *
>   * Free the integrity information(iint) associated with an inode.
>   */
> -void integrity_inode_free(struct inode *inode)
> +static void integrity_inode_free(struct inode *inode)
>  {
>  	struct integrity_iint_cache *iint;
>  
> @@ -193,11 +193,39 @@ static void iint_init_once(void *foo)
>  	memset(iint, 0, sizeof(*iint));
>  }
>  
> +static struct security_hook_list integrity_hooks[] __ro_after_init = {
> +	LSM_HOOK_INIT(inode_free_security, integrity_inode_free),
> +#ifdef CONFIG_INTEGRITY_ASYMMETRIC_KEYS
> +	LSM_HOOK_INIT(kernel_module_request, integrity_kernel_module_request),
> +#endif
> +};
> +
> +/*
> + * Perform the initialization of the 'integrity', 'ima' and 'evm' LSMs to
> + * ensure that the management of integrity metadata is working at the time
> + * IMA and EVM hooks are registered to the LSM infrastructure, and to keep
> + * the original ordering of IMA and EVM functions as when they were hardcoded.
> + */
>  static int __init integrity_lsm_init(void)
>  {
> +	const struct lsm_id *lsmid;
> +
>  	iint_cache =
>  	    kmem_cache_create("iint_cache", sizeof(struct integrity_iint_cache),
>  			      0, SLAB_PANIC, iint_init_once);
> +	/*
> +	 * Obtain either the IMA or EVM LSM ID to register integrity-specific
> +	 * hooks under that LSM, since there is no LSM ID assigned to the
> +	 * 'integrity' LSM.
> +	 */
> +	lsmid = ima_get_lsm_id();
> +	if (!lsmid)
> +		lsmid = evm_get_lsm_id();
> +	/* No point in continuing, since both IMA and EVM are disabled. */
> +	if (!lsmid)
> +		return 0;
> +
> +	security_add_hooks(integrity_hooks, ARRAY_SIZE(integrity_hooks), lsmid);
>  	init_ima_lsm();
>  	init_evm_lsm();
>  	return 0;
> diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
> index 7534ec06324e..e4df82d6f6e7 100644
> --- a/security/integrity/integrity.h
> +++ b/security/integrity/integrity.h
> @@ -180,6 +180,7 @@ struct integrity_iint_cache {
>   * integrity data associated with an inode.
>   */
>  struct integrity_iint_cache *integrity_iint_find(struct inode *inode);
> +struct integrity_iint_cache *integrity_inode_get(struct inode *inode);
>  
>  int integrity_kernel_read(struct file *file, loff_t offset,
>  			  void *addr, unsigned long count);
> @@ -266,12 +267,18 @@ static inline int __init integrity_load_cert(const unsigned int id,
>  #ifdef CONFIG_INTEGRITY_ASYMMETRIC_KEYS
>  int asymmetric_verify(struct key *keyring, const char *sig,
>  		      int siglen, const char *data, int datalen);
> +int integrity_kernel_module_request(char *kmod_name);
>  #else
>  static inline int asymmetric_verify(struct key *keyring, const char *sig,
>  				    int siglen, const char *data, int datalen)
>  {
>  	return -EOPNOTSUPP;
>  }
> +
> +static inline int integrity_kernel_module_request(char *kmod_name)
> +{
> +	return 0;
> +}
>  #endif
>  
>  #ifdef CONFIG_IMA_APPRAISE_MODSIG
> diff --git a/security/security.c b/security/security.c
> index 9703549b6ddc..0d9eaa4cd260 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -19,7 +19,6 @@
>  #include <linux/kernel.h>
>  #include <linux/kernel_read_file.h>
>  #include <linux/lsm_hooks.h>
> -#include <linux/integrity.h>
>  #include <linux/fsnotify.h>
>  #include <linux/mman.h>
>  #include <linux/mount.h>
> @@ -1597,7 +1596,6 @@ static void inode_free_by_rcu(struct rcu_head *head)
>   */
>  void security_inode_free(struct inode *inode)
>  {
> -	integrity_inode_free(inode);
>  	call_void_hook(inode_free_security, inode);
>  	/*
>  	 * The inode may still be referenced in a path walk and
> @@ -3182,12 +3180,7 @@ int security_kernel_create_files_as(struct cred *new, struct inode *inode)
>   */
>  int security_kernel_module_request(char *kmod_name)
>  {
> -	int ret;
> -
> -	ret = call_int_hook(kernel_module_request, 0, kmod_name);
> -	if (ret)
> -		return ret;
> -	return integrity_kernel_module_request(kmod_name);
> +	return call_int_hook(kernel_module_request, 0, kmod_name);
>  }
>  
>  /**
Paul Moore Nov. 16, 2023, 4:33 a.m. UTC | #2 
On Nov  7, 2023 Roberto Sassu <roberto.sassu@huaweicloud.com> wrote:
> 
> Remove hardcoded calls to integrity functions from the LSM infrastructure
> and, instead, register them in integrity_lsm_init() with the IMA or EVM
> LSM ID (the first non-NULL returned by ima_get_lsm_id() and
> evm_get_lsm_id()).
> 
> Also move the global declaration of integrity_inode_get() to
> security/integrity/integrity.h, so that the function can be still called by
> IMA.
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> Reviewed-by: Casey Schaufler <casey@schaufler-ca.com>
> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
> ---
>  include/linux/integrity.h      | 26 --------------------------
>  security/integrity/iint.c      | 30 +++++++++++++++++++++++++++++-
>  security/integrity/integrity.h |  7 +++++++
>  security/security.c            |  9 +--------
>  4 files changed, 37 insertions(+), 35 deletions(-)

...

> diff --git a/security/integrity/iint.c b/security/integrity/iint.c
> index 0b0ac71142e8..882fde2a2607 100644
> --- a/security/integrity/iint.c
> +++ b/security/integrity/iint.c
> @@ -171,7 +171,7 @@ struct integrity_iint_cache *integrity_inode_get(struct inode *inode)
>   *
>   * Free the integrity information(iint) associated with an inode.
>   */
> -void integrity_inode_free(struct inode *inode)
> +static void integrity_inode_free(struct inode *inode)
>  {
>  	struct integrity_iint_cache *iint;
>  
> @@ -193,11 +193,39 @@ static void iint_init_once(void *foo)
>  	memset(iint, 0, sizeof(*iint));
>  }
>  
> +static struct security_hook_list integrity_hooks[] __ro_after_init = {
> +	LSM_HOOK_INIT(inode_free_security, integrity_inode_free),
> +#ifdef CONFIG_INTEGRITY_ASYMMETRIC_KEYS
> +	LSM_HOOK_INIT(kernel_module_request, integrity_kernel_module_request),
> +#endif
> +};
> +
> +/*
> + * Perform the initialization of the 'integrity', 'ima' and 'evm' LSMs to
> + * ensure that the management of integrity metadata is working at the time
> + * IMA and EVM hooks are registered to the LSM infrastructure, and to keep
> + * the original ordering of IMA and EVM functions as when they were hardcoded.
> + */
>  static int __init integrity_lsm_init(void)
>  {
> +	const struct lsm_id *lsmid;
> +
>  	iint_cache =
>  	    kmem_cache_create("iint_cache", sizeof(struct integrity_iint_cache),
>  			      0, SLAB_PANIC, iint_init_once);
> +	/*
> +	 * Obtain either the IMA or EVM LSM ID to register integrity-specific
> +	 * hooks under that LSM, since there is no LSM ID assigned to the
> +	 * 'integrity' LSM.
> +	 */
> +	lsmid = ima_get_lsm_id();
> +	if (!lsmid)
> +		lsmid = evm_get_lsm_id();
> +	/* No point in continuing, since both IMA and EVM are disabled. */
> +	if (!lsmid)
> +		return 0;
> +
> +	security_add_hooks(integrity_hooks, ARRAY_SIZE(integrity_hooks), lsmid);

Ooof.  I understand, or at least I think I understand, why the above
hack is needed, but I really don't like the idea of @integrity_hooks
jumping between IMA and EVM depending on how the kernel is configured.

Just to make sure I'm understanding things correctly, the "integrity"
LSM exists to ensure the proper hook ordering between IMA/EVM, shared
metadata management for IMA/EVM, and a little bit of a hack to solve
some kernel module loading issues with signatures.  Is that correct?

I see that patch 23/23 makes some nice improvements to the metadata
management, moving them into LSM security blobs, but it appears that
they are still shared, and thus the requirement is still there for
an "integrity" LSM to manage the shared blobs.

I'd like to hear everyone's honest opinion on this next question: do
we have any hope of separating IMA and EVM so they are independent
(ignore the ordering issues for a moment), or are we always going to
need to have the "integrity" LSM to manage shared resources, hooks,
etc.?

>  	init_ima_lsm();
>  	init_evm_lsm();
>  	return 0;

--
paul-moore.com
Roberto Sassu Nov. 16, 2023, 10:07 a.m. UTC | #3 
On Wed, 2023-11-15 at 23:33 -0500, Paul Moore wrote:
> On Nov  7, 2023 Roberto Sassu <roberto.sassu@huaweicloud.com> wrote:
> > 
> > Remove hardcoded calls to integrity functions from the LSM infrastructure
> > and, instead, register them in integrity_lsm_init() with the IMA or EVM
> > LSM ID (the first non-NULL returned by ima_get_lsm_id() and
> > evm_get_lsm_id()).
> > 
> > Also move the global declaration of integrity_inode_get() to
> > security/integrity/integrity.h, so that the function can be still called by
> > IMA.
> > 
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > Reviewed-by: Casey Schaufler <casey@schaufler-ca.com>
> > Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
> > ---
> >  include/linux/integrity.h      | 26 --------------------------
> >  security/integrity/iint.c      | 30 +++++++++++++++++++++++++++++-
> >  security/integrity/integrity.h |  7 +++++++
> >  security/security.c            |  9 +--------
> >  4 files changed, 37 insertions(+), 35 deletions(-)
> 
> ...
> 
> > diff --git a/security/integrity/iint.c b/security/integrity/iint.c
> > index 0b0ac71142e8..882fde2a2607 100644
> > --- a/security/integrity/iint.c
> > +++ b/security/integrity/iint.c
> > @@ -171,7 +171,7 @@ struct integrity_iint_cache *integrity_inode_get(struct inode *inode)
> >   *
> >   * Free the integrity information(iint) associated with an inode.
> >   */
> > -void integrity_inode_free(struct inode *inode)
> > +static void integrity_inode_free(struct inode *inode)
> >  {
> >  	struct integrity_iint_cache *iint;
> >  
> > @@ -193,11 +193,39 @@ static void iint_init_once(void *foo)
> >  	memset(iint, 0, sizeof(*iint));
> >  }
> >  
> > +static struct security_hook_list integrity_hooks[] __ro_after_init = {
> > +	LSM_HOOK_INIT(inode_free_security, integrity_inode_free),
> > +#ifdef CONFIG_INTEGRITY_ASYMMETRIC_KEYS
> > +	LSM_HOOK_INIT(kernel_module_request, integrity_kernel_module_request),
> > +#endif
> > +};
> > +
> > +/*
> > + * Perform the initialization of the 'integrity', 'ima' and 'evm' LSMs to
> > + * ensure that the management of integrity metadata is working at the time
> > + * IMA and EVM hooks are registered to the LSM infrastructure, and to keep
> > + * the original ordering of IMA and EVM functions as when they were hardcoded.
> > + */
> >  static int __init integrity_lsm_init(void)
> >  {
> > +	const struct lsm_id *lsmid;
> > +
> >  	iint_cache =
> >  	    kmem_cache_create("iint_cache", sizeof(struct integrity_iint_cache),
> >  			      0, SLAB_PANIC, iint_init_once);
> > +	/*
> > +	 * Obtain either the IMA or EVM LSM ID to register integrity-specific
> > +	 * hooks under that LSM, since there is no LSM ID assigned to the
> > +	 * 'integrity' LSM.
> > +	 */
> > +	lsmid = ima_get_lsm_id();
> > +	if (!lsmid)
> > +		lsmid = evm_get_lsm_id();
> > +	/* No point in continuing, since both IMA and EVM are disabled. */
> > +	if (!lsmid)
> > +		return 0;
> > +
> > +	security_add_hooks(integrity_hooks, ARRAY_SIZE(integrity_hooks), lsmid);
> 
> Ooof.  I understand, or at least I think I understand, why the above
> hack is needed, but I really don't like the idea of @integrity_hooks
> jumping between IMA and EVM depending on how the kernel is configured.
> 
> Just to make sure I'm understanding things correctly, the "integrity"
> LSM exists to ensure the proper hook ordering between IMA/EVM, shared
> metadata management for IMA/EVM, and a little bit of a hack to solve
> some kernel module loading issues with signatures.  Is that correct?
> 
> I see that patch 23/23 makes some nice improvements to the metadata
> management, moving them into LSM security blobs, but it appears that
> they are still shared, and thus the requirement is still there for
> an "integrity" LSM to manage the shared blobs.

Yes, all is correct.

> I'd like to hear everyone's honest opinion on this next question: do
> we have any hope of separating IMA and EVM so they are independent
> (ignore the ordering issues for a moment), or are we always going to
> need to have the "integrity" LSM to manage shared resources, hooks,
> etc.?

I think it should not be technically difficult to do it. But, it would
be very important to understand all the implications of doing those
changes.

Sorry, for now I don't see an immediate need to do that, other than
solving this LSM naming issue. I tried to find the best solution I
could.

Thanks

Roberto

> >  	init_ima_lsm();
> >  	init_evm_lsm();
> >  	return 0;
> 
> --
> paul-moore.com
Paul Moore Nov. 17, 2023, 9:22 p.m. UTC | #4 
On Thu, Nov 16, 2023 at 5:08 AM Roberto Sassu
<roberto.sassu@huaweicloud.com> wrote:
> On Wed, 2023-11-15 at 23:33 -0500, Paul Moore wrote:
> > On Nov  7, 2023 Roberto Sassu <roberto.sassu@huaweicloud.com> wrote:

...

> > > +/*
> > > + * Perform the initialization of the 'integrity', 'ima' and 'evm' LSMs to
> > > + * ensure that the management of integrity metadata is working at the time
> > > + * IMA and EVM hooks are registered to the LSM infrastructure, and to keep
> > > + * the original ordering of IMA and EVM functions as when they were hardcoded.
> > > + */
> > >  static int __init integrity_lsm_init(void)
> > >  {
> > > +   const struct lsm_id *lsmid;
> > > +
> > >     iint_cache =
> > >         kmem_cache_create("iint_cache", sizeof(struct integrity_iint_cache),
> > >                           0, SLAB_PANIC, iint_init_once);
> > > +   /*
> > > +    * Obtain either the IMA or EVM LSM ID to register integrity-specific
> > > +    * hooks under that LSM, since there is no LSM ID assigned to the
> > > +    * 'integrity' LSM.
> > > +    */
> > > +   lsmid = ima_get_lsm_id();
> > > +   if (!lsmid)
> > > +           lsmid = evm_get_lsm_id();
> > > +   /* No point in continuing, since both IMA and EVM are disabled. */
> > > +   if (!lsmid)
> > > +           return 0;
> > > +
> > > +   security_add_hooks(integrity_hooks, ARRAY_SIZE(integrity_hooks), lsmid);
> >
> > Ooof.  I understand, or at least I think I understand, why the above
> > hack is needed, but I really don't like the idea of @integrity_hooks
> > jumping between IMA and EVM depending on how the kernel is configured.
> >
> > Just to make sure I'm understanding things correctly, the "integrity"
> > LSM exists to ensure the proper hook ordering between IMA/EVM, shared
> > metadata management for IMA/EVM, and a little bit of a hack to solve
> > some kernel module loading issues with signatures.  Is that correct?
> >
> > I see that patch 23/23 makes some nice improvements to the metadata
> > management, moving them into LSM security blobs, but it appears that
> > they are still shared, and thus the requirement is still there for
> > an "integrity" LSM to manage the shared blobs.
>
> Yes, all is correct.

Thanks for the clarification, more on this below.

> > I'd like to hear everyone's honest opinion on this next question: do
> > we have any hope of separating IMA and EVM so they are independent
> > (ignore the ordering issues for a moment), or are we always going to
> > need to have the "integrity" LSM to manage shared resources, hooks,
> > etc.?
>
> I think it should not be technically difficult to do it. But, it would
> be very important to understand all the implications of doing those
> changes.
>
> Sorry, for now I don't see an immediate need to do that, other than
> solving this LSM naming issue. I tried to find the best solution I
> could.

I first want to say that I think you've done a great job thus far, and
I'm very grateful for the work you've done.  We can always use more
help in the kernel security space and I'm very happy to see your
contributions - thank you :)

I'm concerned about the integrity LSM because it isn't really a LSM,
it is simply an implementation artifact from a time when only one LSM
was enabled.  Now that we have basic support for stacking LSMs, as we
promote integrity/IMA/EVM I think this is the perfect time to move
away from the "integrity" portion and integrate the necessary
functionality into the IMA and EVM LSMs.  This is even more important
now that we are looking at making the LSMs more visible to userspace
via syscalls; how would you explain to a developer or user the need
for an "integrity" LSM along with the IMA and EVM LSMs?

Let's look at the three things the the integrity code provides in this patchset:

* IMA/EVM hook ordering

For better or worse, we have requirements on LSM ordering today that
are enforced only by convention, the BPF LSM being the perfect
example.  As long as we document this in Kconfig I think we are okay.

* Shared metadata

Looking at the integrity_iint_cache struct at the end of your patchset
I see the following:

  struct integrity_iint_cache {
    struct mutex mutex;
    struct inode *inode;
    u64 version;
    unsigned long flags;
    unsigned long measured_pcrs;
    unsigned long atomic_flags;
    enum integrity_status ima_file_status:4;
    enum integrity_status ima_mmap_status:4;
    enum integrity_status ima_bprm_status:4;
    enum integrity_status ima_read_status:4;
    enum integrity_status ima_creds_status:4;
    enum integrity_status evm_status:4;
    struct ima_digest_data *ima_hash;
  };

Now that we are stashing the metadata in the inode, we should be able
to remove the @inode field back pointer.  It seems like we could
duplicate @mutex and @version without problem.

I only see the @measured_pcrs, @atomic_flags used in the IMA code.

I only see the @ima_XXX_status fields using in the IMA code, and the
@evm_status used in the EVM code.

I only see the @ima_hash field used by the IMA code.

I do see both IMA and EVM using the @flags field, but only one case
(IMA_NEW_FILE) where one LSM (EVM) looks for another flags (IMA).  I'm
not sure how difficult that would be to untangle, but I imagine we
could do something here; if we had to, we could make EVM be dependent
on IMA in Kconfig and add a function call to check on the inode
status.  Although I hope we could find a better solution.

* Kernel module loading hook (integrity_kernel_module_request(...))

My guess is that this is really an IMA hook, but I can't say for
certain.  If it is needed for EVM we could always duplicate it across
the IMA and EVM LSMs, it is trivially small and one extra strcmp() at
kernel module load time doesn't seem awful to me.
Roberto Sassu Nov. 20, 2023, 1:23 p.m. UTC | #5 
On Fri, 2023-11-17 at 16:22 -0500, Paul Moore wrote:
> On Thu, Nov 16, 2023 at 5:08 AM Roberto Sassu
> <roberto.sassu@huaweicloud.com> wrote:
> > On Wed, 2023-11-15 at 23:33 -0500, Paul Moore wrote:
> > > On Nov  7, 2023 Roberto Sassu <roberto.sassu@huaweicloud.com> wrote:
> 
> ...
> 
> > > > +/*
> > > > + * Perform the initialization of the 'integrity', 'ima' and 'evm' LSMs to
> > > > + * ensure that the management of integrity metadata is working at the time
> > > > + * IMA and EVM hooks are registered to the LSM infrastructure, and to keep
> > > > + * the original ordering of IMA and EVM functions as when they were hardcoded.
> > > > + */
> > > >  static int __init integrity_lsm_init(void)
> > > >  {
> > > > +   const struct lsm_id *lsmid;
> > > > +
> > > >     iint_cache =
> > > >         kmem_cache_create("iint_cache", sizeof(struct integrity_iint_cache),
> > > >                           0, SLAB_PANIC, iint_init_once);
> > > > +   /*
> > > > +    * Obtain either the IMA or EVM LSM ID to register integrity-specific
> > > > +    * hooks under that LSM, since there is no LSM ID assigned to the
> > > > +    * 'integrity' LSM.
> > > > +    */
> > > > +   lsmid = ima_get_lsm_id();
> > > > +   if (!lsmid)
> > > > +           lsmid = evm_get_lsm_id();
> > > > +   /* No point in continuing, since both IMA and EVM are disabled. */
> > > > +   if (!lsmid)
> > > > +           return 0;
> > > > +
> > > > +   security_add_hooks(integrity_hooks, ARRAY_SIZE(integrity_hooks), lsmid);
> > > 
> > > Ooof.  I understand, or at least I think I understand, why the above
> > > hack is needed, but I really don't like the idea of @integrity_hooks
> > > jumping between IMA and EVM depending on how the kernel is configured.
> > > 
> > > Just to make sure I'm understanding things correctly, the "integrity"
> > > LSM exists to ensure the proper hook ordering between IMA/EVM, shared
> > > metadata management for IMA/EVM, and a little bit of a hack to solve
> > > some kernel module loading issues with signatures.  Is that correct?
> > > 
> > > I see that patch 23/23 makes some nice improvements to the metadata
> > > management, moving them into LSM security blobs, but it appears that
> > > they are still shared, and thus the requirement is still there for
> > > an "integrity" LSM to manage the shared blobs.
> > 
> > Yes, all is correct.
> 
> Thanks for the clarification, more on this below.
> 
> > > I'd like to hear everyone's honest opinion on this next question: do
> > > we have any hope of separating IMA and EVM so they are independent
> > > (ignore the ordering issues for a moment), or are we always going to
> > > need to have the "integrity" LSM to manage shared resources, hooks,
> > > etc.?
> > 
> > I think it should not be technically difficult to do it. But, it would
> > be very important to understand all the implications of doing those
> > changes.
> > 
> > Sorry, for now I don't see an immediate need to do that, other than
> > solving this LSM naming issue. I tried to find the best solution I
> > could.
> 
> I first want to say that I think you've done a great job thus far, and
> I'm very grateful for the work you've done.  We can always use more
> help in the kernel security space and I'm very happy to see your
> contributions - thank you :)

Thank you!

> I'm concerned about the integrity LSM because it isn't really a LSM,
> it is simply an implementation artifact from a time when only one LSM
> was enabled.  Now that we have basic support for stacking LSMs, as we
> promote integrity/IMA/EVM I think this is the perfect time to move
> away from the "integrity" portion and integrate the necessary
> functionality into the IMA and EVM LSMs.  This is even more important
> now that we are looking at making the LSMs more visible to userspace
> via syscalls; how would you explain to a developer or user the need
> for an "integrity" LSM along with the IMA and EVM LSMs?
> 
> Let's look at the three things the the integrity code provides in this patchset:
> 
> * IMA/EVM hook ordering
> 
> For better or worse, we have requirements on LSM ordering today that
> are enforced only by convention, the BPF LSM being the perfect
> example.  As long as we document this in Kconfig I think we are okay.
> 
> * Shared metadata
> 
> Looking at the integrity_iint_cache struct at the end of your patchset
> I see the following:
> 
>   struct integrity_iint_cache {
>     struct mutex mutex;
>     struct inode *inode;
>     u64 version;
>     unsigned long flags;
>     unsigned long measured_pcrs;
>     unsigned long atomic_flags;
>     enum integrity_status ima_file_status:4;
>     enum integrity_status ima_mmap_status:4;
>     enum integrity_status ima_bprm_status:4;
>     enum integrity_status ima_read_status:4;
>     enum integrity_status ima_creds_status:4;
>     enum integrity_status evm_status:4;
>     struct ima_digest_data *ima_hash;
>   };
> 
> Now that we are stashing the metadata in the inode, we should be able
> to remove the @inode field back pointer.  It seems like we could
> duplicate @mutex and @version without problem.
> 
> I only see the @measured_pcrs, @atomic_flags used in the IMA code.
> 
> I only see the @ima_XXX_status fields using in the IMA code, and the
> @evm_status used in the EVM code.
> 
> I only see the @ima_hash field used by the IMA code.
> 
> I do see both IMA and EVM using the @flags field, but only one case
> (IMA_NEW_FILE) where one LSM (EVM) looks for another flags (IMA).  I'm
> not sure how difficult that would be to untangle, but I imagine we
> could do something here; if we had to, we could make EVM be dependent
> on IMA in Kconfig and add a function call to check on the inode
> status.  Although I hope we could find a better solution.
> 
> * Kernel module loading hook (integrity_kernel_module_request(...))
> 
> My guess is that this is really an IMA hook, but I can't say for
> certain.  If it is needed for EVM we could always duplicate it across
> the IMA and EVM LSMs, it is trivially small and one extra strcmp() at
> kernel module load time doesn't seem awful to me.

Ok... so, for now I'm trying to separate them just to see if it is
possible. Will send just the integrity-related patches shortly.

Thanks

Roberto
diff mbox series

Patch

diff --git a/include/linux/integrity.h b/include/linux/integrity.h
index 2ea0f2f65ab6..afaae7ad26f4 100644
--- a/include/linux/integrity.h
+++ b/include/linux/integrity.h
@@ -21,38 +21,12 @@  enum integrity_status {
 
 /* List of EVM protected security xattrs */
 #ifdef CONFIG_INTEGRITY
-extern struct integrity_iint_cache *integrity_inode_get(struct inode *inode);
-extern void integrity_inode_free(struct inode *inode);
 extern void __init integrity_load_keys(void);
 
 #else
-static inline struct integrity_iint_cache *
-				integrity_inode_get(struct inode *inode)
-{
-	return NULL;
-}
-
-static inline void integrity_inode_free(struct inode *inode)
-{
-	return;
-}
-
 static inline void integrity_load_keys(void)
 {
 }
 #endif /* CONFIG_INTEGRITY */
 
-#ifdef CONFIG_INTEGRITY_ASYMMETRIC_KEYS
-
-extern int integrity_kernel_module_request(char *kmod_name);
-
-#else
-
-static inline int integrity_kernel_module_request(char *kmod_name)
-{
-	return 0;
-}
-
-#endif /* CONFIG_INTEGRITY_ASYMMETRIC_KEYS */
-
 #endif /* _LINUX_INTEGRITY_H */
diff --git a/security/integrity/iint.c b/security/integrity/iint.c
index 0b0ac71142e8..882fde2a2607 100644
--- a/security/integrity/iint.c
+++ b/security/integrity/iint.c
@@ -171,7 +171,7 @@  struct integrity_iint_cache *integrity_inode_get(struct inode *inode)
  *
  * Free the integrity information(iint) associated with an inode.
  */
-void integrity_inode_free(struct inode *inode)
+static void integrity_inode_free(struct inode *inode)
 {
 	struct integrity_iint_cache *iint;
 
@@ -193,11 +193,39 @@  static void iint_init_once(void *foo)
 	memset(iint, 0, sizeof(*iint));
 }
 
+static struct security_hook_list integrity_hooks[] __ro_after_init = {
+	LSM_HOOK_INIT(inode_free_security, integrity_inode_free),
+#ifdef CONFIG_INTEGRITY_ASYMMETRIC_KEYS
+	LSM_HOOK_INIT(kernel_module_request, integrity_kernel_module_request),
+#endif
+};
+
+/*
+ * Perform the initialization of the 'integrity', 'ima' and 'evm' LSMs to
+ * ensure that the management of integrity metadata is working at the time
+ * IMA and EVM hooks are registered to the LSM infrastructure, and to keep
+ * the original ordering of IMA and EVM functions as when they were hardcoded.
+ */
 static int __init integrity_lsm_init(void)
 {
+	const struct lsm_id *lsmid;
+
 	iint_cache =
 	    kmem_cache_create("iint_cache", sizeof(struct integrity_iint_cache),
 			      0, SLAB_PANIC, iint_init_once);
+	/*
+	 * Obtain either the IMA or EVM LSM ID to register integrity-specific
+	 * hooks under that LSM, since there is no LSM ID assigned to the
+	 * 'integrity' LSM.
+	 */
+	lsmid = ima_get_lsm_id();
+	if (!lsmid)
+		lsmid = evm_get_lsm_id();
+	/* No point in continuing, since both IMA and EVM are disabled. */
+	if (!lsmid)
+		return 0;
+
+	security_add_hooks(integrity_hooks, ARRAY_SIZE(integrity_hooks), lsmid);
 	init_ima_lsm();
 	init_evm_lsm();
 	return 0;
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 7534ec06324e..e4df82d6f6e7 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -180,6 +180,7 @@  struct integrity_iint_cache {
  * integrity data associated with an inode.
  */
 struct integrity_iint_cache *integrity_iint_find(struct inode *inode);
+struct integrity_iint_cache *integrity_inode_get(struct inode *inode);
 
 int integrity_kernel_read(struct file *file, loff_t offset,
 			  void *addr, unsigned long count);
@@ -266,12 +267,18 @@  static inline int __init integrity_load_cert(const unsigned int id,
 #ifdef CONFIG_INTEGRITY_ASYMMETRIC_KEYS
 int asymmetric_verify(struct key *keyring, const char *sig,
 		      int siglen, const char *data, int datalen);
+int integrity_kernel_module_request(char *kmod_name);
 #else
 static inline int asymmetric_verify(struct key *keyring, const char *sig,
 				    int siglen, const char *data, int datalen)
 {
 	return -EOPNOTSUPP;
 }
+
+static inline int integrity_kernel_module_request(char *kmod_name)
+{
+	return 0;
+}
 #endif
 
 #ifdef CONFIG_IMA_APPRAISE_MODSIG
diff --git a/security/security.c b/security/security.c
index 9703549b6ddc..0d9eaa4cd260 100644
--- a/security/security.c
+++ b/security/security.c
@@ -19,7 +19,6 @@ 
 #include <linux/kernel.h>
 #include <linux/kernel_read_file.h>
 #include <linux/lsm_hooks.h>
-#include <linux/integrity.h>
 #include <linux/fsnotify.h>
 #include <linux/mman.h>
 #include <linux/mount.h>
@@ -1597,7 +1596,6 @@  static void inode_free_by_rcu(struct rcu_head *head)
  */
 void security_inode_free(struct inode *inode)
 {
-	integrity_inode_free(inode);
 	call_void_hook(inode_free_security, inode);
 	/*
 	 * The inode may still be referenced in a path walk and
@@ -3182,12 +3180,7 @@  int security_kernel_create_files_as(struct cred *new, struct inode *inode)
  */
 int security_kernel_module_request(char *kmod_name)
 {
-	int ret;
-
-	ret = call_int_hook(kernel_module_request, 0, kmod_name);
-	if (ret)
-		return ret;
-	return integrity_kernel_module_request(kmod_name);
+	return call_int_hook(kernel_module_request, 0, kmod_name);
 }
 
 /**