diff mbox series

[5/8] ima: allocate and initialize tfm for each PCR bank

Message ID 20200127170443.21538-6-roberto.sassu@huawei.com (mailing list archive)
State New, archived
Headers show
Series ima: support stronger algorithms for attestation | expand

Commit Message

Roberto Sassu Jan. 27, 2020, 5:04 p.m. UTC 
This patch creates a crypto_shash structure for each allocated PCR bank and
for SHA1 if a bank with that algorithm is not currently allocated.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 security/integrity/ima/ima_crypto.c | 137 +++++++++++++++++++++++-----
 1 file changed, 113 insertions(+), 24 deletions(-)

Comments

Mimi Zohar Jan. 31, 2020, 12:18 p.m. UTC | #1 
On Mon, 2020-01-27 at 18:04 +0100, Roberto Sassu wrote:
> This patch creates a crypto_shash structure for each allocated PCR bank and
> for SHA1 if a bank with that algorithm is not currently allocated.
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>

<snip>

> +int __init ima_init_crypto(void)
> +{
> +	enum hash_algo algo;
> +	long rc;
> +	int i;
> +
> +	rc = ima_init_ima_crypto();
> +	if (rc)
> +		return rc;
> +
> +	ima_algo_array = kmalloc_array(ima_tpm_chip->nr_allocated_banks + 1,
> +				       sizeof(*ima_algo_array), GFP_KERNEL);

Perhaps instead of hard coding "nr_allocated_banks + 1", create a
variable for storing the number of "extra" banks.  Walk the banks
setting ima_sha1_idx and, later, in a subsequent patch set
ima_hash_algo_idx.

> +	if (!ima_algo_array) {
> +		rc = -ENOMEM;
> +		goto out;
> +	}
> +
> +	for (i = 0; i < ima_tpm_chip->nr_allocated_banks + 1; i++) {
> +		ima_algo_array[i].tfm = NULL;
> +		ima_algo_array[i].algo = HASH_ALGO__LAST;
> +	}

ima_init_crypto() is executed once on initialization.  I'm not sure if
it makes a difference, but if you're really concerned about an
additional loop, the initialization, here, could be limited to the
"extra" banks.  The other banks could be initialized at the beginning
of the next loop.

thanks,

Mimi

> +	ima_sha1_idx = -1;
> +
> +	for (i = 0; i < ima_tpm_chip->nr_allocated_banks; i++) {
> +		algo = ima_tpm_chip->allocated_banks[i].crypto_id;
> +		ima_algo_array[i].algo = algo;
> +
> +		/* unknown TPM algorithm */
> +		if (algo == HASH_ALGO__LAST)
> +			continue;
> +
> +		if (algo == HASH_ALGO_SHA1)
> +			ima_sha1_idx = i;
> +
> +		if (algo == ima_hash_algo) {
> +			ima_algo_array[i].tfm = ima_shash_tfm;
> +			continue;
> +		}
> +
> +		ima_algo_array[i].tfm = ima_alloc_tfm(algo);
> +		if (IS_ERR(ima_algo_array[i].tfm)) {
> +			if (algo == HASH_ALGO_SHA1) {
> +				rc = PTR_ERR(ima_algo_array[i].tfm);
> +				ima_algo_array[i].tfm = NULL;
> +				goto out_array;
> +			}
> +
> +			ima_algo_array[i].tfm = NULL;
> +		}
> +	}
> +
> +	if (ima_sha1_idx < 0) {
> +		ima_algo_array[i].tfm = ima_alloc_tfm(HASH_ALGO_SHA1);
> +		if (IS_ERR(ima_algo_array[i].tfm)) {
> +			rc = PTR_ERR(ima_algo_array[i].tfm);
> +			goto out_array;
> +		}
> +
> +		ima_sha1_idx = i;
> +		ima_algo_array[i].algo = HASH_ALGO_SHA1;
> +	}
> +
> +	return 0;
> +out_array:
> +	for (i = 0; i < ima_tpm_chip->nr_allocated_banks + 1; i++) {
> +		if (!ima_algo_array[i].tfm ||
> +		    ima_algo_array[i].tfm == ima_shash_tfm)
> +			continue;
> +
> +		crypto_free_shash(ima_algo_array[i].tfm);
> +	}
> +out:
> +	crypto_free_shash(ima_shash_tfm);
> +	return rc;
> +}
Roberto Sassu Jan. 31, 2020, 1:42 p.m. UTC | #2 
> -----Original Message-----
> From: linux-integrity-owner@vger.kernel.org [mailto:linux-integrity-
> owner@vger.kernel.org] On Behalf Of Mimi Zohar
> Sent: Friday, January 31, 2020 1:19 PM
> 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 <Silviu.Vlasceanu@huawei.com>
> Subject: Re: [PATCH 5/8] ima: allocate and initialize tfm for each PCR bank
> 
> On Mon, 2020-01-27 at 18:04 +0100, Roberto Sassu wrote:
> > This patch creates a crypto_shash structure for each allocated PCR bank
> and
> > for SHA1 if a bank with that algorithm is not currently allocated.
> >
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> 
> <snip>
> 
> > +int __init ima_init_crypto(void)
> > +{
> > +	enum hash_algo algo;
> > +	long rc;
> > +	int i;
> > +
> > +	rc = ima_init_ima_crypto();
> > +	if (rc)
> > +		return rc;
> > +
> > +	ima_algo_array = kmalloc_array(ima_tpm_chip-
> >nr_allocated_banks + 1,
> > +				       sizeof(*ima_algo_array), GFP_KERNEL);
> 
> Perhaps instead of hard coding "nr_allocated_banks + 1", create a
> variable for storing the number of "extra" banks.  Walk the banks
> setting ima_sha1_idx and, later, in a subsequent patch set
> ima_hash_algo_idx.

I could store the indexes in an array and add ARRAY_SIZE of that array.

> > +	if (!ima_algo_array) {
> > +		rc = -ENOMEM;
> > +		goto out;
> > +	}
> > +
> > +	for (i = 0; i < ima_tpm_chip->nr_allocated_banks + 1; i++) {
> > +		ima_algo_array[i].tfm = NULL;
> > +		ima_algo_array[i].algo = HASH_ALGO__LAST;
> > +	}
> 
> ima_init_crypto() is executed once on initialization.  I'm not sure if
> it makes a difference, but if you're really concerned about an
> additional loop, the initialization, here, could be limited to the
> "extra" banks.  The other banks could be initialized at the beginning
> of the next loop.

I set algo to HASH_ALGO__LAST to mark the ima_algo_array entries as
uninitialized. ima_alloc_tfm() uses ima_algo_array in the next loop.
Replacing kmalloc_array() with kcalloc() would cause algo to be set to
HASH_ALGO_MD4.

I could check tfm first, which ensures that also algo was initialized.

Roberto

> > +	ima_sha1_idx = -1;
> > +
> > +	for (i = 0; i < ima_tpm_chip->nr_allocated_banks; i++) {
> > +		algo = ima_tpm_chip->allocated_banks[i].crypto_id;
> > +		ima_algo_array[i].algo = algo;
> > +
> > +		/* unknown TPM algorithm */
> > +		if (algo == HASH_ALGO__LAST)
> > +			continue;
> > +
> > +		if (algo == HASH_ALGO_SHA1)
> > +			ima_sha1_idx = i;
> > +
> > +		if (algo == ima_hash_algo) {
> > +			ima_algo_array[i].tfm = ima_shash_tfm;
> > +			continue;
> > +		}
> > +
> > +		ima_algo_array[i].tfm = ima_alloc_tfm(algo);
> > +		if (IS_ERR(ima_algo_array[i].tfm)) {
> > +			if (algo == HASH_ALGO_SHA1) {
> > +				rc = PTR_ERR(ima_algo_array[i].tfm);
> > +				ima_algo_array[i].tfm = NULL;
> > +				goto out_array;
> > +			}
> > +
> > +			ima_algo_array[i].tfm = NULL;
> > +		}
> > +	}
> > +
> > +	if (ima_sha1_idx < 0) {
> > +		ima_algo_array[i].tfm = ima_alloc_tfm(HASH_ALGO_SHA1);
> > +		if (IS_ERR(ima_algo_array[i].tfm)) {
> > +			rc = PTR_ERR(ima_algo_array[i].tfm);
> > +			goto out_array;
> > +		}
> > +
> > +		ima_sha1_idx = i;
> > +		ima_algo_array[i].algo = HASH_ALGO_SHA1;
> > +	}
> > +
> > +	return 0;
> > +out_array:
> > +	for (i = 0; i < ima_tpm_chip->nr_allocated_banks + 1; i++) {
> > +		if (!ima_algo_array[i].tfm ||
> > +		    ima_algo_array[i].tfm == ima_shash_tfm)
> > +			continue;
> > +
> > +		crypto_free_shash(ima_algo_array[i].tfm);
> > +	}
> > +out:
> > +	crypto_free_shash(ima_shash_tfm);
> > +	return rc;
> > +}
Mimi Zohar Jan. 31, 2020, 1:58 p.m. UTC | #3 
On Fri, 2020-01-31 at 13:42 +0000, Roberto Sassu wrote:
> > -----Original Message-----
> > From: linux-integrity-owner@vger.kernel.org [mailto:linux-integrity-
> > owner@vger.kernel.org] On Behalf Of Mimi Zohar
> > Sent: Friday, January 31, 2020 1:19 PM
> > 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 <Silviu.Vlasceanu@huawei.com>
> > Subject: Re: [PATCH 5/8] ima: allocate and initialize tfm for each PCR bank
> > 
> > On Mon, 2020-01-27 at 18:04 +0100, Roberto Sassu wrote:
> > > This patch creates a crypto_shash structure for each allocated PCR bank
> > and
> > > for SHA1 if a bank with that algorithm is not currently allocated.
> > >
> > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > 
> > <snip>
> > 
> > > +int __init ima_init_crypto(void)
> > > +{
> > > +	enum hash_algo algo;
> > > +	long rc;
> > > +	int i;
> > > +
> > > +	rc = ima_init_ima_crypto();
> > > +	if (rc)
> > > +		return rc;
> > > +
> > > +	ima_algo_array = kmalloc_array(ima_tpm_chip-
> > >nr_allocated_banks + 1,
> > > +				       sizeof(*ima_algo_array), GFP_KERNEL);
> > 
> > Perhaps instead of hard coding "nr_allocated_banks + 1", create a
> > variable for storing the number of "extra" banks.  Walk the banks
> > setting ima_sha1_idx and, later, in a subsequent patch set
> > ima_hash_algo_idx.
> 
> I could store the indexes in an array and add ARRAY_SIZE of that array.

Your current patches are straight forward and easy to review.  I
really like that.  What I'm proposing is a minor change, at least I
think it is a minor change.  I trust whatever change you decide to
make will be just as easy to review.

Before re-posting, please define ima_sha1_idx and ima_hash_algo_idx as
__ro_after_init.

thanks,

Mimi
diff mbox series

Patch

diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
index 57b06321d5c8..63fb4bdf80b0 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -59,7 +59,14 @@  MODULE_PARM_DESC(ahash_bufsize, "Maximum ahash buffer size");
 static struct crypto_shash *ima_shash_tfm;
 static struct crypto_ahash *ima_ahash_tfm;
 
-int __init ima_init_crypto(void)
+struct ima_algo_desc {
+	struct crypto_shash *tfm;
+	enum hash_algo algo;
+};
+
+static struct ima_algo_desc *ima_algo_array;
+
+int __init ima_init_ima_crypto(void)
 {
 	long rc;
 
@@ -78,26 +85,116 @@  int __init ima_init_crypto(void)
 static struct crypto_shash *ima_alloc_tfm(enum hash_algo algo)
 {
 	struct crypto_shash *tfm = ima_shash_tfm;
-	int rc;
+	int rc, i;
 
 	if (algo < 0 || algo >= HASH_ALGO__LAST)
 		algo = ima_hash_algo;
 
-	if (algo != ima_hash_algo) {
-		tfm = crypto_alloc_shash(hash_algo_name[algo], 0, 0);
-		if (IS_ERR(tfm)) {
-			rc = PTR_ERR(tfm);
-			pr_err("Can not allocate %s (reason: %d)\n",
-			       hash_algo_name[algo], rc);
-		}
+	if (algo == ima_hash_algo)
+		return tfm;
+
+	for (i = 0; i < ima_tpm_chip->nr_allocated_banks + 1; i++)
+		if (ima_algo_array[i].algo == algo && ima_algo_array[i].tfm)
+			return ima_algo_array[i].tfm;
+
+	tfm = crypto_alloc_shash(hash_algo_name[algo], 0, 0);
+	if (IS_ERR(tfm)) {
+		rc = PTR_ERR(tfm);
+		pr_err("Can not allocate %s (reason: %d)\n",
+		       hash_algo_name[algo], rc);
 	}
 	return tfm;
 }
 
+int __init ima_init_crypto(void)
+{
+	enum hash_algo algo;
+	long rc;
+	int i;
+
+	rc = ima_init_ima_crypto();
+	if (rc)
+		return rc;
+
+	ima_algo_array = kmalloc_array(ima_tpm_chip->nr_allocated_banks + 1,
+				       sizeof(*ima_algo_array), GFP_KERNEL);
+	if (!ima_algo_array) {
+		rc = -ENOMEM;
+		goto out;
+	}
+
+	for (i = 0; i < ima_tpm_chip->nr_allocated_banks + 1; i++) {
+		ima_algo_array[i].tfm = NULL;
+		ima_algo_array[i].algo = HASH_ALGO__LAST;
+	}
+
+	ima_sha1_idx = -1;
+
+	for (i = 0; i < ima_tpm_chip->nr_allocated_banks; i++) {
+		algo = ima_tpm_chip->allocated_banks[i].crypto_id;
+		ima_algo_array[i].algo = algo;
+
+		/* unknown TPM algorithm */
+		if (algo == HASH_ALGO__LAST)
+			continue;
+
+		if (algo == HASH_ALGO_SHA1)
+			ima_sha1_idx = i;
+
+		if (algo == ima_hash_algo) {
+			ima_algo_array[i].tfm = ima_shash_tfm;
+			continue;
+		}
+
+		ima_algo_array[i].tfm = ima_alloc_tfm(algo);
+		if (IS_ERR(ima_algo_array[i].tfm)) {
+			if (algo == HASH_ALGO_SHA1) {
+				rc = PTR_ERR(ima_algo_array[i].tfm);
+				ima_algo_array[i].tfm = NULL;
+				goto out_array;
+			}
+
+			ima_algo_array[i].tfm = NULL;
+		}
+	}
+
+	if (ima_sha1_idx < 0) {
+		ima_algo_array[i].tfm = ima_alloc_tfm(HASH_ALGO_SHA1);
+		if (IS_ERR(ima_algo_array[i].tfm)) {
+			rc = PTR_ERR(ima_algo_array[i].tfm);
+			goto out_array;
+		}
+
+		ima_sha1_idx = i;
+		ima_algo_array[i].algo = HASH_ALGO_SHA1;
+	}
+
+	return 0;
+out_array:
+	for (i = 0; i < ima_tpm_chip->nr_allocated_banks + 1; i++) {
+		if (!ima_algo_array[i].tfm ||
+		    ima_algo_array[i].tfm == ima_shash_tfm)
+			continue;
+
+		crypto_free_shash(ima_algo_array[i].tfm);
+	}
+out:
+	crypto_free_shash(ima_shash_tfm);
+	return rc;
+}
+
 static void ima_free_tfm(struct crypto_shash *tfm)
 {
-	if (tfm != ima_shash_tfm)
-		crypto_free_shash(tfm);
+	int i;
+
+	if (tfm == ima_shash_tfm)
+		return;
+
+	for (i = 0; i < ima_tpm_chip->nr_allocated_banks + 1; i++)
+		if (ima_algo_array[i].tfm == tfm)
+			return;
+
+	crypto_free_shash(tfm);
 }
 
 /**
@@ -467,14 +564,14 @@  int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash)
  */
 static int ima_calc_field_array_hash_tfm(struct ima_field_data *field_data,
 					 struct ima_template_entry *entry,
-					 struct crypto_shash *tfm)
+					 int tfm_idx)
 {
-	SHASH_DESC_ON_STACK(shash, tfm);
+	SHASH_DESC_ON_STACK(shash, ima_algo_array[tfm_idx].tfm);
 	struct ima_template_desc *td = entry->template_desc;
 	int num_fields = entry->template_desc->num_fields;
 	int rc, i;
 
-	shash->tfm = tfm;
+	shash->tfm = ima_algo_array[tfm_idx].tfm;
 
 	rc = crypto_shash_init(shash);
 	if (rc != 0)
@@ -505,7 +602,7 @@  static int ima_calc_field_array_hash_tfm(struct ima_field_data *field_data,
 
 	if (!rc)
 		rc = crypto_shash_final(shash,
-					entry->digests[ima_sha1_idx].digest);
+					entry->digests[tfm_idx].digest);
 
 	return rc;
 }
@@ -513,17 +610,9 @@  static int ima_calc_field_array_hash_tfm(struct ima_field_data *field_data,
 int ima_calc_field_array_hash(struct ima_field_data *field_data,
 			      struct ima_template_entry *entry)
 {
-	struct crypto_shash *tfm;
 	int rc;
 
-	tfm = ima_alloc_tfm(HASH_ALGO_SHA1);
-	if (IS_ERR(tfm))
-		return PTR_ERR(tfm);
-
-	rc = ima_calc_field_array_hash_tfm(field_data, entry, tfm);
-
-	ima_free_tfm(tfm);
-
+	rc = ima_calc_field_array_hash_tfm(field_data, entry, ima_sha1_idx);
 	return rc;
 }