Re: [RFC][PATCH 8/8] ima: Detect if digest cache changed since last measurement/appraisal
On Mon, 2024-03-11 at 10:11 +0100, Roberto Sassu wrote: > > > > @@ -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. Agreed. > > 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. Yes, something similar to the "ima_lsm_policy_notifier". Mimi
Re: [RFC][PATCH 8/8] ima: Detect if digest cache changed since last measurement/appraisal
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__APPRAISE, IMA__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)
>
Re: [RFC][PATCH 8/8] ima: Detect if digest cache changed since last measurement/appraisal
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__APPRAISE, IMA__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;
> + }
>
> 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.
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?
thanks,
Mimi
> }
>
> if (action & IMA_MEASURE)
