ima: use asynchronous hash API for hash calculation

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 'ima_ahash' kernel 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.

Signed-off-by Dmitry Kasatkin <[email protected]>

---
 Documentation/kernel-parameters.txt |   5 +
 security/integrity/ima/ima_crypto.c | 185 +++++++++++++++++++++++++++++++++++-
 2 files changed, 186 insertions(+), 4 deletions(-)

Documentation/kernel-parameters.txt +5 -0

index 5a214a3..b406f5c 100644
@@ -1291,6 +1291,11 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 	ihash_entries=	[KNL]
 			Set number of hash buckets for inode cache.
 
+	ima_ahash=	[IMA] Asynchronous hash usage parameters
+			Format: <min_file_size>
+			Set the minimal file size when use asynchronous hash.
+			If ima_ahash is not provided, ahash usage is disabled.
+
 	ima_appraise=	[IMA] appraise integrity measurements
 			Format: { "off" | "enforce" | "fix" }
 			default: "enforce"

security/integrity/ima/ima_crypto.c +0 -0

index f82d1d7..5eb19b4 100644

security/integrity/ima/ima_crypto.c +160 -4

+struct ahash_completion {
+	struct completion completion;
+	int err;
+};
+
 static struct crypto_shash *ima_shash_tfm;
+static struct crypto_ahash *ima_ahash_tfm;
+
+/* minimal file size for ahash use */
+static loff_t ima_ahash_size;
+
+static int __init ima_ahash_setup(char *str)
+{
+	int rc;
+
+	rc = kstrtoll(str, 10, &ima_ahash_size);
+
+	return !rc;
+}
+__setup("ima_ahash=", ima_ahash_setup);
 
 /**
  * ima_kernel_read - read file content

@@ -93,9 +113,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("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;
+			vi 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 +313,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;

security/integrity/ima/ima_crypto.c +3 -0

+	return ima_calc_file_shash(file, hash);
+}
+
 /*
  * Calculate the hash of template data
  */

Quoted 4 lines · 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.

Quoted 8 lines

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 'ima_ahash' kernel 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.

Thank you for the updated patches. The changes should be listed here in the patch description.

I keep going back and forth as to whether the ahash routines should be totally separate, as posted, from the shash routines. Having separate functions is very clear/clean, but there is quite a bit of code duplication (eg. ima_calc_file_hash()/ima_calc_file_ahash(), ima_free_tfm()/ima_free_atfm(), ima_alloc_tfm()/ima_alloc_atfm()).

Soliciting comments ...

Quoted 28 lines

Signed-off-by Dmitry Kasatkin <[email protected]>

---
 Documentation/kernel-parameters.txt |   5 +
 security/integrity/ima/ima_crypto.c | 185 +++++++++++++++++++++++++++++++++++-
 2 files changed, 186 insertions(+), 4 deletions(-)

Documentation/kernel-parameters.txt +5 -0

index 5a214a3..b406f5c 100644
@@ -1291,6 +1291,11 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
      ihash_entries=  [KNL]
                      Set number of hash buckets for inode cache.
 
+     ima_ahash=      [IMA] Asynchronous hash usage parameters
+                     Format: <min_file_size>
+                     Set the minimal file size when use asynchronous hash.
+                     If ima_ahash is not provided, ahash usage is disabled.
+
      ima_appraise=   [IMA] appraise integrity measurements
                      Format: { "off" | "enforce" | "fix" }
                      default: "enforce"

security/integrity/ima/ima_crypto.c +0 -0

index f82d1d7..5eb19b4 100644

security/integrity/ima/ima_crypto.c line 28

Quoted 4 lines

security/integrity/ima/ima_crypto.c +1 -0

@@ -25,7 +25,27 @@
 #include <crypto/hash_info.h>
 #include "ima.h"
 
+

Extra blank line not needed.

Quoted 186 lines

security/integrity/ima/ima_crypto.c +160 -4

+struct ahash_completion {
+     struct completion completion;
+     int err;
+};
+
 static struct crypto_shash *ima_shash_tfm;
+static struct crypto_ahash *ima_ahash_tfm;
+
+/* minimal file size for ahash use */
+static loff_t ima_ahash_size;
+
+static int __init ima_ahash_setup(char *str)
+{
+     int rc;
+
+     rc = kstrtoll(str, 10, &ima_ahash_size);
+
+     return !rc;
+}
+__setup("ima_ahash=", ima_ahash_setup);
 
 /**
  * ima_kernel_read - read file content

@@ -93,9 +113,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("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;
+                     vi 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 +313,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 +329,26 @@ int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash)
      return rc;
 }
 

security/integrity/ima/ima_crypto.c int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash) · line 337

Quoted 6 lines

security/integrity/ima/ima_crypto.c +6 -0

+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));

Depending on the config options, i_size_read() does more than just
access the inode's i_size. Instead of calling it again, perhaps it
should be passed to ima_calc_file_hash()/ahash().

security/integrity/ima/ima_crypto.c int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash) · line 342

Quoted 5 lines

security/integrity/ima/ima_crypto.c +5 -0

+
+     /* shash is more efficient for small data
+      * ahash performance depends on data size and particular HW
+      * ima_ahash_size allows to specify the best value for the system
+      */

Function descriptions belong above the function, not inside.

security/integrity/ima/ima_crypto.c int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash) · line 348

Quoted 6 lines

security/integrity/ima/ima_crypto.c +6 -0

+     if (ima_ahash_size && i_size >= ima_ahash_size) {
+             rc = ima_calc_file_ahash(file, hash);
+             if (!rc)
+                     return 0;
+     }
+     /* fallback to shash if ahash failed */

Same here.

thanks,

Mimi

security/integrity/ima/ima_crypto.c int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash) · line 354

Quoted 6 lines

security/integrity/ima/ima_crypto.c +3 -0

+     return ima_calc_file_shash(file, hash);
+}
+
 /*
  * Calculate the hash of template data
  */

Quoted 17 lines · Mimi Zohar

Quoted 4 lines · 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.

Quoted 8 lines

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 'ima_ahash' kernel 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.

Thank you for the updated patches. The changes should be listed here in the patch description.

Quoted 8 lines

I keep going back and forth as to whether the ahash routines should be totally separate, as posted, from the shash routines. Having separate functions is very clear/clean, but there is quite a bit of code duplication (eg. ima_calc_file_hash()/ima_calc_file_ahash(), ima_free_tfm()/ima_free_atfm(), ima_alloc_tfm()/ima_alloc_atfm()).

Soliciting comments ...

I think what you say is a "pattern": alloc/calc/free.
But the code uses different API mostly and there very small duplication...

In fact with this 'clean' separation we can have CONFIG_ option to ifdef the code ot have it in separate file for those who really want to make smallest kernel possible...

Quoted 222 lines

Quoted 29 lines

Signed-off-by Dmitry Kasatkin <[email protected]>

---
 Documentation/kernel-parameters.txt |   5 +
 security/integrity/ima/ima_crypto.c | 185 +++++++++++++++++++++++++++++++++++-
 2 files changed, 186 insertions(+), 4 deletions(-)

Documentation/kernel-parameters.txt +5 -0

index 5a214a3..b406f5c 100644
@@ -1291,6 +1291,11 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
      ihash_entries=  [KNL]
                      Set number of hash buckets for inode cache.
 
+     ima_ahash=      [IMA] Asynchronous hash usage parameters
+                     Format: <min_file_size>
+                     Set the minimal file size when use asynchronous hash.
+                     If ima_ahash is not provided, ahash usage is disabled.
+
      ima_appraise=   [IMA] appraise integrity measurements
                      Format: { "off" | "enforce" | "fix" }
                      default: "enforce"

security/integrity/ima/ima_crypto.c +0 -0

index f82d1d7..5eb19b4 100644

security/integrity/ima/ima_crypto.c line 28

Quoted 4 lines

security/integrity/ima/ima_crypto.c +1 -0

@@ -25,7 +25,27 @@
 #include <crypto/hash_info.h>
 #include "ima.h"
 
+

Extra blank line not needed.

Quoted 186 lines

security/integrity/ima/ima_crypto.c +160 -4

+struct ahash_completion {
+     struct completion completion;
+     int err;
+};
+
 static struct crypto_shash *ima_shash_tfm;
+static struct crypto_ahash *ima_ahash_tfm;
+
+/* minimal file size for ahash use */
+static loff_t ima_ahash_size;
+
+static int __init ima_ahash_setup(char *str)
+{
+     int rc;
+
+     rc = kstrtoll(str, 10, &ima_ahash_size);
+
+     return !rc;
+}
+__setup("ima_ahash=", ima_ahash_setup);
 
 /**
  * ima_kernel_read - read file content

@@ -93,9 +113,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("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;
+                     vi 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 +313,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 +329,26 @@ int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash)
      return rc;
 }
 

Quoted 11 lines

security/integrity/ima/ima_crypto.c int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash) · line 337

Quoted 6 lines

security/integrity/ima/ima_crypto.c +6 -0

+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));

Depending on the config options, i_size_read() does more than just
access the inode's i_size. Instead of calling it again, perhaps it
should be passed to ima_calc_file_hash()/ahash().

I thought about it.. Indeed let's make it like that.

Quoted 8 lines

security/integrity/ima/ima_crypto.c int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash) · line 342

Quoted 5 lines

security/integrity/ima/ima_crypto.c +5 -0

+
+     /* shash is more efficient for small data
+      * ahash performance depends on data size and particular HW
+      * ima_ahash_size allows to specify the best value for the system
+      */

Function descriptions belong above the function, not inside.

This is a comment to the code..

Quoted 9 lines

security/integrity/ima/ima_crypto.c int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash) · line 348

Quoted 6 lines

security/integrity/ima/ima_crypto.c +6 -0

+     if (ima_ahash_size && i_size >= ima_ahash_size) {
+             rc = ima_calc_file_ahash(file, hash);
+             if (!rc)
+                     return 0;
+     }
+     /* fallback to shash if ahash failed */

Same here.

Same here...

But sure it can move to function description...

Quoted 12 lines

thanks,

Mimi

security/integrity/ima/ima_crypto.c int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash) · line 354

Quoted 6 lines

security/integrity/ima/ima_crypto.c +3 -0

+     return ima_calc_file_shash(file, hash);
+}
+
 /*
  * Calculate the hash of template data
  */

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

security/integrity/ima/ima_crypto.c +17 -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);
+}

security/integrity/ima/ima_crypto.c static void ima_free_tfm(struct crypto_shash *tfm) · line 122

Quoted 10 lines · Dmitry Kasatkin

security/integrity/ima/ima_crypto.c +7 -3

-/*
- * 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);

In the case where algo isn't the same as ima_hash_algo, won't this replace the existing ima_ahash_tfm without freeing it?

Mimi

Quoted 3 lines

security/integrity/ima/ima_crypto.c +3 -0

+             if (!IS_ERR(tfm)) {
+                     if (algo == ima_hash_algo)
+                             ima_ahash_tfm = tfm;

Quoted 14 lines

security/integrity/ima/ima_crypto.c +14 -0

+             } 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);
+}

Quoted 16 lines · Mimi Zohar

security/integrity/ima/ima_crypto.c static void ima_free_tfm(struct crypto_shash *tfm) · line 122

Quoted 10 lines · Dmitry Kasatkin

security/integrity/ima/ima_crypto.c +7 -3

-/*
- * 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);

In the case where algo isn't the same as ima_hash_algo, won't this replace the existing ima_ahash_tfm without freeing it?

Look to next comment...

Documentation/kernel-parameters.txt bytes respectively. Such letter suffixes can also be entirely omitted. · line 1297

Quoted 4 lines · Dmitry Kasatkin

Documentation/kernel-parameters.txt +4 -0

+     ima_ahash=      [IMA] Asynchronous hash usage parameters
+                     Format: <min_file_size>
+                     Set the minimal file size when use asynchronous hash.
+                     If ima_ahash is not provided, ahash usage is disabled.

<groan> ... another boot option...

Can we just set this to something sane, and then make a sysctl or something else at runtime to tweak it? The kernel won't use IMA much before userspace comes up, and it can surely live with a slightly suboptimal tuning until the boot scripts have a chance to go bang the tunable.

We should reserve command-line parameters for things that really need tweaking in early boot or are _needed_ to boot.

Quoted 2 lines

Quoted 2 lines

We should reserve command-line parameters for things that really need tweaking in early boot or are _needed_ to boot.

Is module param good enough or it should be sysctl?

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 by providing additional option to the 'ima_ahash=' kernel parameter. 'ima_ahash=2048,16384' specifies that minimal file size to use ahash is 2048 byes and buffer size is 16384 bytes. Default buffer size is 4096 bytes.

Signed-off-by Dmitry Kasatkin <[email protected]>

---
 Documentation/kernel-parameters.txt |  3 +-
 security/integrity/ima/ima_crypto.c | 85 ++++++++++++++++++++++++++++++++++---
 2 files changed, 81 insertions(+), 7 deletions(-)

Documentation/kernel-parameters.txt +2 -1

index b406f5c..529ba58 100644
@@ -1292,9 +1292,10 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 			Set number of hash buckets for inode cache.
 
 	ima_ahash=	[IMA] Asynchronous hash usage parameters
-			Format: <min_file_size>
+			Format: <min_file_size>[,<bufsize>]
 			Set the minimal file size when use asynchronous hash.
 			If ima_ahash is not provided, ahash usage is disabled.
+			bufsize - hashing buffer size. 4k if not specified.
 
 	ima_appraise=	[IMA] appraise integrity measurements
 			Format: { "off" | "enforce" | "fix" }

security/integrity/ima/ima_crypto.c +2 -1

index 5eb19b4..41f2695 100644
@@ -25,7 +25,6 @@
 #include <crypto/hash_info.h>
 #include "ima.h"
 
-
 struct ahash_completion {
 	struct completion completion;
 	int err;

@@ -36,14 +35,24 @@ static struct crypto_ahash *ima_ahash_tfm;
 
 /* minimal file size for ahash use */
 static loff_t ima_ahash_size;
+/* default is 0 - 1 page. */
+static int ima_max_order;
 
 static int __init ima_ahash_setup(char *str)

security/integrity/ima/ima_crypto.c +63 -2

-	rc = kstrtoll(str, 10, &ima_ahash_size);
+	pr_info("ima_ahash: minsize=%lld, bufsize=%lu\n",
+		ima_ahash_size, (PAGE_SIZE << ima_max_order));
 
-	return !rc;
+	return 1;
 }
 __setup("ima_ahash=", ima_ahash_setup);
 

@@ -166,6 +175,65 @@ 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;
+	unsigned int order;
+	gfp_t gfp_mask = __GFP_NOWARN | __GFP_WAIT | __GFP_NORETRY;
+
+	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)

@@ -176,6 +244,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);
 

security/integrity/ima/ima_crypto.c +2 -1

+	rbuf = ima_alloc_pages(i_size, &rbuf_size, 1);
 	if (!rbuf) {
 		rc = -ENOMEM;
 		goto out1;

@@ -226,7 +299,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);

Quoted 58 lines · 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 by providing additional option to the 'ima_ahash=' kernel parameter. 'ima_ahash=2048,16384' specifies that minimal file size to use ahash is 2048 byes and buffer size is 16384 bytes. Default buffer size is 4096 bytes.

Signed-off-by Dmitry Kasatkin <[email protected]>

---
 Documentation/kernel-parameters.txt |  3 +-
 security/integrity/ima/ima_crypto.c | 85 ++++++++++++++++++++++++++++++++++---
 2 files changed, 81 insertions(+), 7 deletions(-)

Documentation/kernel-parameters.txt +2 -1

index b406f5c..529ba58 100644
@@ -1292,9 +1292,10 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
                      Set number of hash buckets for inode cache.
 
      ima_ahash=      [IMA] Asynchronous hash usage parameters
-                     Format: <min_file_size>
+                     Format: <min_file_size>[,<bufsize>]
                      Set the minimal file size when use asynchronous hash.
                      If ima_ahash is not provided, ahash usage is disabled.
+                     bufsize - hashing buffer size. 4k if not specified.
 
      ima_appraise=   [IMA] appraise integrity measurements
                      Format: { "off" | "enforce" | "fix" }

security/integrity/ima/ima_crypto.c +2 -1

index 5eb19b4..41f2695 100644
@@ -25,7 +25,6 @@
 #include <crypto/hash_info.h>
 #include "ima.h"
 
-
 struct ahash_completion {
      struct completion completion;
      int err;

@@ -36,14 +35,24 @@ static struct crypto_ahash *ima_ahash_tfm;
 
 /* minimal file size for ahash use */
 static loff_t ima_ahash_size;
+/* default is 0 - 1 page. */
+static int ima_max_order;
 
 static int __init ima_ahash_setup(char *str)

security/integrity/ima/ima_crypto.c static struct crypto_ahash *ima_ahash_tfm; · line 50

Quoted 10 lines

security/integrity/ima/ima_crypto.c +8 -1

 {
-     int rc;
+     int ints[3] = { 0, };
+
+     str = get_options(str, ARRAY_SIZE(ints), ints);
+     if (!ints[0])
+             return 0;
+
+     ima_ahash_size = ints[1];
+     ima_max_order = get_order(ints[2]);

get_options() returns the number of options processed in ints[0]. Before assigning ima_max_order, we normally check to see if it exists. I guess in this case it doesn't matter since it would return 0 anyway.

Should there be any value checking here? Should the values be some multiple of 1024?

Quoted 85 lines

security/integrity/ima/ima_crypto.c +63 -2

-     rc = kstrtoll(str, 10, &ima_ahash_size);
+     pr_info("ima_ahash: minsize=%lld, bufsize=%lu\n",
+             ima_ahash_size, (PAGE_SIZE << ima_max_order));
 
-     return !rc;
+     return 1;
 }
 __setup("ima_ahash=", ima_ahash_setup);
 

@@ -166,6 +175,65 @@ 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;
+     unsigned int order;
+     gfp_t gfp_mask = __GFP_NOWARN | __GFP_WAIT | __GFP_NORETRY;
+
+     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)

@@ -176,6 +244,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);
 

security/integrity/ima/ima_crypto.c static int ima_calc_file_hash_atfm(struct file *file, · line 272

Quoted 8 lines

security/integrity/ima/ima_crypto.c +4 -1

@@ -197,7 +266,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.
+      */

The comment is nice explanation, but might be unnecessary if there was a function comment.

Mimi

Quoted 13 lines

security/integrity/ima/ima_crypto.c +2 -1

+     rbuf = ima_alloc_pages(i_size, &rbuf_size, 1);
      if (!rbuf) {
              rc = -ENOMEM;
              goto out1;

@@ -226,7 +299,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);