Re: [RFC][PATCH 8/8] ima: Detect if digest cache changed since last measurement/appraisal

[Roberto Sassu <roberto.sassu@huaweicloud.com>]

On Fri, 2024-03-08 at 12:35 -0500, Mimi Zohar wrote:
> Hi Roberto,
> 
> > b/security/integrity/ima/ima_main.c
> > index a66522a22cbc..e1b2f5737753 100644
> > --- a/security/integrity/ima/ima_main.c
> > +++ b/security/integrity/ima/ima_main.c
> > @@ -301,6 +301,15 @@ static int process_measurement(struct file *file, const
> > struct cred *cred,
> >  		}
> >  	}
> >  
> > +	/* Check if digest cache changed since last measurement/appraisal. */
> > +	if (iint->digest_cache &&
> > +	    digest_cache_changed(inode, iint->digest_cache)) {
> > +		iint->flags &= ~IMA_DONE_MASK;
> > +		iint->measured_pcrs = 0;
> > +		digest_cache_put(iint->digest_cache);
> > +		iint->digest_cache = NULL;
> > +	}
> > +
> >  	/* Determine if already appraised/measured based on bitmask
> >  	 * (IMA_MEASURE, IMA_MEASURED, IMA_XXXX_APPRAISE, IMA_XXXX_APPRAISED,
> >  	 *  IMA_AUDIT, IMA_AUDITED)
> > @@ -371,8 +380,15 @@ static int process_measurement(struct file *file, const
> > struct cred *cred,
> >  	 * Since we allow IMA policy rules without func=, we have to enforce
> >  	 * this restriction here.
> >  	 */
> > -	if (rc == 0 && policy_mask && func != DIGEST_LIST_CHECK)
> > -		digest_cache = digest_cache_get(file_dentry(file));
> > +	if (rc == 0 && policy_mask && func != DIGEST_LIST_CHECK) {
> > +		if (!iint->digest_cache) {
> > +			/* Released by ima_iint_free(). */
> > +			digest_cache = digest_cache_get(file_dentry(file));
> > +			iint->digest_cache = digest_cache;
> > +		} else {
> > +			digest_cache = iint->digest_cache;
> > +		}
> 
> Simple cleanup:
> 		if (!iint->digest_cache)
> 			iint->digest_cache =digest_cache_get(file_dentry(file));
> 
> 		digest_cache = iint->digest_cache;

Thanks.

> > +	}
> >  
> >  	if (digest_cache) {
> >  		found = digest_cache_lookup(file_dentry(file), digest_cache,
> > @@ -386,8 +402,6 @@ static int process_measurement(struct file *file, const
> > struct cred *cred,
> >  			if (verif_mask_ptr)
> >  				allow_mask = policy_mask & *verif_mask_ptr;
> >  		}
> > -
> > -		digest_cache_put(digest_cache);
> 
> Keeping a reference to the digest_cache list for each file in the iint cache
> until the file is re-accessed, might take a while to free.

Yes, that is the drawback...

> I'm wondering if it necessary to keep a reference to the digest_cache.  Or is it
> possible to just compare the existing iint->digest_cache pointer with the
> current digest_cache pointer?

If the pointer value is the same, it does not guarantee that it is the
same digest cache used for the previous verification. It might have
been freed and reallocated.

Maybe, if the digest_cache LSM is able to notify to IMA that the digest
cache changed, so that IMA resets its flags in the integrity metadata,
we would not need to pin it.

Roberto

> thanks,
> 
> Mimi
> 
> >  	}
> >  
> >  	if (action & IMA_MEASURE)
>