Received-SPF: pass (google.com: domain of linux-kernel+bounces-97401-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <ddb1c28356fb8a4dcca9bff6dc206802d7981bb8.camel@linux.ibm.com>
Subject: Re: [RFC][PATCH 8/8] ima: Detect if digest cache changed since last
 measurement/appraisal
From: Mimi Zohar <zohar@linux.ibm.com>
To: Roberto Sassu <roberto.sassu@huaweicloud.com>, corbet@lwn.net,
        dmitry.kasatkin@gmail.com, eric.snowberg@oracle.com,
        paul@paul-moore.com, jmorris@namei.org, serge@hallyn.com
Cc: linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org,
        linux-integrity@vger.kernel.org, linux-security-module@vger.kernel.org,
        wufan@linux.microsoft.com, pbrobinson@gmail.com, zbyszek@in.waw.pl,
        hch@lst.de, mjg59@srcf.ucam.org, pmatilai@redhat.com, jannh@google.com,
        dhowells@redhat.com, jikos@kernel.org, mkoutny@suse.com,
        ppavlu@suse.com, petr.vorel@gmail.com, petrtesarik@huaweicloud.com,
        mzerqung@0pointer.de, kgold@linux.ibm.com,
        Roberto Sassu <roberto.sassu@huawei.com>
Date: Fri, 08 Mar 2024 12:35:05 -0500
In-Reply-To: <20240214143525.2205481-9-roberto.sassu@huaweicloud.com>
References: <20240214143525.2205481-1-roberto.sassu@huaweicloud.com>
	 <20240214143525.2205481-9-roberto.sassu@huaweicloud.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit

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;

> +	}
>  
>  	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)