Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <b0ba8db6-cf85-0ee3-af19-c8b1251d5b63@linux.ibm.com>
Date:   Mon, 14 Feb 2022 15:13:32 -0500
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
Subject: Re: [PATCH v5 2/8] ima: define ima_max_digest_data struct without a
 flexible array variable
Content-Language: en-US
To:     Mimi Zohar <zohar@linux.ibm.com>, linux-integrity@vger.kernel.org
Cc:     Eric Biggers <ebiggers@kernel.org>, linux-fscrypt@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20220211214310.119257-1-zohar@linux.ibm.com>
 <20220211214310.119257-3-zohar@linux.ibm.com>
From:   Stefan Berger <stefanb@linux.ibm.com>
In-Reply-To: <20220211214310.119257-3-zohar@linux.ibm.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 2/11/22 16:43, Mimi Zohar wrote:
> To support larger hash digests in the 'iint' cache, instead of defining
> the 'digest' field as the maximum digest size, the 'digest' field was
> defined as a flexible array variable.  The "ima_digest_data" struct was
> wrapped inside a local structure with the maximum digest size.  But
> before adding the record to the iint cache, memory for the exact digest
> size was dynamically allocated.
>
> The original reason for defining the 'digest' field as a flexible array
> variable is still valid for the 'iint' cache use case.  Instead of
> wrapping the 'ima_digest_data' struct in a local structure define
> 'ima_max_digest_data' struct.
>
> Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> ---
>   security/integrity/ima/ima_api.c          | 10 ++++------
>   security/integrity/ima/ima_init.c         |  5 +----
>   security/integrity/ima/ima_main.c         |  5 +----
>   security/integrity/ima/ima_template_lib.c |  5 +----
>   security/integrity/integrity.h            | 10 ++++++++++
>   5 files changed, 17 insertions(+), 18 deletions(-)
>
> diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
> index 5b220a2fe573..c6805af46211 100644
> --- a/security/integrity/ima/ima_api.c
> +++ b/security/integrity/ima/ima_api.c
> @@ -217,14 +217,11 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
>   	const char *audit_cause = "failed";
>   	struct inode *inode = file_inode(file);
>   	const char *filename = file->f_path.dentry->d_name.name;
> +	struct ima_max_digest_data hash;
>   	int result = 0;
>   	int length;
>   	void *tmpbuf;
>   	u64 i_version;
> -	struct {
> -		struct ima_digest_data hdr;
> -		char digest[IMA_MAX_DIGEST_SIZE];
> -	} hash;
>   
>   	/*
>   	 * Always collect the modsig, because IMA might have already collected
> @@ -239,8 +236,9 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
>   
>   	/*
>   	 * Detecting file change is based on i_version. On filesystems
> -	 * which do not support i_version, support is limited to an initial
> -	 * measurement/appraisal/audit.
> +	 * which do not support i_version, support was originally limited
> +	 * to an initial measurement/appraisal/audit, but was modified to
> +	 * assume the file changed.
>   	 */
>   	i_version = inode_query_iversion(inode);
>   	hash.hdr.algo = algo;
> diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c
> index b26fa67476b4..63979aefc95f 100644
> --- a/security/integrity/ima/ima_init.c
> +++ b/security/integrity/ima/ima_init.c
> @@ -47,12 +47,9 @@ static int __init ima_add_boot_aggregate(void)
>   	struct integrity_iint_cache tmp_iint, *iint = &tmp_iint;
>   	struct ima_event_data event_data = { .iint = iint,
>   					     .filename = boot_aggregate_name };
> +	struct ima_max_digest_data hash;
>   	int result = -ENOMEM;
>   	int violation = 0;
> -	struct {
> -		struct ima_digest_data hdr;
> -		char digest[TPM_MAX_DIGEST_SIZE];
> -	} hash;
>   
>   	memset(iint, 0, sizeof(*iint));
>   	memset(&hash, 0, sizeof(hash));
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 7c80dfe2c7a5..c6412dec3810 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -874,10 +874,7 @@ int process_buffer_measurement(struct user_namespace *mnt_userns,
>   					    .buf = buf,
>   					    .buf_len = size};
>   	struct ima_template_desc *template;
> -	struct {
> -		struct ima_digest_data hdr;
> -		char digest[IMA_MAX_DIGEST_SIZE];
> -	} hash = {};
> +	struct ima_max_digest_data hash;


It looks like the initialization wasn't necessary.

Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>


>   	char digest_hash[IMA_MAX_DIGEST_SIZE];
>   	int digest_hash_len = hash_digest_size[ima_hash_algo];
>   	int violation = 0;
> diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c
> index 5a5d462ab36d..7155d17a3b75 100644
> --- a/security/integrity/ima/ima_template_lib.c
> +++ b/security/integrity/ima/ima_template_lib.c
> @@ -307,10 +307,7 @@ static int ima_eventdigest_init_common(const u8 *digest, u32 digestsize,
>   int ima_eventdigest_init(struct ima_event_data *event_data,
>   			 struct ima_field_data *field_data)
>   {
> -	struct {
> -		struct ima_digest_data hdr;
> -		char digest[IMA_MAX_DIGEST_SIZE];
> -	} hash;
> +	struct ima_max_digest_data hash;
>   	u8 *cur_digest = NULL;
>   	u32 cur_digestsize = 0;
>   	struct inode *inode;
> diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
> index d045dccd415a..daf49894fd7d 100644
> --- a/security/integrity/integrity.h
> +++ b/security/integrity/integrity.h
> @@ -15,6 +15,7 @@
>   #include <linux/types.h>
>   #include <linux/integrity.h>
>   #include <crypto/sha1.h>
> +#include <crypto/hash.h>
>   #include <linux/key.h>
>   #include <linux/audit.h>
>   
> @@ -110,6 +111,15 @@ struct ima_digest_data {
>   	u8 digest[];
>   } __packed;
>   
> +/*
> + * Instead of wrapping the ima_digest_data struct inside a local structure
> + * with the maximum hash size, define ima_max_digest_data struct.
> + */
> +struct ima_max_digest_data {
> +	struct ima_digest_data hdr;
> +	u8 digest[HASH_MAX_DIGESTSIZE];
> +} __packed;
> +
>   /*
>    * signature format v2 - for using with asymmetric keys
>    */