Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH 7/8] ima: use ima_hash_algo for collision detection in
 the measurement list
From:   Mimi Zohar <zohar@linux.ibm.com>
To:     Roberto Sassu <roberto.sassu@huawei.com>,
        jarkko.sakkinen@linux.intel.com,
        james.bottomley@hansenpartnership.com,
        linux-integrity@vger.kernel.org
Cc:     linux-security-module@vger.kernel.org,
        linux-kernel@vger.kernel.org, silviu.vlasceanu@huawei.com
Date:   Thu, 30 Jan 2020 17:26:09 -0500
In-Reply-To: <20200127170443.21538-8-roberto.sassu@huawei.com>
References: <20200127170443.21538-1-roberto.sassu@huawei.com>
         <20200127170443.21538-8-roberto.sassu@huawei.com>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 8bit
Message-Id: <1580423169.6104.18.camel@linux.ibm.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, 2020-01-27 at 18:04 +0100, Roberto Sassu wrote:
> Before calculating a digest for each PCR bank, collisions were detected
> with a SHA1 digest. This patch includes ima_hash_algo among the algorithms
> used to calculate the template digest and checks collisions on that digest.
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>

Definitely needed to protect against a sha1 collision attack.

<snip>

>  
> diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
> index ebaf0056735c..a9bb45de6db9 100644
> --- a/security/integrity/ima/ima_api.c
> +++ b/security/integrity/ima/ima_api.c
> @@ -51,7 +51,7 @@ int ima_alloc_init_template(struct ima_event_data *event_data,
>  	if (!*entry)
>  		return -ENOMEM;
>  
> -	(*entry)->digests = kcalloc(ima_tpm_chip->nr_allocated_banks + 1,
> +	(*entry)->digests = kcalloc(ima_tpm_chip->nr_allocated_banks + 2,
>  				    sizeof(*(*entry)->digests), GFP_NOFS);
>  	if (!(*entry)->digests) {
>  		result = -ENOMEM;

I would prefer not having to allocate and use "nr_allocated_banks + 1"
everywhere, but I understand the need for it.  I'm not sure this patch
warrants allocating +2.  Perhaps, if a TPM bank doesn't exist for the
IMA default hash algorithm, use a different algorithm or, worst case,
continue using the ima_sha1_idx.

Mimi