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

[PATCH v3 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 (battery)
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 asynchronous 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 module parameter
'ima.ahash_minsize=<min_file_size>', which allows to specify optimal file size
when start using ahash. By default ahash is disabled until non-zero value
is specified.

Second patch introduces multi-page buffers which makes HW acceleration more
efficient. It provides 'ima.ahash_bufsize=<bufsize>' module parameter to
specify buffer size. Buffer is 4k if parameter is unspecified.

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

Changes in v3:
- kernel parameters replaced with module parameters
- more clear comments and function descriptions
- pr_crit replaced with pr_crit_ratelimited

Changes in v2:
- ima_ahash_size and ima_ahash_bufsize were combined as ima_ahash
- ahash pre-allocation moved out from __init code to be able to use
  ahash crypto modules. Ahash allocated once on the first use.
- hash calculation falls back to sahsh if ahash allocation/calculation fails
- complex initialization separated from variable declaration
- improved comments

- 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 |  17 ++
 security/integrity/ima/ima_crypto.c | 312 +++++++++++++++++++++++++++++++++++-
 2 files changed, 326 insertions(+), 3 deletions(-)

-- 
1.9.1

[PATCH v3 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 performance depends on data size and particular HW. Under certain
limit, depending on the system, shash performance may be better.

This patch defines 'ahash_minsize' module parameter which can be used to
define minimal file size to use with ahash. When file size is not set
or smaller than defined by the parameter, shash will be used.

Changes in v3:
- kernel parameter replaced with module parameter
- pr_crit replaced with pr_crit_ratelimited

Changes in v2:
- ima_ahash_size became as ima_ahash
- ahash pre-allocation moved out from __init code to be able to use
  ahash crypto modules. Ahash allocated once on the first use.
- hash calculation falls back to shash if ahash allocation/calculation fails
- complex initialization separated from variable declaration
- improved comments

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

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 5a214a3..03c8452 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -1318,6 +1318,15 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 			Formats: { "ima" | "ima-ng" }
 			Default: "ima-ng"
 
+	ima.ahash_minsize= [IMA] Minimum file size for asynchronous hash usage
+			Format: <min_file_size>
+			Set the minimal file size for using asynchronous hash.
+			If left unspecified, ahash usage is disabled.
+
+			ahash performance varies for different data sizes on
+			different crypto accelerators. This option can be used
+			to achieve best performance for particular HW.
+
 	init=		[KNL]
 			Format: <full_path>
 			Run specified binary instead of /sbin/init as init
diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
index f82d1d7..bc38160 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -16,6 +16,8 @@
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
 #include <linux/kernel.h>
+#include <linux/moduleparam.h>
+#include <linux/ratelimit.h>
 #include <linux/file.h>
 #include <linux/crypto.h>
 #include <linux/scatterlist.h>
@@ -25,7 +27,18 @@
 #include <crypto/hash_info.h>
 #include "ima.h"
 
+struct ahash_completion {
+	struct completion completion;
+	int err;
+};
+
+/* minimum file size for ahash use */
+static unsigned long ima_ahash_minsize;
+module_param_named(ahash_minsize, ima_ahash_minsize, ulong, 0644);
+MODULE_PARM_DESC(ahash_minsize, "Minimum file size for ahash use");
+
 static struct crypto_shash *ima_shash_tfm;
+static struct crypto_ahash *ima_ahash_tfm;
 
 /**
  * ima_kernel_read - read file content
@@ -93,9 +106,146 @@ 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) {
+		tfm = crypto_alloc_ahash(hash_algo_name[algo], 0, 0);
+		if (!IS_ERR(tfm)) {
+			if (algo == ima_hash_algo)
+				ima_ahash_tfm = tfm;
+		} else {
+			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_ratelimited("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(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 +306,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 +322,33 @@ int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash)
 	return rc;
 }
 
+/**
+ * ima_calc_file_hash - calculae file hash
+ *
+ * if ima_ahash_minsize parameter is non-zero, this function uses
+ * ahash for hash caclulation. ahash performance varies for different
+ * data sizes on different crypto accelerators. shash performance might
+ * be better for small file. 'ima.ahash_minsize' module parameter allows
+ * to specify the best value for the system.
+ *
+ * If ahash fails, it fallbacks to shash.
+ */
+int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash)
+{
+	loff_t i_size;
+	int rc;
+
+	i_size = i_size_read(file_inode(file));
+
+	if (ima_ahash_minsize && i_size >= ima_ahash_minsize) {
+		rc = ima_calc_file_ahash(file, hash);
+		if (!rc)
+			return 0;
+	}
+
+	return ima_calc_file_shash(file, hash);
+}
+
 /*
  * Calculate the hash of template data
  */
-- 
1.9.1

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

[Mimi Zohar]

On Fri, 2014-07-04 at 15:05 +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 performance depends on data size and particular HW. Under certain
> limit, depending on the system, shash performance may be better.
> 
> This patch defines 'ahash_minsize' module parameter which can be used to
> define minimal file size to use with ahash. When file size is not set
> or smaller than defined by the parameter, shash will be used.
> 
> Changes in v3:
> - kernel parameter replaced with module parameter
> - pr_crit replaced with pr_crit_ratelimited
> 
> Changes in v2:
> - ima_ahash_size became as ima_ahash
> - ahash pre-allocation moved out from __init code to be able to use
>   ahash crypto modules. Ahash allocated once on the first use.
> - hash calculation falls back to shash if ahash allocation/calculation fails
> - complex initialization separated from variable declaration
> - improved comments
> 
> Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
> ---
>  Documentation/kernel-parameters.txt |   9 ++
>  security/integrity/ima/ima_crypto.c | 185 +++++++++++++++++++++++++++++++++++-
>  2 files changed, 190 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> index 5a214a3..03c8452 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -1318,6 +1318,15 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
>  			Formats: { "ima" | "ima-ng" }
>  			Default: "ima-ng"
> 
> +	ima.ahash_minsize= [IMA] Minimum file size for asynchronous hash usage
> +			Format: <min_file_size>
> +			Set the minimal file size for using asynchronous hash.
> +			If left unspecified, ahash usage is disabled.
> +
> +			ahash performance varies for different data sizes on
> +			different crypto accelerators. This option can be used
> +			to achieve best performance for particular HW.
> +
>  	init=		[KNL]
>  			Format: <full_path>
>  			Run specified binary instead of /sbin/init as init
> diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
> index f82d1d7..bc38160 100644
> --- a/security/integrity/ima/ima_crypto.c
> +++ b/security/integrity/ima/ima_crypto.c
> @@ -16,6 +16,8 @@
>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> 
>  #include <linux/kernel.h>
> +#include <linux/moduleparam.h>
> +#include <linux/ratelimit.h>
>  #include <linux/file.h>
>  #include <linux/crypto.h>
>  #include <linux/scatterlist.h>
> @@ -25,7 +27,18 @@
>  #include <crypto/hash_info.h>
>  #include "ima.h"
> 
> +struct ahash_completion {
> +	struct completion completion;
> +	int err;
> +};
> +
> +/* minimum file size for ahash use */
> +static unsigned long ima_ahash_minsize;
> +module_param_named(ahash_minsize, ima_ahash_minsize, ulong, 0644);
> +MODULE_PARM_DESC(ahash_minsize, "Minimum file size for ahash use");
> +
>  static struct crypto_shash *ima_shash_tfm;
> +static struct crypto_ahash *ima_ahash_tfm;
> 
>  /**
>   * ima_kernel_read - read file content
> @@ -93,9 +106,146 @@ 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) {
> +		tfm = crypto_alloc_ahash(hash_algo_name[algo], 0, 0);
> +		if (!IS_ERR(tfm)) {
> +			if (algo == ima_hash_algo)
> +				ima_ahash_tfm = tfm;
> +		} else {
> +			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_ratelimited("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(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 +306,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 +322,33 @@ int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash)
>  	return rc;
>  }
> 
> +/**

This is the kernel-doc delimiter.

> + * ima_calc_file_hash - calculae file hash
> + *

Missing kernel-doc argument descriptions.  Refer to
Documentation/kernel-doc-nano-HOWTO.txt.

> + * if ima_ahash_minsize parameter is non-zero, this function uses
> + * ahash for hash caclulation. ahash performance varies for different
> + * data sizes on different crypto accelerators. shash performance might
> + * be better for small file. 'ima.ahash_minsize' module parameter allows
> + * to specify the best value for the system.
> + *
> + * If ahash fails, it fallbacks to shash.
> + */
> +int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash)
> +{
> +	loff_t i_size;
> +	int rc;
> +
> +	i_size = i_size_read(file_inode(file));
> +
> +	if (ima_ahash_minsize && i_size >= ima_ahash_minsize) {
> +		rc = ima_calc_file_ahash(file, hash);
> +		if (!rc)
> +			return 0;
> +	}
> +
> +	return ima_calc_file_shash(file, hash);
> +}

If the crypto accelerator fails, it falls back to using shash.  Is their
any indication that the HW error is intermittent or persistent?  Should
ima_ahash_minsize be reset?

If the crypto accelerator, built as a kernel module, is removed,
ima_ahash_minsize would still be set.  It would continue to use ahash.
Is this the correct behavior? Or should ima_ahash_minsize be reset?

thanks,

Mimi

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

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

[Dmitry Kasatkin]

On 07/07/14 14:56, Mimi Zohar wrote:
> On Fri, 2014-07-04 at 15:05 +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 performance depends on data size and particular HW. Under certain
>> limit, depending on the system, shash performance may be better.
>>
>> This patch defines 'ahash_minsize' module parameter which can be used to
>> define minimal file size to use with ahash. When file size is not set
>> or smaller than defined by the parameter, shash will be used.
>>
>> Changes in v3:
>> - kernel parameter replaced with module parameter
>> - pr_crit replaced with pr_crit_ratelimited
>>
>> Changes in v2:
>> - ima_ahash_size became as ima_ahash
>> - ahash pre-allocation moved out from __init code to be able to use
>>   ahash crypto modules. Ahash allocated once on the first use.
>> - hash calculation falls back to shash if ahash allocation/calculation fails
>> - complex initialization separated from variable declaration
>> - improved comments
>>
>> Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
>> ---
>>  Documentation/kernel-parameters.txt |   9 ++
>>  security/integrity/ima/ima_crypto.c | 185 +++++++++++++++++++++++++++++++++++-
>>  2 files changed, 190 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
>> index 5a214a3..03c8452 100644
>> --- a/Documentation/kernel-parameters.txt
>> +++ b/Documentation/kernel-parameters.txt
>> @@ -1318,6 +1318,15 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
>>  			Formats: { "ima" | "ima-ng" }
>>  			Default: "ima-ng"
>>
>> +	ima.ahash_minsize= [IMA] Minimum file size for asynchronous hash usage
>> +			Format: <min_file_size>
>> +			Set the minimal file size for using asynchronous hash.
>> +			If left unspecified, ahash usage is disabled.
>> +
>> +			ahash performance varies for different data sizes on
>> +			different crypto accelerators. This option can be used
>> +			to achieve best performance for particular HW.
>> +
>>  	init=		[KNL]
>>  			Format: <full_path>
>>  			Run specified binary instead of /sbin/init as init
>> diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
>> index f82d1d7..bc38160 100644
>> --- a/security/integrity/ima/ima_crypto.c
>> +++ b/security/integrity/ima/ima_crypto.c
>> @@ -16,6 +16,8 @@
>>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>>
>>  #include <linux/kernel.h>
>> +#include <linux/moduleparam.h>
>> +#include <linux/ratelimit.h>
>>  #include <linux/file.h>
>>  #include <linux/crypto.h>
>>  #include <linux/scatterlist.h>
>> @@ -25,7 +27,18 @@
>>  #include <crypto/hash_info.h>
>>  #include "ima.h"
>>
>> +struct ahash_completion {
>> +	struct completion completion;
>> +	int err;
>> +};
>> +
>> +/* minimum file size for ahash use */
>> +static unsigned long ima_ahash_minsize;
>> +module_param_named(ahash_minsize, ima_ahash_minsize, ulong, 0644);
>> +MODULE_PARM_DESC(ahash_minsize, "Minimum file size for ahash use");
>> +
>>  static struct crypto_shash *ima_shash_tfm;
>> +static struct crypto_ahash *ima_ahash_tfm;
>>
>>  /**
>>   * ima_kernel_read - read file content
>> @@ -93,9 +106,146 @@ 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) {
>> +		tfm = crypto_alloc_ahash(hash_algo_name[algo], 0, 0);
>> +		if (!IS_ERR(tfm)) {
>> +			if (algo == ima_hash_algo)
>> +				ima_ahash_tfm = tfm;
>> +		} else {
>> +			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_ratelimited("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(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 +306,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 +322,33 @@ int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash)
>>  	return rc;
>>  }
>>
>> +/**
> This is the kernel-doc delimiter.
>
>> + * ima_calc_file_hash - calculae file hash
>> + *
> Missing kernel-doc argument descriptions.  Refer to
> Documentation/kernel-doc-nano-HOWTO.txt.
>

There is no need to explain arguments as they self-evident.

>> + * if ima_ahash_minsize parameter is non-zero, this function uses
>> + * ahash for hash caclulation. ahash performance varies for different
>> + * data sizes on different crypto accelerators. shash performance might
>> + * be better for small file. 'ima.ahash_minsize' module parameter allows
>> + * to specify the best value for the system.
>> + *
>> + * If ahash fails, it fallbacks to shash.
>> + */
>> +int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash)
>> +{
>> +	loff_t i_size;
>> +	int rc;
>> +
>> +	i_size = i_size_read(file_inode(file));
>> +
>> +	if (ima_ahash_minsize && i_size >= ima_ahash_minsize) {
>> +		rc = ima_calc_file_ahash(file, hash);
>> +		if (!rc)
>> +			return 0;
>> +	}
>> +
>> +	return ima_calc_file_shash(file, hash);
>> +}
> If the crypto accelerator fails, it falls back to using shash.  Is their
> any indication that the HW error is intermittent or persistent?  Should
> ima_ahash_minsize be reset?

If hw constantly does not work then it is simply broken.

You want to be protected from "random" failures.

For me it is not the case either... If it works then it works...

> If the crypto accelerator, built as a kernel module, is removed,
> ima_ahash_minsize would still be set.  It would continue to use ahash.
> Is this the correct behavior? Or should ima_ahash_minsize be reset?
>
> thanks,
>
> Mimi

It cannot be removed, because it is used and module usage counter
protects from removing...


What is your worry?

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

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

[Mimi Zohar]

On Mon, 2014-07-07 at 16:37 +0300, Dmitry Kasatkin wrote: 
> On 07/07/14 14:56, Mimi Zohar wrote:
> > On Fri, 2014-07-04 at 15:05 +0300, Dmitry Kasatkin wrote: 

> >>
> >> +/**
> > This is the kernel-doc delimiter.
> >
> >> + * ima_calc_file_hash - calculae file hash
> >> + *
> > Missing kernel-doc argument descriptions.  Refer to
> > Documentation/kernel-doc-nano-HOWTO.txt.

Not defining the arguments results in a kernel-doc warning.  Providing
kernel-doc is nice, but is unnecessary in this case, as it isn't an
exported loadable module, nor an externally visible function to other
kernel files.  Either remove the extra asterisk, making it a regular
comment, or add the arguments.

> 
> There is no need to explain arguments as they self-evident.
> 
> >> + * if ima_ahash_minsize parameter is non-zero, this function uses
> >> + * ahash for hash caclulation. ahash performance varies for different
> >> + * data sizes on different crypto accelerators. shash performance might
> >> + * be better for small file. 'ima.ahash_minsize' module parameter allows
> >> + * to specify the best value for the system.
> >> + *
> >> + * If ahash fails, it fallbacks to shash.
> >> + */
> >> +int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash)
> >> +{
> >> +	loff_t i_size;
> >> +	int rc;
> >> +
> >> +	i_size = i_size_read(file_inode(file));
> >> +
> >> +	if (ima_ahash_minsize && i_size >= ima_ahash_minsize) {
> >> +		rc = ima_calc_file_ahash(file, hash);
> >> +		if (!rc)
> >> +			return 0;
> >> +	}
> >> +
> >> +	return ima_calc_file_shash(file, hash);
> >> +}
> > If the crypto accelerator fails, it falls back to using shash.  Is their
> > any indication that the HW error is intermittent or persistent?  Should
> > ima_ahash_minsize be reset?
> 
> If hw constantly does not work then it is simply broken.

True

> You want to be protected from "random" failures.

> For me it is not the case either... If it works then it works...

This discussion isn't about your particular HW environment, but a
general question.  For example, suppose we were discussing a laptop with
a HW crypto accelerator.  If the HW crypto broke, I would at least want
to be able to quiesce the system properly.  I'd most likely want to be
able to continue using my laptop with software crypto.

> > If the crypto accelerator, built as a kernel module, is removed,
> > ima_ahash_minsize would still be set.  It would continue to use ahash.
> > Is this the correct behavior? Or should ima_ahash_minsize be reset?
> >
> 
> It cannot be removed, because it is used and module usage counter
> protects from removing...

Ok

Mimi

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

[Dmitry Kasatkin]

On 07/07/14 18:44, Mimi Zohar wrote:
> On Mon, 2014-07-07 at 16:37 +0300, Dmitry Kasatkin wrote: 
>> On 07/07/14 14:56, Mimi Zohar wrote:
>>> On Fri, 2014-07-04 at 15:05 +0300, Dmitry Kasatkin wrote: 
>>>> +/**
>>> This is the kernel-doc delimiter.
>>>
>>>> + * ima_calc_file_hash - calculae file hash
>>>> + *
>>> Missing kernel-doc argument descriptions.  Refer to
>>> Documentation/kernel-doc-nano-HOWTO.txt.
> Not defining the arguments results in a kernel-doc warning.  Providing
> kernel-doc is nice, but is unnecessary in this case, as it isn't an
> exported loadable module, nor an externally visible function to other
> kernel files.  Either remove the extra asterisk, making it a regular
> comment, or add the arguments.
>
>> There is no need to explain arguments as they self-evident.
>>
>>>> + * if ima_ahash_minsize parameter is non-zero, this function uses
>>>> + * ahash for hash caclulation. ahash performance varies for different
>>>> + * data sizes on different crypto accelerators. shash performance might
>>>> + * be better for small file. 'ima.ahash_minsize' module parameter allows
>>>> + * to specify the best value for the system.
>>>> + *
>>>> + * If ahash fails, it fallbacks to shash.
>>>> + */
>>>> +int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash)
>>>> +{
>>>> +	loff_t i_size;
>>>> +	int rc;
>>>> +
>>>> +	i_size = i_size_read(file_inode(file));
>>>> +
>>>> +	if (ima_ahash_minsize && i_size >= ima_ahash_minsize) {
>>>> +		rc = ima_calc_file_ahash(file, hash);
>>>> +		if (!rc)
>>>> +			return 0;
>>>> +	}
>>>> +
>>>> +	return ima_calc_file_shash(file, hash);
>>>> +}
>>> If the crypto accelerator fails, it falls back to using shash.  Is their
>>> any indication that the HW error is intermittent or persistent?  Should
>>> ima_ahash_minsize be reset?
>> If hw constantly does not work then it is simply broken.
> True
>
>> You want to be protected from "random" failures.
>> For me it is not the case either... If it works then it works...
> This discussion isn't about your particular HW environment, but a
> general question.  For example, suppose we were discussing a laptop with
> a HW crypto accelerator.  If the HW crypto broke, I would at least want
> to be able to quiesce the system properly.  I'd most likely want to be
> able to continue using my laptop with software crypto.

Driver probing code will detect that HW is not responding and driver
will not be enabled...

IMA will not be able to use it...

It is the same story as with any other HW and driver in the system.

>>> If the crypto accelerator, built as a kernel module, is removed,
>>> ima_ahash_minsize would still be set.  It would continue to use ahash.
>>> Is this the correct behavior? Or should ima_ahash_minsize be reset?
>>>
>> It cannot be removed, because it is used and module usage counter
>> protects from removing...
> Ok
>
> Mimi
>
>

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

[Mimi Zohar]

On Mon, 2014-07-07 at 19:11 +0300, Dmitry Kasatkin wrote: 
> On 07/07/14 18:44, Mimi Zohar wrote:
> > On Mon, 2014-07-07 at 16:37 +0300, Dmitry Kasatkin wrote: 
> >> On 07/07/14 14:56, Mimi Zohar wrote:
> >>> On Fri, 2014-07-04 at 15:05 +0300, Dmitry Kasatkin wrote: 
> >>>> +/**
> >>> This is the kernel-doc delimiter.
> >>>
> >>>> + * ima_calc_file_hash - calculae file hash
> >>>> + *
> >>> Missing kernel-doc argument descriptions.  Refer to
> >>> Documentation/kernel-doc-nano-HOWTO.txt.
> > Not defining the arguments results in a kernel-doc warning.  Providing
> > kernel-doc is nice, but is unnecessary in this case, as it isn't an
> > exported loadable module, nor an externally visible function to other
> > kernel files.  Either remove the extra asterisk, making it a regular
> > comment, or add the arguments.
> >
> >> There is no need to explain arguments as they self-evident.
> >>
> >>>> + * if ima_ahash_minsize parameter is non-zero, this function uses
> >>>> + * ahash for hash caclulation. ahash performance varies for different
> >>>> + * data sizes on different crypto accelerators. shash performance might
> >>>> + * be better for small file. 'ima.ahash_minsize' module parameter allows
> >>>> + * to specify the best value for the system.
> >>>> + *
> >>>> + * If ahash fails, it fallbacks to shash.
> >>>> + */
> >>>> +int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash)
> >>>> +{
> >>>> +	loff_t i_size;
> >>>> +	int rc;
> >>>> +
> >>>> +	i_size = i_size_read(file_inode(file));
> >>>> +
> >>>> +	if (ima_ahash_minsize && i_size >= ima_ahash_minsize) {
> >>>> +		rc = ima_calc_file_ahash(file, hash);
> >>>> +		if (!rc)
> >>>> +			return 0;
> >>>> +	}
> >>>> +
> >>>> +	return ima_calc_file_shash(file, hash);
> >>>> +}
> >>> If the crypto accelerator fails, it falls back to using shash.  Is their
> >>> any indication that the HW error is intermittent or persistent?  Should
> >>> ima_ahash_minsize be reset?
> >> If hw constantly does not work then it is simply broken.
> > True
> >
> >> You want to be protected from "random" failures.
> >> For me it is not the case either... If it works then it works...
> > This discussion isn't about your particular HW environment, but a
> > general question.  For example, suppose we were discussing a laptop with
> > a HW crypto accelerator.  If the HW crypto broke, I would at least want
> > to be able to quiesce the system properly.  I'd most likely want to be
> > able to continue using my laptop with software crypto.
> 
> Driver probing code will detect that HW is not responding and driver
> will not be enabled...
> 
> IMA will not be able to use it...
> 
> It is the same story as with any other HW and driver in the system.

Right, but my concern is not about unloading the kernel module, but
about the IMA module parameters left initialized.  The existing code
will continue using ahash (software version), even though the kernel
module was unloaded, not shash.  My question is about the software
implementations of ahash vs. shash performance.

Mimi

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

[Dmitry Kasatkin]

On 07/07/14 19:34, Mimi Zohar wrote:
> On Mon, 2014-07-07 at 19:11 +0300, Dmitry Kasatkin wrote: 
>> On 07/07/14 18:44, Mimi Zohar wrote:
>>> On Mon, 2014-07-07 at 16:37 +0300, Dmitry Kasatkin wrote: 
>>>> On 07/07/14 14:56, Mimi Zohar wrote:
>>>>> On Fri, 2014-07-04 at 15:05 +0300, Dmitry Kasatkin wrote: 
>>>>>> +/**
>>>>> This is the kernel-doc delimiter.
>>>>>
>>>>>> + * ima_calc_file_hash - calculae file hash
>>>>>> + *
>>>>> Missing kernel-doc argument descriptions.  Refer to
>>>>> Documentation/kernel-doc-nano-HOWTO.txt.
>>> Not defining the arguments results in a kernel-doc warning.  Providing
>>> kernel-doc is nice, but is unnecessary in this case, as it isn't an
>>> exported loadable module, nor an externally visible function to other
>>> kernel files.  Either remove the extra asterisk, making it a regular
>>> comment, or add the arguments.
>>>
>>>> There is no need to explain arguments as they self-evident.
>>>>
>>>>>> + * if ima_ahash_minsize parameter is non-zero, this function uses
>>>>>> + * ahash for hash caclulation. ahash performance varies for different
>>>>>> + * data sizes on different crypto accelerators. shash performance might
>>>>>> + * be better for small file. 'ima.ahash_minsize' module parameter allows
>>>>>> + * to specify the best value for the system.
>>>>>> + *
>>>>>> + * If ahash fails, it fallbacks to shash.
>>>>>> + */
>>>>>> +int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash)
>>>>>> +{
>>>>>> +	loff_t i_size;
>>>>>> +	int rc;
>>>>>> +
>>>>>> +	i_size = i_size_read(file_inode(file));
>>>>>> +
>>>>>> +	if (ima_ahash_minsize && i_size >= ima_ahash_minsize) {
>>>>>> +		rc = ima_calc_file_ahash(file, hash);
>>>>>> +		if (!rc)
>>>>>> +			return 0;
>>>>>> +	}
>>>>>> +
>>>>>> +	return ima_calc_file_shash(file, hash);
>>>>>> +}
>>>>> If the crypto accelerator fails, it falls back to using shash.  Is their
>>>>> any indication that the HW error is intermittent or persistent?  Should
>>>>> ima_ahash_minsize be reset?
>>>> If hw constantly does not work then it is simply broken.
>>> True
>>>
>>>> You want to be protected from "random" failures.
>>>> For me it is not the case either... If it works then it works...
>>> This discussion isn't about your particular HW environment, but a
>>> general question.  For example, suppose we were discussing a laptop with
>>> a HW crypto accelerator.  If the HW crypto broke, I would at least want
>>> to be able to quiesce the system properly.  I'd most likely want to be
>>> able to continue using my laptop with software crypto.
>> Driver probing code will detect that HW is not responding and driver
>> will not be enabled...
>>
>> IMA will not be able to use it...
>>
>> It is the same story as with any other HW and driver in the system.
> Right, but my concern is not about unloading the kernel module, but
> about the IMA module parameters left initialized.  The existing code
> will continue using ahash (software version), even though the kernel
> module was unloaded, not shash.  My question is about the software
> implementations of ahash vs. shash performance.
>
> Mimi

If HW driver will not be available, ahash loads generic driver which is
using shash.
Performance of that will be the same as for using shash directly.

- Dmitry

>

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

[Marek Vasut]

On Tuesday, July 08, 2014 at 10:07:16 AM, Dmitry Kasatkin wrote:
[...]
> > Right, but my concern is not about unloading the kernel module, but
> > about the IMA module parameters left initialized.  The existing code
> > will continue using ahash (software version), even though the kernel
> > module was unloaded, not shash.  My question is about the software
> > implementations of ahash vs. shash performance.
> > 
> > Mimi
> 
> If HW driver will not be available, ahash loads generic driver which is
> using shash.
> Performance of that will be the same as for using shash directly.

Hi Dmitry, I think Mimi is concerned about the crypto accelerator dying mid-
flight.

Imagine a situation where you have a hardware crypto accelerator connected via 
USB. You happily use IMA with this setup for days and then someone comes around 
and pulls the USB cable out. Will this be able to cope with such situation, for 
example by switching to software operations or such in some sane way ?

I presume that's the concern here.

Best regards,
Marek Vasut

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

[Dmitry Kasatkin]

On 10 July 2014 00:00, Marek Vasut <marex@denx.de> wrote:
> On Tuesday, July 08, 2014 at 10:07:16 AM, Dmitry Kasatkin wrote:
> [...]
>> > Right, but my concern is not about unloading the kernel module, but
>> > about the IMA module parameters left initialized.  The existing code
>> > will continue using ahash (software version), even though the kernel
>> > module was unloaded, not shash.  My question is about the software
>> > implementations of ahash vs. shash performance.
>> >
>> > Mimi
>>
>> If HW driver will not be available, ahash loads generic driver which is
>> using shash.
>> Performance of that will be the same as for using shash directly.
>
> Hi Dmitry, I think Mimi is concerned about the crypto accelerator dying mid-
> flight.
>
> Imagine a situation where you have a hardware crypto accelerator connected via
> USB. You happily use IMA with this setup for days and then someone comes around
> and pulls the USB cable out. Will this be able to cope with such situation, for
> example by switching to software operations or such in some sane way ?
>
> I presume that's the concern here.
>
> Best regards,
> Marek Vasut

Hi Marek,

Nice to here from you. How was your rest stay at Japan?

I have not seen any expression of such concern.
But as we fallback to early allocated shash, which is not USB yet,
then there is no problem.
ahash itself does not bring any other additional problem than shash itself.
They are compiled builtin together.

Thanks,
Dmitry

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

[Marek Vasut]

On Thursday, July 10, 2014 at 01:05:39 AM, Dmitry Kasatkin wrote:
> On 10 July 2014 00:00, Marek Vasut <marex@denx.de> wrote:
> > On Tuesday, July 08, 2014 at 10:07:16 AM, Dmitry Kasatkin wrote:
> > [...]
> > 
> >> > Right, but my concern is not about unloading the kernel module, but
> >> > about the IMA module parameters left initialized.  The existing code
> >> > will continue using ahash (software version), even though the kernel
> >> > module was unloaded, not shash.  My question is about the software
> >> > implementations of ahash vs. shash performance.
> >> > 
> >> > Mimi
> >> 
> >> If HW driver will not be available, ahash loads generic driver which is
> >> using shash.
> >> Performance of that will be the same as for using shash directly.
> > 
> > Hi Dmitry, I think Mimi is concerned about the crypto accelerator dying
> > mid- flight.
> > 
> > Imagine a situation where you have a hardware crypto accelerator
> > connected via USB. You happily use IMA with this setup for days and then
> > someone comes around and pulls the USB cable out. Will this be able to
> > cope with such situation, for example by switching to software
> > operations or such in some sane way ?
> > 
> > I presume that's the concern here.
> > 
> > Best regards,
> > Marek Vasut
> 
> Hi Marek,

Hi!

> Nice to here from you. How was your rest stay at Japan?

Thanks for asking, not sure there is a super-positive ultra-awesome word to 
express that, so in short, I had the time of my life. Love that country ;-)

> I have not seen any expression of such concern.

All right, that was my understanding of the entire discussion -- an accelerator 
dying mid-way and what will IMA do about that.

> But as we fallback to early allocated shash, which is not USB yet,
> then there is no problem.
> ahash itself does not bring any other additional problem than shash itself.
> They are compiled builtin together.

Sure, I understood that. But what will happen if the ahash accelerator stops 
working mid-flight, will IMA also go bonkers or is there some graceful stop?

Best regards,
Marek Vasut

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

[Dmitry Kasatkin]

On 10/07/14 11:02, Marek Vasut wrote:
> On Thursday, July 10, 2014 at 01:05:39 AM, Dmitry Kasatkin wrote:
>> On 10 July 2014 00:00, Marek Vasut <marex@denx.de> wrote:
>>> On Tuesday, July 08, 2014 at 10:07:16 AM, Dmitry Kasatkin wrote:
>>> [...]
>>>
>>>>> Right, but my concern is not about unloading the kernel module, but
>>>>> about the IMA module parameters left initialized.  The existing code
>>>>> will continue using ahash (software version), even though the kernel
>>>>> module was unloaded, not shash.  My question is about the software
>>>>> implementations of ahash vs. shash performance.
>>>>>
>>>>> Mimi
>>>> If HW driver will not be available, ahash loads generic driver which is
>>>> using shash.
>>>> Performance of that will be the same as for using shash directly.
>>> Hi Dmitry, I think Mimi is concerned about the crypto accelerator dying
>>> mid- flight.
>>>
>>> Imagine a situation where you have a hardware crypto accelerator
>>> connected via USB. You happily use IMA with this setup for days and then
>>> someone comes around and pulls the USB cable out. Will this be able to
>>> cope with such situation, for example by switching to software
>>> operations or such in some sane way ?
>>>
>>> I presume that's the concern here.
>>>
>>> Best regards,
>>> Marek Vasut
>> Hi Marek,
> Hi!
>
>> Nice to here from you. How was your rest stay at Japan?
> Thanks for asking, not sure there is a super-positive ultra-awesome word to 
> express that, so in short, I had the time of my life. Love that country ;-)
>
>> I have not seen any expression of such concern.
> All right, that was my understanding of the entire discussion -- an accelerator 
> dying mid-way and what will IMA do about that.
>
>> But as we fallback to early allocated shash, which is not USB yet,
>> then there is no problem.
>> ahash itself does not bring any other additional problem than shash itself.
>> They are compiled builtin together.
> Sure, I understood that. But what will happen if the ahash accelerator stops 
> working mid-flight, will IMA also go bonkers or is there some graceful stop?

shash fallback will be used.

> Best regards,
> Marek Vasut
>

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

[Marek Vasut]

On Thursday, July 10, 2014 at 01:18:41 PM, Dmitry Kasatkin wrote:
[...]
> > All right, that was my understanding of the entire discussion -- an
> > accelerator dying mid-way and what will IMA do about that.
> > 
> >> But as we fallback to early allocated shash, which is not USB yet,
> >> then there is no problem.
> >> ahash itself does not bring any other additional problem than shash
> >> itself. They are compiled builtin together.
> > 
> > Sure, I understood that. But what will happen if the ahash accelerator
> > stops working mid-flight, will IMA also go bonkers or is there some
> > graceful stop?
> 
> shash fallback will be used.

Ah, thank you for clearing this!

Best regards,
Marek Vasut

[PATCH v3 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. Buffer size
can be specified using 'ahash_bufsize' module parameter. Default buffer
size is 4096 bytes.

Changes in v3:
- kernel parameter replaced with module parameter

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

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 03c8452..7d7c38d 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -1327,6 +1327,14 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 			different crypto accelerators. This option can be used
 			to achieve best performance for particular HW.
 
+	ima.ahash_bufsize= [IMA] Asynchronous hash buffer size
+			Format: <bufsize>
+			Set hashing buffer size. Default: 4k.
+
+			ahash performance varies for different chunk sizes on
+			different crypto accelerators. This option can be used
+			to achieve best performance for particular HW.
+
 	init=		[KNL]
 			Format: <full_path>
 			Run specified binary instead of /sbin/init as init
diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
index bc38160..2340238 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -37,6 +37,33 @@ static unsigned long ima_ahash_minsize;
 module_param_named(ahash_minsize, ima_ahash_minsize, ulong, 0644);
 MODULE_PARM_DESC(ahash_minsize, "Minimum file size for ahash use");
 
+/* default is 0 - 1 page. */
+static int ima_maxorder;
+static unsigned int ima_bufsize = PAGE_SIZE;
+
+static int param_set_bufsize(const char *val, const struct kernel_param *kp)
+{
+	unsigned long long size;
+	int order;
+
+	size = memparse(val, NULL);
+	order = get_order(size);
+	if (order >= MAX_ORDER)
+		return -EINVAL;
+	ima_maxorder = order;
+	ima_bufsize = PAGE_SIZE << order;
+	return 0;
+}
+
+static struct kernel_param_ops param_ops_bufsize = {
+	.set = param_set_bufsize,
+	.get = param_get_uint,
+};
+#define param_check_bufsize(name, p) __param_check(name, p, unsigned int)
+
+module_param_named(ahash_bufsize, ima_bufsize, bufsize, 0644);
+MODULE_PARM_DESC(ahash_bufsize, "Maximum ahash buffer size");
+
 static struct crypto_shash *ima_shash_tfm;
 static struct crypto_ahash *ima_ahash_tfm;
 
@@ -106,6 +133,68 @@ static void ima_free_tfm(struct crypto_shash *tfm)
 		crypto_free_shash(tfm);
 }
 
+/**
+ * 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.
+ *
+ * By default, ima_maxorder is 0 and it is equivalent to kmalloc(GFP_KERNEL)
+ *
+ * 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;
+	int order = ima_maxorder;
+	gfp_t gfp_mask = __GFP_WAIT | __GFP_NOWARN | __GFP_NORETRY;
+
+	if (order)
+		order = min(get_order(max_size), 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 struct crypto_ahash *ima_alloc_atfm(enum hash_algo algo)
 {
 	struct crypto_ahash *tfm = ima_ahash_tfm;
@@ -169,6 +258,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);
 
@@ -190,7 +280,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;
@@ -219,7 +313,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 v3 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 simultaneous
hashing and reading of the next chunk of data from the storage.

Changes in v3:
- better comments

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

diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
index 2340238..bc79531 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -253,12 +253,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);
 
@@ -284,36 +284,71 @@ 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 (!rbuf[1] && offset) {
+			/* if we do not use second buffer and it is not the
+			 * the first read/request, then wait for completion
+			 * of ahash_update() request from the previous cycle
+			 * before reading to the same buffer
+			 */
+			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 (rbuf[1] && offset) {
+			/* if we use second buffer and it is not the
+			 * first read/request, then wait for completion
+			 * of ahash_update() request from previous the cycle
+			 * before the next ahash_update()
+			 */
+			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, if we use two */
 	}
+	/* wait for the last update 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