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

[Paul Moore <paul@paul-moore.com>]

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