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

[Roberto Sassu]

On Wed, 2023-11-15 at 23:33 -0500, Paul Moore wrote:
> On Nov  7, 2023 Roberto Sassu <roberto.sassu@xxxxxxxxxxxxxxx> 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@xxxxxxxxxx>
> > Reviewed-by: Casey Schaufler <casey@xxxxxxxxxxxxxxxx>
> > Reviewed-by: Mimi Zohar <zohar@xxxxxxxxxxxxx>
> > ---
> >  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