[PATCH v1 0/3] ima: use asynchronous hash API for hash calculation

[PATCH v1 0/3] ima: use asynchronous hash API for hash calculation

[Dmitry Kasatkin]

Depending on the IMA policy, it might require to measure huge amount of files.
It may be very important to speedup hash calculation or to reduce (bettery)
energy required to do it. Currently IMA uses synchronous hash API (shash)
which is CPU based. CPU based hash calculation is very CPU intensive and on the
battery powered device will be also high energy consuming.

Many platforms provide cryptographic acceleration modules which allow speedup
and/or reduce energy consumption, and provide ansyhchronous way to calculate
hashes. Defacto way to implement drivers for such accelerators is using
asynchronous hash API (ahash).

The first patch adds use of ahash API to IMA. Performance of using HW
acceleration depends very much on amount of data to hash and it depends
on particular HW. It is usually inefficient for small data due to HW
initialization overhead. In order to make it possible to optimize performance
for particular system, the patch provides kernel parameter 'ima_ahash_size=size'
which allows to specify optimal data size when start using ahash. By default
ahash is dsiabled until non-zero value to 'ima_ahash_size' is provided.

Second patch introduces multi-page buffers which makes HW acceleration more
efficient. It adds kernel parameter to specify buffer size to use.

Third patch introduces double-buffering which allows to readahead next portion
of data for hashing while calculating the hash.

- Dmitry

Dmitry Kasatkin (3):
  ima: use ahash API for file hash calculation
  ima: introduce multi-page collect buffers
  ima: provide double buffering for hash calculation

 Documentation/kernel-parameters.txt |   6 +
 security/integrity/ima/ima_crypto.c | 282 +++++++++++++++++++++++++++++++++++-
 2 files changed, 285 insertions(+), 3 deletions(-)

-- 
1.9.1

[PATCH v1 1/3] ima: use ahash API for file hash calculation

[Dmitry Kasatkin]

Async hash API allows to use HW acceleration for hash calculation.
It may give significant performance gain or/and reduce power consumption,
which might be very beneficial for battery powered devices.

This patch introduces hash calculation using ahash API.

ahash peformance depends on data size and particular HW. Under certain
limit, depending on the system, shash performance may be better.

This patch also introduces 'ima_ahash_size' kernel parameter which can
be used to defines minimal data size to use with ahash. When this
parameter is not set or file size is smaller than defined by this
parameter, shash will be used. Thus, by defult, original shash
implementation is used.

Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
---
 Documentation/kernel-parameters.txt |   3 +
 security/integrity/ima/ima_crypto.c | 182 +++++++++++++++++++++++++++++++++++-
 2 files changed, 181 insertions(+), 4 deletions(-)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index a0c155c..f8efb01 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -1286,6 +1286,9 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 	ihash_entries=	[KNL]
 			Set number of hash buckets for inode cache.
 
+	ima_ahash_size=size [IMA]
+			Set the minimal file size when use ahash API.
+
 	ima_appraise=	[IMA] appraise integrity measurements
 			Format: { "off" | "enforce" | "fix" }
 			default: "enforce"
diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
index ccd0ac8..b7a8650 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -25,7 +25,25 @@
 #include <crypto/hash_info.h>
 #include "ima.h"
 
+
+struct ahash_completion {
+	struct completion completion;
+	int err;
+};
+
 static struct crypto_shash *ima_shash_tfm;
+static struct crypto_ahash *ima_ahash_tfm;
+
+/* data size for ahash use */
+static loff_t ima_ahash_size;
+
+static int __init ima_ahash_setup(char *str)
+{
+	int rc = kstrtoll(str, 10, &ima_ahash_size);
+	pr_info("ima_ahash_size = %lld\n", ima_ahash_size);
+	return !rc;
+}
+__setup("ima_ahash_size=", ima_ahash_setup);
 
 /**
  * ima_kernel_read - read file content
@@ -68,6 +86,14 @@ int ima_init_crypto(void)
 		       hash_algo_name[ima_hash_algo], rc);
 		return rc;
 	}
+	ima_ahash_tfm = crypto_alloc_ahash(hash_algo_name[ima_hash_algo], 0, 0);
+	if (IS_ERR(ima_ahash_tfm)) {
+		rc = PTR_ERR(ima_ahash_tfm);
+		crypto_free_shash(ima_shash_tfm);
+		pr_err("Can not allocate %s (reason: %ld)\n",
+		       hash_algo_name[ima_hash_algo], rc);
+		return rc;
+	}
 	return 0;
 }
 
@@ -93,9 +119,143 @@ static void ima_free_tfm(struct crypto_shash *tfm)
 		crypto_free_shash(tfm);
 }
 
-/*
- * Calculate the MD5/SHA1 file digest
- */
+static struct crypto_ahash *ima_alloc_atfm(enum hash_algo algo)
+{
+	struct crypto_ahash *tfm = ima_ahash_tfm;
+	int rc;
+
+	if (algo != ima_hash_algo && algo < HASH_ALGO__LAST) {
+		tfm = crypto_alloc_ahash(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;
+}
+
+static void ima_free_atfm(struct crypto_ahash *tfm)
+{
+	if (tfm != ima_ahash_tfm)
+		crypto_free_ahash(tfm);
+}
+
+static void ahash_complete(struct crypto_async_request *req, int err)
+{
+	struct ahash_completion *res = req->data;
+
+	if (err == -EINPROGRESS)
+		return;
+	res->err = err;
+	complete(&res->completion);
+}
+
+static int ahash_wait(int err, struct ahash_completion *res)
+{
+	switch (err) {
+	case 0:
+		break;
+	case -EINPROGRESS:
+	case -EBUSY:
+		wait_for_completion(&res->completion);
+		reinit_completion(&res->completion);
+		err = res->err;
+		/* fall through */
+	default:
+		pr_crit("ahash calculation failed: err: %d\n", err);
+	}
+
+	return err;
+}
+
+static int ima_calc_file_hash_atfm(struct file *file,
+				   struct ima_digest_data *hash,
+				   struct crypto_ahash *tfm)
+{
+	loff_t i_size, offset;
+	char *rbuf;
+	int rc, read = 0, rbuf_len;
+	struct ahash_request *req;
+	struct scatterlist sg[1];
+	struct ahash_completion res;
+
+	hash->length = crypto_ahash_digestsize(tfm);
+
+	req = ahash_request_alloc(ima_ahash_tfm, GFP_KERNEL);
+	if (!req)
+		return -ENOMEM;
+
+	init_completion(&res.completion);
+	ahash_request_set_callback(req, CRYPTO_TFM_REQ_MAY_BACKLOG |
+				   CRYPTO_TFM_REQ_MAY_SLEEP,
+				   ahash_complete, &res);
+
+	rc = ahash_wait(crypto_ahash_init(req), &res);
+	if (rc)
+		goto out1;
+
+	i_size = i_size_read(file_inode(file));
+
+	if (i_size == 0)
+		goto out2;
+
+	rbuf = kzalloc(PAGE_SIZE, GFP_KERNEL);
+	if (!rbuf) {
+		rc = -ENOMEM;
+		goto out1;
+	}
+
+	if (!(file->f_mode & FMODE_READ)) {
+		file->f_mode |= FMODE_READ;
+		read = 1;
+	}
+
+	for (offset = 0; offset < i_size; offset += rbuf_len) {
+		rbuf_len = ima_kernel_read(file, offset, rbuf, PAGE_SIZE);
+		if (rbuf_len < 0) {
+			rc = rbuf_len;
+			break;
+		}
+		if (rbuf_len == 0)
+			break;
+
+		sg_init_one(&sg[0], rbuf, rbuf_len);
+		ahash_request_set_crypt(req, sg, NULL, rbuf_len);
+
+		rc = ahash_wait(crypto_ahash_update(req), &res);
+		if (rc)
+			break;
+	}
+	if (read)
+		file->f_mode &= ~FMODE_READ;
+	kfree(rbuf);
+out2:
+	if (!rc) {
+		ahash_request_set_crypt(req, NULL, hash->digest, 0);
+		rc = ahash_wait(crypto_ahash_final(req), &res);
+	}
+out1:
+	ahash_request_free(req);
+	return rc;
+}
+
+static int ima_calc_file_ahash(struct file *file, struct ima_digest_data *hash)
+{
+	struct crypto_ahash *tfm;
+	int rc;
+
+	tfm = ima_alloc_atfm(hash->algo);
+	if (IS_ERR(tfm))
+		return PTR_ERR(tfm);
+
+	rc = ima_calc_file_hash_atfm(file, hash, tfm);
+
+	ima_free_atfm(tfm);
+
+	return rc;
+}
+
 static int ima_calc_file_hash_tfm(struct file *file,
 				  struct ima_digest_data *hash,
 				  struct crypto_shash *tfm)
@@ -156,7 +316,7 @@ out:
 	return rc;
 }
 
-int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash)
+static int ima_calc_file_shash(struct file *file, struct ima_digest_data *hash)
 {
 	struct crypto_shash *tfm;
 	int rc;
@@ -172,6 +332,20 @@ int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash)
 	return rc;
 }
 
+int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash)
+{
+	loff_t i_size = i_size_read(file_inode(file));
+
+	/* shash is more efficient small data
+	 * ahash performance depends on data size and particular HW
+	 * ima_ahash_size allows to specify the best value for the system
+	 */
+	if (ima_ahash_size && i_size >= ima_ahash_size)
+		return ima_calc_file_ahash(file, hash);
+	else
+		return ima_calc_file_shash(file, hash);
+}
+
 /*
  * Calculate the hash of template data
  */
-- 
1.9.1

Re: [PATCH v1 1/3] ima: use ahash API for file hash calculation

[Mimi Zohar]

On Thu, 2014-06-19 at 18:20 +0300, Dmitry Kasatkin wrote: 
> Async hash API allows to use HW acceleration for hash calculation.
> It may give significant performance gain or/and reduce power consumption,
> which might be very beneficial for battery powered devices.
> 
> This patch introduces hash calculation using ahash API.
> 
> ahash peformance depends on data size and particular HW. Under certain
> limit, depending on the system, shash performance may be better.
> 
> This patch also introduces 'ima_ahash_size' kernel parameter which can
> be used to defines minimal data size to use with ahash. When this
> parameter is not set or file size is smaller than defined by this
> parameter, shash will be used. Thus, by defult, original shash
> implementation is used.
> 
> Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com>

Thanks, Dmitry.  What type of values are you using for ima_ahash_size?
Have you specified ima_ahash_size, when booting with an initramfs?
ahash should probably only be used after pivoting root.

thanks,

Mimi

> ---
>  Documentation/kernel-parameters.txt |   3 +
>  security/integrity/ima/ima_crypto.c | 182 +++++++++++++++++++++++++++++++++++-
>  2 files changed, 181 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> index a0c155c..f8efb01 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -1286,6 +1286,9 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
>  	ihash_entries=	[KNL]
>  			Set number of hash buckets for inode cache.
> 
> +	ima_ahash_size=size [IMA]
> +			Set the minimal file size when use ahash API.
> +
>  	ima_appraise=	[IMA] appraise integrity measurements
>  			Format: { "off" | "enforce" | "fix" }
>  			default: "enforce"
> diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
> index ccd0ac8..b7a8650 100644
> --- a/security/integrity/ima/ima_crypto.c
> +++ b/security/integrity/ima/ima_crypto.c
> @@ -25,7 +25,25 @@
>  #include <crypto/hash_info.h>
>  #include "ima.h"
> 
> +
> +struct ahash_completion {
> +	struct completion completion;
> +	int err;
> +};
> +
>  static struct crypto_shash *ima_shash_tfm;
> +static struct crypto_ahash *ima_ahash_tfm;
> +
> +/* data size for ahash use */
> +static loff_t ima_ahash_size;
> +
> +static int __init ima_ahash_setup(char *str)
> +{
> +	int rc = kstrtoll(str, 10, &ima_ahash_size);
> +	pr_info("ima_ahash_size = %lld\n", ima_ahash_size);
> +	return !rc;
> +}
> +__setup("ima_ahash_size=", ima_ahash_setup);
> 
>  /**
>   * ima_kernel_read - read file content
> @@ -68,6 +86,14 @@ int ima_init_crypto(void)
>  		       hash_algo_name[ima_hash_algo], rc);
>  		return rc;
>  	}
> +	ima_ahash_tfm = crypto_alloc_ahash(hash_algo_name[ima_hash_algo], 0, 0);
> +	if (IS_ERR(ima_ahash_tfm)) {
> +		rc = PTR_ERR(ima_ahash_tfm);
> +		crypto_free_shash(ima_shash_tfm);
> +		pr_err("Can not allocate %s (reason: %ld)\n",
> +		       hash_algo_name[ima_hash_algo], rc);
> +		return rc;
> +	}
>  	return 0;
>  }
> 
> @@ -93,9 +119,143 @@ static void ima_free_tfm(struct crypto_shash *tfm)
>  		crypto_free_shash(tfm);
>  }
> 
> -/*
> - * Calculate the MD5/SHA1 file digest
> - */
> +static struct crypto_ahash *ima_alloc_atfm(enum hash_algo algo)
> +{
> +	struct crypto_ahash *tfm = ima_ahash_tfm;
> +	int rc;
> +
> +	if (algo != ima_hash_algo && algo < HASH_ALGO__LAST) {
> +		tfm = crypto_alloc_ahash(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;
> +}
> +
> +static void ima_free_atfm(struct crypto_ahash *tfm)
> +{
> +	if (tfm != ima_ahash_tfm)
> +		crypto_free_ahash(tfm);
> +}
> +
> +static void ahash_complete(struct crypto_async_request *req, int err)
> +{
> +	struct ahash_completion *res = req->data;
> +
> +	if (err == -EINPROGRESS)
> +		return;
> +	res->err = err;
> +	complete(&res->completion);
> +}
> +
> +static int ahash_wait(int err, struct ahash_completion *res)
> +{
> +	switch (err) {
> +	case 0:
> +		break;
> +	case -EINPROGRESS:
> +	case -EBUSY:
> +		wait_for_completion(&res->completion);
> +		reinit_completion(&res->completion);
> +		err = res->err;
> +		/* fall through */
> +	default:
> +		pr_crit("ahash calculation failed: err: %d\n", err);
> +	}
> +
> +	return err;
> +}
> +
> +static int ima_calc_file_hash_atfm(struct file *file,
> +				   struct ima_digest_data *hash,
> +				   struct crypto_ahash *tfm)
> +{
> +	loff_t i_size, offset;
> +	char *rbuf;
> +	int rc, read = 0, rbuf_len;
> +	struct ahash_request *req;
> +	struct scatterlist sg[1];
> +	struct ahash_completion res;
> +
> +	hash->length = crypto_ahash_digestsize(tfm);
> +
> +	req = ahash_request_alloc(ima_ahash_tfm, GFP_KERNEL);
> +	if (!req)
> +		return -ENOMEM;
> +
> +	init_completion(&res.completion);
> +	ahash_request_set_callback(req, CRYPTO_TFM_REQ_MAY_BACKLOG |
> +				   CRYPTO_TFM_REQ_MAY_SLEEP,
> +				   ahash_complete, &res);
> +
> +	rc = ahash_wait(crypto_ahash_init(req), &res);
> +	if (rc)
> +		goto out1;
> +
> +	i_size = i_size_read(file_inode(file));
> +
> +	if (i_size == 0)
> +		goto out2;
> +
> +	rbuf = kzalloc(PAGE_SIZE, GFP_KERNEL);
> +	if (!rbuf) {
> +		rc = -ENOMEM;
> +		goto out1;
> +	}
> +
> +	if (!(file->f_mode & FMODE_READ)) {
> +		file->f_mode |= FMODE_READ;
> +		read = 1;
> +	}
> +
> +	for (offset = 0; offset < i_size; offset += rbuf_len) {
> +		rbuf_len = ima_kernel_read(file, offset, rbuf, PAGE_SIZE);
> +		if (rbuf_len < 0) {
> +			rc = rbuf_len;
> +			break;
> +		}
> +		if (rbuf_len == 0)
> +			break;
> +
> +		sg_init_one(&sg[0], rbuf, rbuf_len);
> +		ahash_request_set_crypt(req, sg, NULL, rbuf_len);
> +
> +		rc = ahash_wait(crypto_ahash_update(req), &res);
> +		if (rc)
> +			break;
> +	}
> +	if (read)
> +		file->f_mode &= ~FMODE_READ;
> +	kfree(rbuf);
> +out2:
> +	if (!rc) {
> +		ahash_request_set_crypt(req, NULL, hash->digest, 0);
> +		rc = ahash_wait(crypto_ahash_final(req), &res);
> +	}
> +out1:
> +	ahash_request_free(req);
> +	return rc;
> +}
> +
> +static int ima_calc_file_ahash(struct file *file, struct ima_digest_data *hash)
> +{
> +	struct crypto_ahash *tfm;
> +	int rc;
> +
> +	tfm = ima_alloc_atfm(hash->algo);
> +	if (IS_ERR(tfm))
> +		return PTR_ERR(tfm);
> +
> +	rc = ima_calc_file_hash_atfm(file, hash, tfm);
> +
> +	ima_free_atfm(tfm);
> +
> +	return rc;
> +}
> +
>  static int ima_calc_file_hash_tfm(struct file *file,
>  				  struct ima_digest_data *hash,
>  				  struct crypto_shash *tfm)
> @@ -156,7 +316,7 @@ out:
>  	return rc;
>  }
> 
> -int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash)
> +static int ima_calc_file_shash(struct file *file, struct ima_digest_data *hash)
>  {
>  	struct crypto_shash *tfm;
>  	int rc;
> @@ -172,6 +332,20 @@ int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash)
>  	return rc;
>  }
> 
> +int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash)
> +{
> +	loff_t i_size = i_size_read(file_inode(file));
> +
> +	/* shash is more efficient small data
> +	 * ahash performance depends on data size and particular HW
> +	 * ima_ahash_size allows to specify the best value for the system
> +	 */
> +	if (ima_ahash_size && i_size >= ima_ahash_size)
> +		return ima_calc_file_ahash(file, hash);
> +	else
> +		return ima_calc_file_shash(file, hash);
> +}
> +
>  /*
>   * Calculate the hash of template data
>   */

Re: [PATCH v1 1/3] ima: use ahash API for file hash calculation

[Dmitry Kasatkin]

On 23/06/14 14:32, Mimi Zohar wrote:
> On Thu, 2014-06-19 at 18:20 +0300, Dmitry Kasatkin wrote: 
>> Async hash API allows to use HW acceleration for hash calculation.
>> It may give significant performance gain or/and reduce power consumption,
>> which might be very beneficial for battery powered devices.
>>
>> This patch introduces hash calculation using ahash API.
>>
>> ahash peformance depends on data size and particular HW. Under certain
>> limit, depending on the system, shash performance may be better.
>>
>> This patch also introduces 'ima_ahash_size' kernel parameter which can
>> be used to defines minimal data size to use with ahash. When this
>> parameter is not set or file size is smaller than defined by this
>> parameter, shash will be used. Thus, by defult, original shash
>> implementation is used.
>>
>> Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
> Thanks, Dmitry.  What type of values are you using for ima_ahash_size?
> Have you specified ima_ahash_size, when booting with an initramfs?
> ahash should probably only be used after pivoting root.
>
> thanks,

Hi,

There is no guideline because it depends on the HW...
On SW it does not mater.
To check it up and compare you can set it to "1" to make it working for
"any" file size.

- Dmitry


>
> Mimi
>
>> ---
>>  Documentation/kernel-parameters.txt |   3 +
>>  security/integrity/ima/ima_crypto.c | 182 +++++++++++++++++++++++++++++++++++-
>>  2 files changed, 181 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
>> index a0c155c..f8efb01 100644
>> --- a/Documentation/kernel-parameters.txt
>> +++ b/Documentation/kernel-parameters.txt
>> @@ -1286,6 +1286,9 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
>>  	ihash_entries=	[KNL]
>>  			Set number of hash buckets for inode cache.
>>
>> +	ima_ahash_size=size [IMA]
>> +			Set the minimal file size when use ahash API.
>> +
>>  	ima_appraise=	[IMA] appraise integrity measurements
>>  			Format: { "off" | "enforce" | "fix" }
>>  			default: "enforce"
>> diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
>> index ccd0ac8..b7a8650 100644
>> --- a/security/integrity/ima/ima_crypto.c
>> +++ b/security/integrity/ima/ima_crypto.c
>> @@ -25,7 +25,25 @@
>>  #include <crypto/hash_info.h>
>>  #include "ima.h"
>>
>> +
>> +struct ahash_completion {
>> +	struct completion completion;
>> +	int err;
>> +};
>> +
>>  static struct crypto_shash *ima_shash_tfm;
>> +static struct crypto_ahash *ima_ahash_tfm;
>> +
>> +/* data size for ahash use */
>> +static loff_t ima_ahash_size;
>> +
>> +static int __init ima_ahash_setup(char *str)
>> +{
>> +	int rc = kstrtoll(str, 10, &ima_ahash_size);
>> +	pr_info("ima_ahash_size = %lld\n", ima_ahash_size);
>> +	return !rc;
>> +}
>> +__setup("ima_ahash_size=", ima_ahash_setup);
>>
>>  /**
>>   * ima_kernel_read - read file content
>> @@ -68,6 +86,14 @@ int ima_init_crypto(void)
>>  		       hash_algo_name[ima_hash_algo], rc);
>>  		return rc;
>>  	}
>> +	ima_ahash_tfm = crypto_alloc_ahash(hash_algo_name[ima_hash_algo], 0, 0);
>> +	if (IS_ERR(ima_ahash_tfm)) {
>> +		rc = PTR_ERR(ima_ahash_tfm);
>> +		crypto_free_shash(ima_shash_tfm);
>> +		pr_err("Can not allocate %s (reason: %ld)\n",
>> +		       hash_algo_name[ima_hash_algo], rc);
>> +		return rc;
>> +	}
>>  	return 0;
>>  }
>>
>> @@ -93,9 +119,143 @@ static void ima_free_tfm(struct crypto_shash *tfm)
>>  		crypto_free_shash(tfm);
>>  }
>>
>> -/*
>> - * Calculate the MD5/SHA1 file digest
>> - */
>> +static struct crypto_ahash *ima_alloc_atfm(enum hash_algo algo)
>> +{
>> +	struct crypto_ahash *tfm = ima_ahash_tfm;
>> +	int rc;
>> +
>> +	if (algo != ima_hash_algo && algo < HASH_ALGO__LAST) {
>> +		tfm = crypto_alloc_ahash(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;
>> +}
>> +
>> +static void ima_free_atfm(struct crypto_ahash *tfm)
>> +{
>> +	if (tfm != ima_ahash_tfm)
>> +		crypto_free_ahash(tfm);
>> +}
>> +
>> +static void ahash_complete(struct crypto_async_request *req, int err)
>> +{
>> +	struct ahash_completion *res = req->data;
>> +
>> +	if (err == -EINPROGRESS)
>> +		return;
>> +	res->err = err;
>> +	complete(&res->completion);
>> +}
>> +
>> +static int ahash_wait(int err, struct ahash_completion *res)
>> +{
>> +	switch (err) {
>> +	case 0:
>> +		break;
>> +	case -EINPROGRESS:
>> +	case -EBUSY:
>> +		wait_for_completion(&res->completion);
>> +		reinit_completion(&res->completion);
>> +		err = res->err;
>> +		/* fall through */
>> +	default:
>> +		pr_crit("ahash calculation failed: err: %d\n", err);
>> +	}
>> +
>> +	return err;
>> +}
>> +
>> +static int ima_calc_file_hash_atfm(struct file *file,
>> +				   struct ima_digest_data *hash,
>> +				   struct crypto_ahash *tfm)
>> +{
>> +	loff_t i_size, offset;
>> +	char *rbuf;
>> +	int rc, read = 0, rbuf_len;
>> +	struct ahash_request *req;
>> +	struct scatterlist sg[1];
>> +	struct ahash_completion res;
>> +
>> +	hash->length = crypto_ahash_digestsize(tfm);
>> +
>> +	req = ahash_request_alloc(ima_ahash_tfm, GFP_KERNEL);
>> +	if (!req)
>> +		return -ENOMEM;
>> +
>> +	init_completion(&res.completion);
>> +	ahash_request_set_callback(req, CRYPTO_TFM_REQ_MAY_BACKLOG |
>> +				   CRYPTO_TFM_REQ_MAY_SLEEP,
>> +				   ahash_complete, &res);
>> +
>> +	rc = ahash_wait(crypto_ahash_init(req), &res);
>> +	if (rc)
>> +		goto out1;
>> +
>> +	i_size = i_size_read(file_inode(file));
>> +
>> +	if (i_size == 0)
>> +		goto out2;
>> +
>> +	rbuf = kzalloc(PAGE_SIZE, GFP_KERNEL);
>> +	if (!rbuf) {
>> +		rc = -ENOMEM;
>> +		goto out1;
>> +	}
>> +
>> +	if (!(file->f_mode & FMODE_READ)) {
>> +		file->f_mode |= FMODE_READ;
>> +		read = 1;
>> +	}
>> +
>> +	for (offset = 0; offset < i_size; offset += rbuf_len) {
>> +		rbuf_len = ima_kernel_read(file, offset, rbuf, PAGE_SIZE);
>> +		if (rbuf_len < 0) {
>> +			rc = rbuf_len;
>> +			break;
>> +		}
>> +		if (rbuf_len == 0)
>> +			break;
>> +
>> +		sg_init_one(&sg[0], rbuf, rbuf_len);
>> +		ahash_request_set_crypt(req, sg, NULL, rbuf_len);
>> +
>> +		rc = ahash_wait(crypto_ahash_update(req), &res);
>> +		if (rc)
>> +			break;
>> +	}
>> +	if (read)
>> +		file->f_mode &= ~FMODE_READ;
>> +	kfree(rbuf);
>> +out2:
>> +	if (!rc) {
>> +		ahash_request_set_crypt(req, NULL, hash->digest, 0);
>> +		rc = ahash_wait(crypto_ahash_final(req), &res);
>> +	}
>> +out1:
>> +	ahash_request_free(req);
>> +	return rc;
>> +}
>> +
>> +static int ima_calc_file_ahash(struct file *file, struct ima_digest_data *hash)
>> +{
>> +	struct crypto_ahash *tfm;
>> +	int rc;
>> +
>> +	tfm = ima_alloc_atfm(hash->algo);
>> +	if (IS_ERR(tfm))
>> +		return PTR_ERR(tfm);
>> +
>> +	rc = ima_calc_file_hash_atfm(file, hash, tfm);
>> +
>> +	ima_free_atfm(tfm);
>> +
>> +	return rc;
>> +}
>> +
>>  static int ima_calc_file_hash_tfm(struct file *file,
>>  				  struct ima_digest_data *hash,
>>  				  struct crypto_shash *tfm)
>> @@ -156,7 +316,7 @@ out:
>>  	return rc;
>>  }
>>
>> -int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash)
>> +static int ima_calc_file_shash(struct file *file, struct ima_digest_data *hash)
>>  {
>>  	struct crypto_shash *tfm;
>>  	int rc;
>> @@ -172,6 +332,20 @@ int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash)
>>  	return rc;
>>  }
>>
>> +int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash)
>> +{
>> +	loff_t i_size = i_size_read(file_inode(file));
>> +
>> +	/* shash is more efficient small data
>> +	 * ahash performance depends on data size and particular HW
>> +	 * ima_ahash_size allows to specify the best value for the system
>> +	 */
>> +	if (ima_ahash_size && i_size >= ima_ahash_size)
>> +		return ima_calc_file_ahash(file, hash);
>> +	else
>> +		return ima_calc_file_shash(file, hash);
>> +}
>> +
>>  /*
>>   * Calculate the hash of template data
>>   */
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH v1 1/3] ima: use ahash API for file hash calculation

[Mimi Zohar]

On Thu, 2014-06-19 at 18:20 +0300, Dmitry Kasatkin wrote:
> Async hash API allows to use HW acceleration for hash calculation.
> It may give significant performance gain or/and reduce power consumption,
> which might be very beneficial for battery powered devices.
> 
> This patch introduces hash calculation using ahash API.
> 
> ahash peformance depends on data size and particular HW. Under certain
> limit, depending on the system, shash performance may be better.
> 
> This patch also introduces 'ima_ahash_size' kernel parameter which can
> be used to defines minimal data size to use with ahash. When this
> parameter is not set or file size is smaller than defined by this
> parameter, shash will be used. Thus, by defult, original shash
> implementation is used.
> 
> Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
> ---
>  Documentation/kernel-parameters.txt |   3 +
>  security/integrity/ima/ima_crypto.c | 182 +++++++++++++++++++++++++++++++++++-
>  2 files changed, 181 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> index a0c155c..f8efb01 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -1286,6 +1286,9 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
>  	ihash_entries=	[KNL]
>  			Set number of hash buckets for inode cache.
> 
> +	ima_ahash_size=size [IMA]
> +			Set the minimal file size when use ahash API.
> +
>  	ima_appraise=	[IMA] appraise integrity measurements
>  			Format: { "off" | "enforce" | "fix" }
>  			default: "enforce"
> diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
> index ccd0ac8..b7a8650 100644
> --- a/security/integrity/ima/ima_crypto.c
> +++ b/security/integrity/ima/ima_crypto.c
> @@ -25,7 +25,25 @@
>  #include <crypto/hash_info.h>
>  #include "ima.h"
> 
> +
> +struct ahash_completion {
> +	struct completion completion;
> +	int err;
> +};
> +
>  static struct crypto_shash *ima_shash_tfm;
> +static struct crypto_ahash *ima_ahash_tfm;
> +
> +/* data size for ahash use */
> +static loff_t ima_ahash_size;
> +
> +static int __init ima_ahash_setup(char *str)
> +{
> +	int rc = kstrtoll(str, 10, &ima_ahash_size);

In general, variable definitions should be separated from code.  A
simple initialization is fine.  Please separate variable definitions
from code with a blank line. 

> +	pr_info("ima_ahash_size = %lld\n", ima_ahash_size);
> +	return !rc;
> +}
> +__setup("ima_ahash_size=", ima_ahash_setup);

This boot parameter name doesn't reflect its purpose, defining the
minimum file size for using ahash. The next patch defines an additional
boot parameter ima_ahash_bufsize. Perhaps defining a single boot
parameter (eg. ima_ahash=) with multiple fields would be better. 

>  /**
>   * ima_kernel_read - read file content
> @@ -68,6 +86,14 @@ int ima_init_crypto(void)
>  		       hash_algo_name[ima_hash_algo], rc);
>  		return rc;
>  	}
> +	ima_ahash_tfm = crypto_alloc_ahash(hash_algo_name[ima_hash_algo], 0, 0);
> +	if (IS_ERR(ima_ahash_tfm)) {
> +		rc = PTR_ERR(ima_ahash_tfm);
> +		crypto_free_shash(ima_shash_tfm);

Only crypto_alloc_ahash() failed, not crypto_alloc_shash(). shash has
worked fine up to now. Why require both shash and ahash to succeed? 

> +		pr_err("Can not allocate %s (reason: %ld)\n",
> +		       hash_algo_name[ima_hash_algo], rc);
> +		return rc;
> +	}
>  	return 0;
>  }
> 

> @@ -93,9 +119,143 @@ static void ima_free_tfm(struct crypto_shash *tfm)
>  		crypto_free_shash(tfm);
>  }
> 
> -/*
> - * Calculate the MD5/SHA1 file digest
> - */
> +static struct crypto_ahash *ima_alloc_atfm(enum hash_algo algo)
> +{
> +	struct crypto_ahash *tfm = ima_ahash_tfm;
> +	int rc;
> +
> +	if (algo != ima_hash_algo && algo < HASH_ALGO__LAST) {
> +		tfm = crypto_alloc_ahash(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;
> +}
> +
> +static void ima_free_atfm(struct crypto_ahash *tfm)
> +{
> +	if (tfm != ima_ahash_tfm)
> +		crypto_free_ahash(tfm);
> +}
> +
> +static void ahash_complete(struct crypto_async_request *req, int err)
> +{
> +	struct ahash_completion *res = req->data;
> +
> +	if (err == -EINPROGRESS)
> +		return;
> +	res->err = err;
> +	complete(&res->completion);
> +}
> +
> +static int ahash_wait(int err, struct ahash_completion *res)
> +{
> +	switch (err) {
> +	case 0:
> +		break;
> +	case -EINPROGRESS:
> +	case -EBUSY:
> +		wait_for_completion(&res->completion);
> +		reinit_completion(&res->completion);
> +		err = res->err;
> +		/* fall through */
> +	default:
> +		pr_crit("ahash calculation failed: err: %d\n", err);
> +	}
> +
> +	return err;
> +}
> +
> +static int ima_calc_file_hash_atfm(struct file *file,
> +				   struct ima_digest_data *hash,
> +				   struct crypto_ahash *tfm)
> +{
> +	loff_t i_size, offset;
> +	char *rbuf;
> +	int rc, read = 0, rbuf_len;
> +	struct ahash_request *req;
> +	struct scatterlist sg[1];
> +	struct ahash_completion res;
> +
> +	hash->length = crypto_ahash_digestsize(tfm);
> +
> +	req = ahash_request_alloc(ima_ahash_tfm, GFP_KERNEL);

This should be tfm not ima_ahash_tfm.

> +	if (!req)
> +		return -ENOMEM;
> +
> +	init_completion(&res.completion);
> +	ahash_request_set_callback(req, CRYPTO_TFM_REQ_MAY_BACKLOG |
> +				   CRYPTO_TFM_REQ_MAY_SLEEP,
> +				   ahash_complete, &res);
> +
> +	rc = ahash_wait(crypto_ahash_init(req), &res);
> +	if (rc)
> +		goto out1;
> +
> +	i_size = i_size_read(file_inode(file));
> +
> +	if (i_size == 0)
> +		goto out2;
> +
> +	rbuf = kzalloc(PAGE_SIZE, GFP_KERNEL);
> +	if (!rbuf) {
> +		rc = -ENOMEM;
> +		goto out1;
> +	}
> +
> +	if (!(file->f_mode & FMODE_READ)) {
> +		file->f_mode |= FMODE_READ;
> +		read = 1;
> +	}
> +
> +	for (offset = 0; offset < i_size; offset += rbuf_len) {
> +		rbuf_len = ima_kernel_read(file, offset, rbuf, PAGE_SIZE);
> +		if (rbuf_len < 0) {
> +			rc = rbuf_len;
> +			break;
> +		}
> +		if (rbuf_len == 0)
> +			break;
> +
> +		sg_init_one(&sg[0], rbuf, rbuf_len);
> +		ahash_request_set_crypt(req, sg, NULL, rbuf_len);
> +
> +		rc = ahash_wait(crypto_ahash_update(req), &res);
> +		if (rc)
> +			break;
> +	}
> +	if (read)
> +		file->f_mode &= ~FMODE_READ;
> +	kfree(rbuf);
> +out2:
> +	if (!rc) {
> +		ahash_request_set_crypt(req, NULL, hash->digest, 0);
> +		rc = ahash_wait(crypto_ahash_final(req), &res);
> +	}
> +out1:
> +	ahash_request_free(req);
> +	return rc;
> +}
> +
> +static int ima_calc_file_ahash(struct file *file, struct ima_digest_data *hash)
> +{
> +	struct crypto_ahash *tfm;
> +	int rc;
> +
> +	tfm = ima_alloc_atfm(hash->algo);
> +	if (IS_ERR(tfm))
> +		return PTR_ERR(tfm);
> +
> +	rc = ima_calc_file_hash_atfm(file, hash, tfm);
> +
> +	ima_free_atfm(tfm);
> +
> +	return rc;
> +}
> +
>  static int ima_calc_file_hash_tfm(struct file *file,
>  				  struct ima_digest_data *hash,
>  				  struct crypto_shash *tfm)
> @@ -156,7 +316,7 @@ out:
>  	return rc;
>  }
> 
> -int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash)
> +static int ima_calc_file_shash(struct file *file, struct ima_digest_data *hash)
>  {
>  	struct crypto_shash *tfm;
>  	int rc;
> @@ -172,6 +332,20 @@ int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash)
>  	return rc;
>  }
> 
> +int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash)
> +{
> +	loff_t i_size = i_size_read(file_inode(file));
> +
> +	/* shash is more efficient small data
> +	 * ahash performance depends on data size and particular HW
> +	 * ima_ahash_size allows to specify the best value for the system
> +	 */
> +	if (ima_ahash_size && i_size >= ima_ahash_size)
> +		return ima_calc_file_ahash(file, hash);
> +	else
> +		return ima_calc_file_shash(file, hash);
> +}

If calculating the file hash using ahash fails, should it fall back to
using shash?

Mimi

>  /*
>   * Calculate the hash of template data
>   */

Re: [PATCH v1 1/3] ima: use ahash API for file hash calculation

[Dmitry Kasatkin]

On 26/06/14 14:54, Mimi Zohar wrote:
> On Thu, 2014-06-19 at 18:20 +0300, Dmitry Kasatkin wrote:
>> Async hash API allows to use HW acceleration for hash calculation.
>> It may give significant performance gain or/and reduce power consumption,
>> which might be very beneficial for battery powered devices.
>>
>> This patch introduces hash calculation using ahash API.
>>
>> ahash peformance depends on data size and particular HW. Under certain
>> limit, depending on the system, shash performance may be better.
>>
>> This patch also introduces 'ima_ahash_size' kernel parameter which can
>> be used to defines minimal data size to use with ahash. When this
>> parameter is not set or file size is smaller than defined by this
>> parameter, shash will be used. Thus, by defult, original shash
>> implementation is used.
>>
>> Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
>> ---
>>  Documentation/kernel-parameters.txt |   3 +
>>  security/integrity/ima/ima_crypto.c | 182 +++++++++++++++++++++++++++++++++++-
>>  2 files changed, 181 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
>> index a0c155c..f8efb01 100644
>> --- a/Documentation/kernel-parameters.txt
>> +++ b/Documentation/kernel-parameters.txt
>> @@ -1286,6 +1286,9 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
>>  	ihash_entries=	[KNL]
>>  			Set number of hash buckets for inode cache.
>>
>> +	ima_ahash_size=size [IMA]
>> +			Set the minimal file size when use ahash API.
>> +
>>  	ima_appraise=	[IMA] appraise integrity measurements
>>  			Format: { "off" | "enforce" | "fix" }
>>  			default: "enforce"
>> diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
>> index ccd0ac8..b7a8650 100644
>> --- a/security/integrity/ima/ima_crypto.c
>> +++ b/security/integrity/ima/ima_crypto.c
>> @@ -25,7 +25,25 @@
>>  #include <crypto/hash_info.h>
>>  #include "ima.h"
>>
>> +
>> +struct ahash_completion {
>> +	struct completion completion;
>> +	int err;
>> +};
>> +
>>  static struct crypto_shash *ima_shash_tfm;
>> +static struct crypto_ahash *ima_ahash_tfm;
>> +
>> +/* data size for ahash use */
>> +static loff_t ima_ahash_size;
>> +
>> +static int __init ima_ahash_setup(char *str)
>> +{
>> +	int rc = kstrtoll(str, 10, &ima_ahash_size);
> In general, variable definitions should be separated from code.  A
> simple initialization is fine.  Please separate variable definitions
> from code with a blank line. 
>
>> +	pr_info("ima_ahash_size = %lld\n", ima_ahash_size);
>> +	return !rc;
>> +}
>> +__setup("ima_ahash_size=", ima_ahash_setup);
> This boot parameter name doesn't reflect its purpose, defining the
> minimum file size for using ahash. The next patch defines an additional
> boot parameter ima_ahash_bufsize. Perhaps defining a single boot
> parameter (eg. ima_ahash=) with multiple fields would be better. 
>
>>  /**
>>   * ima_kernel_read - read file content
>> @@ -68,6 +86,14 @@ int ima_init_crypto(void)
>>  		       hash_algo_name[ima_hash_algo], rc);
>>  		return rc;
>>  	}
>> +	ima_ahash_tfm = crypto_alloc_ahash(hash_algo_name[ima_hash_algo], 0, 0);
>> +	if (IS_ERR(ima_ahash_tfm)) {
>> +		rc = PTR_ERR(ima_ahash_tfm);
>> +		crypto_free_shash(ima_shash_tfm);
> Only crypto_alloc_ahash() failed, not crypto_alloc_shash(). shash has
> worked fine up to now. Why require both shash and ahash to succeed? 


>> +		pr_err("Can not allocate %s (reason: %ld)\n",
>> +		       hash_algo_name[ima_hash_algo], rc);
>> +		return rc;
>> +	}
>>  	return 0;
>>  }
>>
>> @@ -93,9 +119,143 @@ static void ima_free_tfm(struct crypto_shash *tfm)
>>  		crypto_free_shash(tfm);
>>  }
>>
>> -/*
>> - * Calculate the MD5/SHA1 file digest
>> - */
>> +static struct crypto_ahash *ima_alloc_atfm(enum hash_algo algo)
>> +{
>> +	struct crypto_ahash *tfm = ima_ahash_tfm;
>> +	int rc;
>> +
>> +	if (algo != ima_hash_algo && algo < HASH_ALGO__LAST) {
>> +		tfm = crypto_alloc_ahash(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;
>> +}
>> +
>> +static void ima_free_atfm(struct crypto_ahash *tfm)
>> +{
>> +	if (tfm != ima_ahash_tfm)
>> +		crypto_free_ahash(tfm);
>> +}
>> +
>> +static void ahash_complete(struct crypto_async_request *req, int err)
>> +{
>> +	struct ahash_completion *res = req->data;
>> +
>> +	if (err == -EINPROGRESS)
>> +		return;
>> +	res->err = err;
>> +	complete(&res->completion);
>> +}
>> +
>> +static int ahash_wait(int err, struct ahash_completion *res)
>> +{
>> +	switch (err) {
>> +	case 0:
>> +		break;
>> +	case -EINPROGRESS:
>> +	case -EBUSY:
>> +		wait_for_completion(&res->completion);
>> +		reinit_completion(&res->completion);
>> +		err = res->err;
>> +		/* fall through */
>> +	default:
>> +		pr_crit("ahash calculation failed: err: %d\n", err);
>> +	}
>> +
>> +	return err;
>> +}
>> +
>> +static int ima_calc_file_hash_atfm(struct file *file,
>> +				   struct ima_digest_data *hash,
>> +				   struct crypto_ahash *tfm)
>> +{
>> +	loff_t i_size, offset;
>> +	char *rbuf;
>> +	int rc, read = 0, rbuf_len;
>> +	struct ahash_request *req;
>> +	struct scatterlist sg[1];
>> +	struct ahash_completion res;
>> +
>> +	hash->length = crypto_ahash_digestsize(tfm);
>> +
>> +	req = ahash_request_alloc(ima_ahash_tfm, GFP_KERNEL);
> This should be tfm not ima_ahash_tfm.
>
>> +	if (!req)
>> +		return -ENOMEM;
>> +
>> +	init_completion(&res.completion);
>> +	ahash_request_set_callback(req, CRYPTO_TFM_REQ_MAY_BACKLOG |
>> +				   CRYPTO_TFM_REQ_MAY_SLEEP,
>> +				   ahash_complete, &res);
>> +
>> +	rc = ahash_wait(crypto_ahash_init(req), &res);
>> +	if (rc)
>> +		goto out1;
>> +
>> +	i_size = i_size_read(file_inode(file));
>> +
>> +	if (i_size == 0)
>> +		goto out2;
>> +
>> +	rbuf = kzalloc(PAGE_SIZE, GFP_KERNEL);
>> +	if (!rbuf) {
>> +		rc = -ENOMEM;
>> +		goto out1;
>> +	}
>> +
>> +	if (!(file->f_mode & FMODE_READ)) {
>> +		file->f_mode |= FMODE_READ;
>> +		read = 1;
>> +	}
>> +
>> +	for (offset = 0; offset < i_size; offset += rbuf_len) {
>> +		rbuf_len = ima_kernel_read(file, offset, rbuf, PAGE_SIZE);
>> +		if (rbuf_len < 0) {
>> +			rc = rbuf_len;
>> +			break;
>> +		}
>> +		if (rbuf_len == 0)
>> +			break;
>> +
>> +		sg_init_one(&sg[0], rbuf, rbuf_len);
>> +		ahash_request_set_crypt(req, sg, NULL, rbuf_len);
>> +
>> +		rc = ahash_wait(crypto_ahash_update(req), &res);
>> +		if (rc)
>> +			break;
>> +	}
>> +	if (read)
>> +		file->f_mode &= ~FMODE_READ;
>> +	kfree(rbuf);
>> +out2:
>> +	if (!rc) {
>> +		ahash_request_set_crypt(req, NULL, hash->digest, 0);
>> +		rc = ahash_wait(crypto_ahash_final(req), &res);
>> +	}
>> +out1:
>> +	ahash_request_free(req);
>> +	return rc;
>> +}
>> +
>> +static int ima_calc_file_ahash(struct file *file, struct ima_digest_data *hash)
>> +{
>> +	struct crypto_ahash *tfm;
>> +	int rc;
>> +
>> +	tfm = ima_alloc_atfm(hash->algo);
>> +	if (IS_ERR(tfm))
>> +		return PTR_ERR(tfm);
>> +
>> +	rc = ima_calc_file_hash_atfm(file, hash, tfm);
>> +
>> +	ima_free_atfm(tfm);
>> +
>> +	return rc;
>> +}
>> +
>>  static int ima_calc_file_hash_tfm(struct file *file,
>>  				  struct ima_digest_data *hash,
>>  				  struct crypto_shash *tfm)
>> @@ -156,7 +316,7 @@ out:
>>  	return rc;
>>  }
>>
>> -int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash)
>> +static int ima_calc_file_shash(struct file *file, struct ima_digest_data *hash)
>>  {
>>  	struct crypto_shash *tfm;
>>  	int rc;
>> @@ -172,6 +332,20 @@ int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash)
>>  	return rc;
>>  }
>>
>> +int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash)
>> +{
>> +	loff_t i_size = i_size_read(file_inode(file));
>> +
>> +	/* shash is more efficient small data
>> +	 * ahash performance depends on data size and particular HW
>> +	 * ima_ahash_size allows to specify the best value for the system
>> +	 */
>> +	if (ima_ahash_size && i_size >= ima_ahash_size)
>> +		return ima_calc_file_ahash(file, hash);
>> +	else
>> +		return ima_calc_file_shash(file, hash);
>> +}
> If calculating the file hash using ahash fails, should it fall back to
> using shash?

If ahash fails, then it could be a HW error, which should not happen.
IF HW fails device is broken.

Do you really want to fallback to shash?

Thanks,

Dmitry

> Mimi
>
>>  /*
>>   * Calculate the hash of template data
>>   */
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH v1 1/3] ima: use ahash API for file hash calculation

[Mimi Zohar]

On Mon, 2014-06-30 at 17:58 +0300, Dmitry Kasatkin wrote: 
> On 26/06/14 14:54, Mimi Zohar wrote:
> > On Thu, 2014-06-19 at 18:20 +0300, Dmitry Kasatkin wrote:

> >> @@ -156,7 +316,7 @@ out:
> >>  	return rc;
> >>  }
> >>
> >> -int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash)
> >> +static int ima_calc_file_shash(struct file *file, struct ima_digest_data *hash)
> >>  {
> >>  	struct crypto_shash *tfm;
> >>  	int rc;
> >> @@ -172,6 +332,20 @@ int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash)
> >>  	return rc;
> >>  }
> >>
> >> +int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash)
> >> +{
> >> +	loff_t i_size = i_size_read(file_inode(file));
> >> +
> >> +	/* shash is more efficient small data
> >> +	 * ahash performance depends on data size and particular HW
> >> +	 * ima_ahash_size allows to specify the best value for the system
> >> +	 */
> >> +	if (ima_ahash_size && i_size >= ima_ahash_size)
> >> +		return ima_calc_file_ahash(file, hash);
> >> +	else
> >> +		return ima_calc_file_shash(file, hash);
> >> +}
> > If calculating the file hash using ahash fails, should it fall back to
> > using shash?
> 
> If ahash fails, then it could be a HW error, which should not happen.
> IF HW fails device is broken.

I would assume it depends on the HW, if the entire device/system is
broken.

> Do you really want to fallback to shash?

Yes, in this case, there is no downside to letting it to continue
working, just slower, using the software crypto implementation.  In any
case, it shouldn't be hard coded.

Mimi

[PATCH v1 2/3] ima: introduce multi-page collect buffers

[Dmitry Kasatkin]

Use of multiple-page collect buffers reduces:
1) the number of block IO requests
2) the number of asynchronous hash update requests

Second is important for HW accelerated hashing, because significant
amount of time is spent for preparation of hash update operation,
which includes configuring acceleration HW, DMA engine, etc...
Thus, HW accelerators are more efficient when working on large
chunks of data.

This patch introduces usage of multi-page collect buffers which
can be controlled with 'ima_ahash_bufsize' kernel parameter.
By default, one page of 4096 bytes is used as a collect buffer.

Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
---
 Documentation/kernel-parameters.txt |  3 ++
 security/integrity/ima/ima_crypto.c | 81 +++++++++++++++++++++++++++++++++++--
 2 files changed, 81 insertions(+), 3 deletions(-)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index f8efb01..608eba0 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -1289,6 +1289,9 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 	ima_ahash_size=size [IMA]
 			Set the minimal file size when use ahash API.
 
+	ima_ahash_bufsize=size [IMA]
+			Set hashing buffer size for ahash API.
+
 	ima_appraise=	[IMA] appraise integrity measurements
 			Format: { "off" | "enforce" | "fix" }
 			default: "enforce"
diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
index b7a8650..9e9414e 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -25,7 +25,6 @@
 #include <crypto/hash_info.h>
 #include "ima.h"
 
-
 struct ahash_completion {
 	struct completion completion;
 	int err;
@@ -45,6 +44,20 @@ static int __init ima_ahash_setup(char *str)
 }
 __setup("ima_ahash_size=", ima_ahash_setup);
 
+/* default is 0 - 1 page. */
+static int ima_max_order;
+
+static int __init ima_bufsize_setup(char *str)
+{
+	long max_size, rc = kstrtol(str, 10, &max_size);
+	if (!rc)
+		ima_max_order = get_order(max_size);
+	pr_info("ima_ahash_bufsize: %ld, order = %d\n",
+		max_size, ima_max_order);
+	return !rc;
+}
+__setup("ima_ahash_bufsize=", ima_bufsize_setup);
+
 /**
  * ima_kernel_read - read file content
  *
@@ -169,6 +182,63 @@ static int ahash_wait(int err, struct ahash_completion *res)
 	return err;
 }
 
+/**
+ * ima_alloc_pages() - Allocated contiguous pages.
+ * @max_size:       Maximum amount of memory to allocate.
+ * @allocated_size: Returned size of actual allocation.
+ * @last_warn:      Should the min_size allocation warn or not.
+ *
+ * Tries to do opportunistic allocation for memory first trying to allocate
+ * max_size amount of memory and then splitting that until zero order is
+ * reached. Allocation is tried without generating allocation warnings unless
+ * last_warn is set. Last_warn set affects only last allocation of zero order.
+ *
+ * Return pointer to allocated memory, or NULL on failure.
+ */
+static void *ima_alloc_pages(loff_t max_size, size_t *allocated_size,
+			     int last_warn)
+{
+	void *ptr;
+	gfp_t gfp_mask = __GFP_NOWARN | __GFP_WAIT | __GFP_NORETRY;
+	unsigned int order = min(get_order(max_size), ima_max_order);
+
+	for (; order; order--) {
+		ptr = (void *)__get_free_pages(gfp_mask, order);
+		if (ptr) {
+			*allocated_size = PAGE_SIZE << order;
+			return ptr;
+		}
+	}
+
+	/* order is zero - one page */
+
+	gfp_mask = GFP_KERNEL;
+
+	if (!last_warn)
+		gfp_mask |= __GFP_NOWARN;
+
+	ptr = (void *)__get_free_pages(gfp_mask, 0);
+	if (ptr) {
+		*allocated_size = PAGE_SIZE;
+		return ptr;
+	}
+
+	*allocated_size = 0;
+	return NULL;
+}
+
+/**
+ * ima_free_pages() - Free pages allocated by ima_alloc_pages().
+ * @ptr:  Pointer to allocated pages.
+ * @size: Size of allocated buffer.
+ */
+static void ima_free_pages(void *ptr, size_t size)
+{
+	if (!ptr)
+		return;
+	free_pages((unsigned long)ptr, get_order(size));
+}
+
 static int ima_calc_file_hash_atfm(struct file *file,
 				   struct ima_digest_data *hash,
 				   struct crypto_ahash *tfm)
@@ -179,6 +249,7 @@ static int ima_calc_file_hash_atfm(struct file *file,
 	struct ahash_request *req;
 	struct scatterlist sg[1];
 	struct ahash_completion res;
+	size_t rbuf_size;
 
 	hash->length = crypto_ahash_digestsize(tfm);
 
@@ -200,7 +271,11 @@ static int ima_calc_file_hash_atfm(struct file *file,
 	if (i_size == 0)
 		goto out2;
 
-	rbuf = kzalloc(PAGE_SIZE, GFP_KERNEL);
+	/*
+	 * Try to allocate maximum size of memory, fail if not even single
+	 * page cannot be allocated.
+	 */
+	rbuf = ima_alloc_pages(i_size, &rbuf_size, 1);
 	if (!rbuf) {
 		rc = -ENOMEM;
 		goto out1;
@@ -229,7 +304,7 @@ static int ima_calc_file_hash_atfm(struct file *file,
 	}
 	if (read)
 		file->f_mode &= ~FMODE_READ;
-	kfree(rbuf);
+	ima_free_pages(rbuf, rbuf_size);
 out2:
 	if (!rc) {
 		ahash_request_set_crypt(req, NULL, hash->digest, 0);
-- 
1.9.1

[PATCH v1 3/3] ima: provide double buffering for hash calculation

[Dmitry Kasatkin]

Asynchronous hash API allows initiate hash calculation and perform
other tasks while hash is calculated.

This patch introduces usage of double buffering for simultenous
hashing and reading of the next chunk of data from the storage.

Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
---
 security/integrity/ima/ima_crypto.c | 59 +++++++++++++++++++++++++++----------
 1 file changed, 43 insertions(+), 16 deletions(-)

diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
index 9e9414e..e6bef4b 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -244,12 +244,12 @@ static int ima_calc_file_hash_atfm(struct file *file,
 				   struct crypto_ahash *tfm)
 {
 	loff_t i_size, offset;
-	char *rbuf;
-	int rc, read = 0, rbuf_len;
+	char *rbuf[2] = { NULL, };
+	int rc, read = 0, rbuf_len, active = 0, ahash_rc = 0;
 	struct ahash_request *req;
 	struct scatterlist sg[1];
 	struct ahash_completion res;
-	size_t rbuf_size;
+	size_t rbuf_size[2];
 
 	hash->length = crypto_ahash_digestsize(tfm);
 
@@ -275,36 +275,63 @@ static int ima_calc_file_hash_atfm(struct file *file,
 	 * Try to allocate maximum size of memory, fail if not even single
 	 * page cannot be allocated.
 	 */
-	rbuf = ima_alloc_pages(i_size, &rbuf_size, 1);
-	if (!rbuf) {
+	rbuf[0] = ima_alloc_pages(i_size, &rbuf_size[0], 1);
+	if (!rbuf[0]) {
 		rc = -ENOMEM;
 		goto out1;
 	}
 
+	/* Only allocate one buffer if that is enough. */
+	if (i_size > rbuf_size[0]) {
+		/*
+		 * Try to allocate secondary buffer if that fails fallback to
+		 * using single buffering. Use previous memory allocation size
+		 * as baseline for possible allocation size.
+		 */
+		rbuf[1] = ima_alloc_pages(i_size - rbuf_size[0],
+					  &rbuf_size[1], 0);
+	}
+
 	if (!(file->f_mode & FMODE_READ)) {
 		file->f_mode |= FMODE_READ;
 		read = 1;
 	}
 
 	for (offset = 0; offset < i_size; offset += rbuf_len) {
-		rbuf_len = ima_kernel_read(file, offset, rbuf, PAGE_SIZE);
-		if (rbuf_len < 0) {
-			rc = rbuf_len;
-			break;
+		if (offset && !rbuf[1]) {
+			/* wait for completion of previous request */
+			rc = ahash_wait(ahash_rc, &res);
+			if (rc)
+				goto out3;
+		}
+		/* read buffer */
+		rbuf_len = min_t(loff_t, i_size - offset, rbuf_size[active]);
+		rc = ima_kernel_read(file, offset, rbuf[active], rbuf_len);
+		if (rc != rbuf_len)
+			goto out3;
+
+		if (offset && rbuf[1]) {
+			/* wait for completion of previous request */
+			rc = ahash_wait(ahash_rc, &res);
+			if (rc)
+				goto out3;
 		}
-		if (rbuf_len == 0)
-			break;
 
-		sg_init_one(&sg[0], rbuf, rbuf_len);
+		sg_init_one(&sg[0], rbuf[active], rbuf_len);
 		ahash_request_set_crypt(req, sg, NULL, rbuf_len);
 
-		rc = ahash_wait(crypto_ahash_update(req), &res);
-		if (rc)
-			break;
+		ahash_rc = crypto_ahash_update(req);
+
+		if (rbuf[1])
+			active = !active; /* swap buffers. */
 	}
+	/* wait for the last request to complete */
+	rc = ahash_wait(ahash_rc, &res);
+out3:
 	if (read)
 		file->f_mode &= ~FMODE_READ;
-	ima_free_pages(rbuf, rbuf_size);
+	ima_free_pages(rbuf[0], rbuf_size[0]);
+	ima_free_pages(rbuf[1], rbuf_size[1]);
 out2:
 	if (!rc) {
 		ahash_request_set_crypt(req, NULL, hash->digest, 0);
-- 
1.9.1

Re: [PATCH v1 3/3] ima: provide double buffering for hash calculation

[Mimi Zohar]

On Thu, 2014-06-19 at 18:20 +0300, Dmitry Kasatkin wrote:
> Asynchronous hash API allows initiate hash calculation and perform
> other tasks while hash is calculated.
> 
> This patch introduces usage of double buffering for simultenous
							^simultaneous
> hashing and reading of the next chunk of data from the storage.
> 
> Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
> ---
>  security/integrity/ima/ima_crypto.c | 59 +++++++++++++++++++++++++++----------
>  1 file changed, 43 insertions(+), 16 deletions(-)
> 
> diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
> index 9e9414e..e6bef4b 100644
> --- a/security/integrity/ima/ima_crypto.c
> +++ b/security/integrity/ima/ima_crypto.c
> @@ -244,12 +244,12 @@ static int ima_calc_file_hash_atfm(struct file *file,
>  				   struct crypto_ahash *tfm)
>  {
>  	loff_t i_size, offset;
> -	char *rbuf;
> -	int rc, read = 0, rbuf_len;
> +	char *rbuf[2] = { NULL, };
> +	int rc, read = 0, rbuf_len, active = 0, ahash_rc = 0;
>  	struct ahash_request *req;
>  	struct scatterlist sg[1];
>  	struct ahash_completion res;
> -	size_t rbuf_size;
> +	size_t rbuf_size[2];
> 
>  	hash->length = crypto_ahash_digestsize(tfm);
> 
> @@ -275,36 +275,63 @@ static int ima_calc_file_hash_atfm(struct file *file,
>  	 * Try to allocate maximum size of memory, fail if not even single
>  	 * page cannot be allocated.
>  	 */
> -	rbuf = ima_alloc_pages(i_size, &rbuf_size, 1);
> -	if (!rbuf) {
> +	rbuf[0] = ima_alloc_pages(i_size, &rbuf_size[0], 1);
> +	if (!rbuf[0]) {
>  		rc = -ENOMEM;
>  		goto out1;
>  	}
> 
> +	/* Only allocate one buffer if that is enough. */
> +	if (i_size > rbuf_size[0]) {
> +		/*
> +		 * Try to allocate secondary buffer if that fails fallback to
> +		 * using single buffering. Use previous memory allocation size
> +		 * as baseline for possible allocation size.
> +		 */
> +		rbuf[1] = ima_alloc_pages(i_size - rbuf_size[0],
> +					  &rbuf_size[1], 0);
> +	}
> +
>  	if (!(file->f_mode & FMODE_READ)) {
>  		file->f_mode |= FMODE_READ;
>  		read = 1;
>  	}
> 
>  	for (offset = 0; offset < i_size; offset += rbuf_len) {
> -		rbuf_len = ima_kernel_read(file, offset, rbuf, PAGE_SIZE);
> -		if (rbuf_len < 0) {
> -			rc = rbuf_len;
> -			break;
> +		if (offset && !rbuf[1]) {
> +			/* wait for completion of previous request */

This comment and the same ones below don't help clarify the code.  Here,
only one buffer is available, so we're waiting for the hash calculation
to complete before reading more data.

> +			rc = ahash_wait(ahash_rc, &res);
> +			if (rc)
> +				goto out3;
> +		}
> +		/* read buffer */
> +		rbuf_len = min_t(loff_t, i_size - offset, rbuf_size[active]);
> +		rc = ima_kernel_read(file, offset, rbuf[active], rbuf_len);
> +		if (rc != rbuf_len)
> +			goto out3;
> +
> +		if (offset && rbuf[1]) {
> +			/* wait for completion of previous request */

Where as here, we finished reading more data and are waiting for the
hash calculation for the prior buffer to complete. 

Mimi

> +			rc = ahash_wait(ahash_rc, &res);
> +			if (rc)
> +				goto out3;
>  		}
> -		if (rbuf_len == 0)
> -			break;
> 
> -		sg_init_one(&sg[0], rbuf, rbuf_len);
> +		sg_init_one(&sg[0], rbuf[active], rbuf_len);
>  		ahash_request_set_crypt(req, sg, NULL, rbuf_len);
> 
> -		rc = ahash_wait(crypto_ahash_update(req), &res);
> -		if (rc)
> -			break;
> +		ahash_rc = crypto_ahash_update(req);
> +
> +		if (rbuf[1])
> +			active = !active; /* swap buffers. */
>  	}
> +	/* wait for the last request to complete */
> +	rc = ahash_wait(ahash_rc, &res);
> +out3:
>  	if (read)
>  		file->f_mode &= ~FMODE_READ;
> -	ima_free_pages(rbuf, rbuf_size);
> +	ima_free_pages(rbuf[0], rbuf_size[0]);
> +	ima_free_pages(rbuf[1], rbuf_size[1]);
>  out2:
>  	if (!rc) {
>  		ahash_request_set_crypt(req, NULL, hash->digest, 0);