Received-SPF: pass (google.com: domain of linux-kernel+bounces-97272-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <acfb4159b16b84d8fa1517d6870edaaaadf901c8.camel@linux.ibm.com>
Subject: Re: [RFC][PATCH 6/8] ima: Use digest cache for measurement
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 11:08:06 -0500
In-Reply-To: <20240214143525.2205481-7-roberto.sassu@huaweicloud.com>
References: <20240214143525.2205481-1-roberto.sassu@huaweicloud.com>
	 <20240214143525.2205481-7-roberto.sassu@huaweicloud.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit

Hi Roberto,

> diff --git a/security/integrity/ima/ima_main.c
> b/security/integrity/ima/ima_main.c
> index 3fc48214850a..48a09747ae7a 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -222,7 +222,9 @@ static int process_measurement(struct file *file, const
> struct cred *cred,
>  	bool violation_check;
>  	enum hash_algo hash_algo;
>  	unsigned int allowed_algos = 0;
> -	u64 verif_mask = 0;
> +	u64 verif_mask = 0, *verif_mask_ptr, policy_mask = 0, allow_mask = 0;
> +	struct digest_cache *digest_cache = NULL, *found_cache;
> +	digest_cache_found_t found;
>  
>  	if (!ima_policy_flag || !S_ISREG(inode->i_mode))
>  		return 0;
> @@ -233,7 +235,7 @@ static int process_measurement(struct file *file, const
> struct cred *cred,
>  	 */
>  	action = ima_get_action(file_mnt_idmap(file), inode, cred, secid,
>  				mask, func, &pcr, &template_desc, NULL,
> -				&allowed_algos, NULL);
> +				&allowed_algos, &policy_mask);
>  	violation_check = ((func == FILE_CHECK || func == MMAP_CHECK ||
>  			    func == MMAP_CHECK_REQPROT) &&
>  			   (ima_policy_flag & IMA_MEASURE));
> @@ -364,10 +366,34 @@ static int process_measurement(struct file *file, const
> struct cred *cred,
>  	if (!pathbuf)	/* ima_rdwr_violation possibly pre-fetched */
>  		pathname = ima_d_path(&file->f_path, &pathbuf, filename);
>  
> +	/*
> +	 * For now we don't support nested verification with digest caches.

I haven't reviewed the digest_cache LSM patch set yet.  What does 'nested' mean
in this context?  Why mention it here?

> +	 * 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));

So whether or not a DIGEST_LIST_CHECK policy rule even exists,
digest_cache_get() will be called.  Similarly, even if a digest_cache list
hasn't been measured or appraised, digest_cache_get() will be called.

Basically every file in policy will check the digest_cache.

> +
> +	if (digest_cache) {
> +		found = digest_cache_lookup(file_dentry(file), digest_cache,
> +					    iint->ima_hash->digest,
> +					    iint->ima_hash->algo);
> +		/* AND what is allowed by the policy, and what IMA verified. */
> +		if (found) {
> +			found_cache = digest_cache_from_found_t(found);
> +			verif_mask_ptr = digest_cache_verif_get(found_cache,
> +								"ima");

Instead of using "verif_{set,get}' consider using '{set,get}_usage', where usage
here means measure or appraise.

> +			if (verif_mask_ptr)
> +				allow_mask = policy_mask & *verif_mask_ptr;
> +		}
> +
> +		digest_cache_put(digest_cache);
> +	}
> +

I'm wondering if it makes sense to create IMA wrappers for each of the
digest_cache functions - checking the digest_cache for the hash, setting the
digest_cache permitted usage, etc - and put all of them in a separate
ima_digest_cache.c file.  The file would only be included in the Makefile if
digest_cache is configured.

In this file you could define a static local global variable to detect whether
the digest_cache is ready to be used.  Only after successfully measuring and
appraising a digest_cache list, based on policy, set the variable.

>  	if (action & IMA_MEASURE)
>  		ima_store_measurement(iint, file, pathname,
>  				      xattr_value, xattr_len, modsig, pcr,
> -				      template_desc);
> +				      template_desc, allow_mask);

'allowed_usage'?

>  	if (rc == 0 && (action & IMA_APPRAISE_SUBMASK)) {
>  		rc = ima_check_blacklist(iint, modsig, pcr);
>  		if (rc != -EPERM) {

thanks,

Mimi