Re: [PATCH v1 1/3] ima: use ahash API for file hash calculation
On Mon, 2014-06-30 at 17:58 +0300, Dmitry Kasatkin wrote:
> On 26/06/14 14:54, Mimi Zohar wrote:
> > On Thu, 2014-06-19 at 18:20 +0300, Dmitry Kasatkin wrote:
> >> @@ -156,7 +316,7 @@ out:
> >>return rc;
> >> }
> >>
> >> -int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash)
> >> +static int ima_calc_file_shash(struct file *file, struct ima_digest_data
> >> *hash)
> >> {
> >>struct crypto_shash *tfm;
> >>int rc;
> >> @@ -172,6 +332,20 @@ int ima_calc_file_hash(struct file *file, struct
> >> ima_digest_data *hash)
> >>return rc;
> >> }
> >>
> >> +int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash)
> >> +{
> >> + loff_t i_size = i_size_read(file_inode(file));
> >> +
> >> + /* shash is more efficient small data
> >> + * ahash performance depends on data size and particular HW
> >> + * ima_ahash_size allows to specify the best value for the system
> >> + */
> >> + if (ima_ahash_size && i_size >= ima_ahash_size)
> >> + return ima_calc_file_ahash(file, hash);
> >> + else
> >> + return ima_calc_file_shash(file, hash);
> >> +}
> > If calculating the file hash using ahash fails, should it fall back to
> > using shash?
>
> If ahash fails, then it could be a HW error, which should not happen.
> IF HW fails device is broken.
I would assume it depends on the HW, if the entire device/system is
broken.
> Do you really want to fallback to shash?
Yes, in this case, there is no downside to letting it to continue
working, just slower, using the software crypto implementation. In any
case, it shouldn't be hard coded.
Mimi
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 1/3] ima: use ahash API for file hash calculation
On 26/06/14 14:54, Mimi Zohar wrote:
> On Thu, 2014-06-19 at 18:20 +0300, Dmitry Kasatkin wrote:
>> Async hash API allows to use HW acceleration for hash calculation.
>> It may give significant performance gain or/and reduce power consumption,
>> which might be very beneficial for battery powered devices.
>>
>> This patch introduces hash calculation using ahash API.
>>
>> ahash peformance depends on data size and particular HW. Under certain
>> limit, depending on the system, shash performance may be better.
>>
>> This patch also introduces 'ima_ahash_size' kernel parameter which can
>> be used to defines minimal data size to use with ahash. When this
>> parameter is not set or file size is smaller than defined by this
>> parameter, shash will be used. Thus, by defult, original shash
>> implementation is used.
>>
>> Signed-off-by: Dmitry Kasatkin
>> ---
>> Documentation/kernel-parameters.txt | 3 +
>> security/integrity/ima/ima_crypto.c | 182
>> +++-
>> 2 files changed, 181 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/kernel-parameters.txt
>> b/Documentation/kernel-parameters.txt
>> index a0c155c..f8efb01 100644
>> --- a/Documentation/kernel-parameters.txt
>> +++ b/Documentation/kernel-parameters.txt
>> @@ -1286,6 +1286,9 @@ bytes respectively. Such letter suffixes can also be
>> entirely omitted.
>> ihash_entries= [KNL]
>> Set number of hash buckets for inode cache.
>>
>> +ima_ahash_size=size [IMA]
>> +Set the minimal file size when use ahash API.
>> +
>> ima_appraise= [IMA] appraise integrity measurements
>> Format: { "off" | "enforce" | "fix" }
>> default: "enforce"
>> diff --git a/security/integrity/ima/ima_crypto.c
>> b/security/integrity/ima/ima_crypto.c
>> index ccd0ac8..b7a8650 100644
>> --- a/security/integrity/ima/ima_crypto.c
>> +++ b/security/integrity/ima/ima_crypto.c
>> @@ -25,7 +25,25 @@
>> #include
>> #include "ima.h"
>>
>> +
>> +struct ahash_completion {
>> +struct completion completion;
>> +int err;
>> +};
>> +
>> static struct crypto_shash *ima_shash_tfm;
>> +static struct crypto_ahash *ima_ahash_tfm;
>> +
>> +/* data size for ahash use */
>> +static loff_t ima_ahash_size;
>> +
>> +static int __init ima_ahash_setup(char *str)
>> +{
>> +int rc = kstrtoll(str, 10, &ima_ahash_size);
> In general, variable definitions should be separated from code. A
> simple initialization is fine. Please separate variable definitions
> from code with a blank line.
>
>> +pr_info("ima_ahash_size = %lld\n", ima_ahash_size);
>> +return !rc;
>> +}
>> +__setup("ima_ahash_size=", ima_ahash_setup);
> This boot parameter name doesn't reflect its purpose, defining the
> minimum file size for using ahash. The next patch defines an additional
> boot parameter ima_ahash_bufsize. Perhaps defining a single boot
> parameter (eg. ima_ahash=) with multiple fields would be better.
>
>> /**
>> * ima_kernel_read - read file content
>> @@ -68,6 +86,14 @@ int ima_init_crypto(void)
>> hash_algo_name[ima_hash_algo], rc);
>> return rc;
>> }
>> +ima_ahash_tfm = crypto_alloc_ahash(hash_algo_name[ima_hash_algo], 0, 0);
>> +if (IS_ERR(ima_ahash_tfm)) {
>> +rc = PTR_ERR(ima_ahash_tfm);
>> +crypto_free_shash(ima_shash_tfm);
> Only crypto_alloc_ahash() failed, not crypto_alloc_shash(). shash has
> worked fine up to now. Why require both shash and ahash to succeed?
>> +pr_err("Can not allocate %s (reason: %ld)\n",
>> + hash_algo_name[ima_hash_algo], rc);
>> +return rc;
>> +}
>> return 0;
>> }
>>
>> @@ -93,9 +119,143 @@ static void ima_free_tfm(struct crypto_shash *tfm)
>> crypto_free_shash(tfm);
>> }
>>
>> -/*
>> - * Calculate the MD5/SHA1 file digest
>> - */
>> +static struct crypto_ahash *ima_alloc_atfm(enum hash_algo algo)
>> +{
>> +struct crypto_ahash *tfm = ima_ahash_tfm;
>> +int rc;
>> +
>> +if (algo != ima_hash_algo && algo < HASH_ALGO__LAST) {
>> +tfm = crypto_alloc_ahash(hash_algo_name[algo], 0, 0);
>> +if (IS_ERR(tfm)) {
>> +rc = PTR_ERR(tfm);
>> +pr_err("Can not allocate %s (reason: %d)\n",
>> + hash_algo_name[algo], rc);
>> +}
>> +}
>> +return tfm;
>> +}
>> +
>> +static void ima_free_atfm(struct crypto_ahash *tfm)
>> +{
>> +if (tfm != ima_ahash_tfm)
>> +crypto_free_ahash(tfm);
>> +}
>> +
>> +static void ahash_complete(struct crypto_async_request *req, int err)
>> +{
>> +struct ahash_completion *res = req->data;
>> +
>> +if (err == -EINPROGRESS)
>> +return;
>> +res->err = err;
>> +complete(&res->completion);
>> +}
>> +
>> +static int ahash_wait(int err, struct ahash_completion *res)
>> +{
>> +switch (err) {
>> +case 0:
>> +
Re: [PATCH v1 1/3] ima: use ahash API for file hash calculation
On Thu, 2014-06-19 at 18:20 +0300, Dmitry Kasatkin wrote:
> Async hash API allows to use HW acceleration for hash calculation.
> It may give significant performance gain or/and reduce power consumption,
> which might be very beneficial for battery powered devices.
>
> This patch introduces hash calculation using ahash API.
>
> ahash peformance depends on data size and particular HW. Under certain
> limit, depending on the system, shash performance may be better.
>
> This patch also introduces 'ima_ahash_size' kernel parameter which can
> be used to defines minimal data size to use with ahash. When this
> parameter is not set or file size is smaller than defined by this
> parameter, shash will be used. Thus, by defult, original shash
> implementation is used.
>
> Signed-off-by: Dmitry Kasatkin
> ---
> Documentation/kernel-parameters.txt | 3 +
> security/integrity/ima/ima_crypto.c | 182
> +++-
> 2 files changed, 181 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/kernel-parameters.txt
> b/Documentation/kernel-parameters.txt
> index a0c155c..f8efb01 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -1286,6 +1286,9 @@ bytes respectively. Such letter suffixes can also be
> entirely omitted.
> ihash_entries= [KNL]
> Set number of hash buckets for inode cache.
>
> + ima_ahash_size=size [IMA]
> + Set the minimal file size when use ahash API.
> +
> ima_appraise= [IMA] appraise integrity measurements
> Format: { "off" | "enforce" | "fix" }
> default: "enforce"
> diff --git a/security/integrity/ima/ima_crypto.c
> b/security/integrity/ima/ima_crypto.c
> index ccd0ac8..b7a8650 100644
> --- a/security/integrity/ima/ima_crypto.c
> +++ b/security/integrity/ima/ima_crypto.c
> @@ -25,7 +25,25 @@
> #include
> #include "ima.h"
>
> +
> +struct ahash_completion {
> + struct completion completion;
> + int err;
> +};
> +
> static struct crypto_shash *ima_shash_tfm;
> +static struct crypto_ahash *ima_ahash_tfm;
> +
> +/* data size for ahash use */
> +static loff_t ima_ahash_size;
> +
> +static int __init ima_ahash_setup(char *str)
> +{
> + int rc = kstrtoll(str, 10, &ima_ahash_size);
In general, variable definitions should be separated from code. A
simple initialization is fine. Please separate variable definitions
from code with a blank line.
> + pr_info("ima_ahash_size = %lld\n", ima_ahash_size);
> + return !rc;
> +}
> +__setup("ima_ahash_size=", ima_ahash_setup);
This boot parameter name doesn't reflect its purpose, defining the
minimum file size for using ahash. The next patch defines an additional
boot parameter ima_ahash_bufsize. Perhaps defining a single boot
parameter (eg. ima_ahash=) with multiple fields would be better.
> /**
> * ima_kernel_read - read file content
> @@ -68,6 +86,14 @@ int ima_init_crypto(void)
> hash_algo_name[ima_hash_algo], rc);
> return rc;
> }
> + ima_ahash_tfm = crypto_alloc_ahash(hash_algo_name[ima_hash_algo], 0, 0);
> + if (IS_ERR(ima_ahash_tfm)) {
> + rc = PTR_ERR(ima_ahash_tfm);
> + crypto_free_shash(ima_shash_tfm);
Only crypto_alloc_ahash() failed, not crypto_alloc_shash(). shash has
worked fine up to now. Why require both shash and ahash to succeed?
> + pr_err("Can not allocate %s (reason: %ld)\n",
> +hash_algo_name[ima_hash_algo], rc);
> + return rc;
> + }
> return 0;
> }
>
> @@ -93,9 +119,143 @@ static void ima_free_tfm(struct crypto_shash *tfm)
> crypto_free_shash(tfm);
> }
>
> -/*
> - * Calculate the MD5/SHA1 file digest
> - */
> +static struct crypto_ahash *ima_alloc_atfm(enum hash_algo algo)
> +{
> + struct crypto_ahash *tfm = ima_ahash_tfm;
> + int rc;
> +
> + if (algo != ima_hash_algo && algo < HASH_ALGO__LAST) {
> + tfm = crypto_alloc_ahash(hash_algo_name[algo], 0, 0);
> + if (IS_ERR(tfm)) {
> + rc = PTR_ERR(tfm);
> + pr_err("Can not allocate %s (reason: %d)\n",
> +hash_algo_name[algo], rc);
> + }
> + }
> + return tfm;
> +}
> +
> +static void ima_free_atfm(struct crypto_ahash *tfm)
> +{
> + if (tfm != ima_ahash_tfm)
> + crypto_free_ahash(tfm);
> +}
> +
> +static void ahash_complete(struct crypto_async_request *req, int err)
> +{
> + struct ahash_completion *res = req->data;
> +
> + if (err == -EINPROGRESS)
> + return;
> + res->err = err;
> + complete(&res->completion);
> +}
> +
> +static int ahash_wait(int err, struct ahash_completion *res)
> +{
> + switch (err) {
> + case 0:
> + break;
> + case -EINPROGRESS:
> + case -EBUSY:
> + wait_for_completion(&res->completion);
> + re
Re: [PATCH v1 1/3] ima: use ahash API for file hash calculation
On 23/06/14 14:32, Mimi Zohar wrote:
> On Thu, 2014-06-19 at 18:20 +0300, Dmitry Kasatkin wrote:
>> Async hash API allows to use HW acceleration for hash calculation.
>> It may give significant performance gain or/and reduce power consumption,
>> which might be very beneficial for battery powered devices.
>>
>> This patch introduces hash calculation using ahash API.
>>
>> ahash peformance depends on data size and particular HW. Under certain
>> limit, depending on the system, shash performance may be better.
>>
>> This patch also introduces 'ima_ahash_size' kernel parameter which can
>> be used to defines minimal data size to use with ahash. When this
>> parameter is not set or file size is smaller than defined by this
>> parameter, shash will be used. Thus, by defult, original shash
>> implementation is used.
>>
>> Signed-off-by: Dmitry Kasatkin
> Thanks, Dmitry. What type of values are you using for ima_ahash_size?
> Have you specified ima_ahash_size, when booting with an initramfs?
> ahash should probably only be used after pivoting root.
>
> thanks,
Hi,
There is no guideline because it depends on the HW...
On SW it does not mater.
To check it up and compare you can set it to "1" to make it working for
"any" file size.
- Dmitry
>
> Mimi
>
>> ---
>> Documentation/kernel-parameters.txt | 3 +
>> security/integrity/ima/ima_crypto.c | 182
>> +++-
>> 2 files changed, 181 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/kernel-parameters.txt
>> b/Documentation/kernel-parameters.txt
>> index a0c155c..f8efb01 100644
>> --- a/Documentation/kernel-parameters.txt
>> +++ b/Documentation/kernel-parameters.txt
>> @@ -1286,6 +1286,9 @@ bytes respectively. Such letter suffixes can also be
>> entirely omitted.
>> ihash_entries= [KNL]
>> Set number of hash buckets for inode cache.
>>
>> +ima_ahash_size=size [IMA]
>> +Set the minimal file size when use ahash API.
>> +
>> ima_appraise= [IMA] appraise integrity measurements
>> Format: { "off" | "enforce" | "fix" }
>> default: "enforce"
>> diff --git a/security/integrity/ima/ima_crypto.c
>> b/security/integrity/ima/ima_crypto.c
>> index ccd0ac8..b7a8650 100644
>> --- a/security/integrity/ima/ima_crypto.c
>> +++ b/security/integrity/ima/ima_crypto.c
>> @@ -25,7 +25,25 @@
>> #include
>> #include "ima.h"
>>
>> +
>> +struct ahash_completion {
>> +struct completion completion;
>> +int err;
>> +};
>> +
>> static struct crypto_shash *ima_shash_tfm;
>> +static struct crypto_ahash *ima_ahash_tfm;
>> +
>> +/* data size for ahash use */
>> +static loff_t ima_ahash_size;
>> +
>> +static int __init ima_ahash_setup(char *str)
>> +{
>> +int rc = kstrtoll(str, 10, &ima_ahash_size);
>> +pr_info("ima_ahash_size = %lld\n", ima_ahash_size);
>> +return !rc;
>> +}
>> +__setup("ima_ahash_size=", ima_ahash_setup);
>>
>> /**
>> * ima_kernel_read - read file content
>> @@ -68,6 +86,14 @@ int ima_init_crypto(void)
>> hash_algo_name[ima_hash_algo], rc);
>> return rc;
>> }
>> +ima_ahash_tfm = crypto_alloc_ahash(hash_algo_name[ima_hash_algo], 0, 0);
>> +if (IS_ERR(ima_ahash_tfm)) {
>> +rc = PTR_ERR(ima_ahash_tfm);
>> +crypto_free_shash(ima_shash_tfm);
>> +pr_err("Can not allocate %s (reason: %ld)\n",
>> + hash_algo_name[ima_hash_algo], rc);
>> +return rc;
>> +}
>> return 0;
>> }
>>
>> @@ -93,9 +119,143 @@ static void ima_free_tfm(struct crypto_shash *tfm)
>> crypto_free_shash(tfm);
>> }
>>
>> -/*
>> - * Calculate the MD5/SHA1 file digest
>> - */
>> +static struct crypto_ahash *ima_alloc_atfm(enum hash_algo algo)
>> +{
>> +struct crypto_ahash *tfm = ima_ahash_tfm;
>> +int rc;
>> +
>> +if (algo != ima_hash_algo && algo < HASH_ALGO__LAST) {
>> +tfm = crypto_alloc_ahash(hash_algo_name[algo], 0, 0);
>> +if (IS_ERR(tfm)) {
>> +rc = PTR_ERR(tfm);
>> +pr_err("Can not allocate %s (reason: %d)\n",
>> + hash_algo_name[algo], rc);
>> +}
>> +}
>> +return tfm;
>> +}
>> +
>> +static void ima_free_atfm(struct crypto_ahash *tfm)
>> +{
>> +if (tfm != ima_ahash_tfm)
>> +crypto_free_ahash(tfm);
>> +}
>> +
>> +static void ahash_complete(struct crypto_async_request *req, int err)
>> +{
>> +struct ahash_completion *res = req->data;
>> +
>> +if (err == -EINPROGRESS)
>> +return;
>> +res->err = err;
>> +complete(&res->completion);
>> +}
>> +
>> +static int ahash_wait(int err, struct ahash_completion *res)
>> +{
>> +switch (err) {
>> +case 0:
>> +break;
>> +case -EINPROGRESS:
>> +case -EBUSY:
>> +wait_for_completion(&res->completion);
>> +reinit_completion(&res->completion);
>> +err = r
Re: [PATCH v1 1/3] ima: use ahash API for file hash calculation
On Thu, 2014-06-19 at 18:20 +0300, Dmitry Kasatkin wrote:
> Async hash API allows to use HW acceleration for hash calculation.
> It may give significant performance gain or/and reduce power consumption,
> which might be very beneficial for battery powered devices.
>
> This patch introduces hash calculation using ahash API.
>
> ahash peformance depends on data size and particular HW. Under certain
> limit, depending on the system, shash performance may be better.
>
> This patch also introduces 'ima_ahash_size' kernel parameter which can
> be used to defines minimal data size to use with ahash. When this
> parameter is not set or file size is smaller than defined by this
> parameter, shash will be used. Thus, by defult, original shash
> implementation is used.
>
> Signed-off-by: Dmitry Kasatkin
Thanks, Dmitry. What type of values are you using for ima_ahash_size?
Have you specified ima_ahash_size, when booting with an initramfs?
ahash should probably only be used after pivoting root.
thanks,
Mimi
> ---
> Documentation/kernel-parameters.txt | 3 +
> security/integrity/ima/ima_crypto.c | 182
> +++-
> 2 files changed, 181 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/kernel-parameters.txt
> b/Documentation/kernel-parameters.txt
> index a0c155c..f8efb01 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -1286,6 +1286,9 @@ bytes respectively. Such letter suffixes can also be
> entirely omitted.
> ihash_entries= [KNL]
> Set number of hash buckets for inode cache.
>
> + ima_ahash_size=size [IMA]
> + Set the minimal file size when use ahash API.
> +
> ima_appraise= [IMA] appraise integrity measurements
> Format: { "off" | "enforce" | "fix" }
> default: "enforce"
> diff --git a/security/integrity/ima/ima_crypto.c
> b/security/integrity/ima/ima_crypto.c
> index ccd0ac8..b7a8650 100644
> --- a/security/integrity/ima/ima_crypto.c
> +++ b/security/integrity/ima/ima_crypto.c
> @@ -25,7 +25,25 @@
> #include
> #include "ima.h"
>
> +
> +struct ahash_completion {
> + struct completion completion;
> + int err;
> +};
> +
> static struct crypto_shash *ima_shash_tfm;
> +static struct crypto_ahash *ima_ahash_tfm;
> +
> +/* data size for ahash use */
> +static loff_t ima_ahash_size;
> +
> +static int __init ima_ahash_setup(char *str)
> +{
> + int rc = kstrtoll(str, 10, &ima_ahash_size);
> + pr_info("ima_ahash_size = %lld\n", ima_ahash_size);
> + return !rc;
> +}
> +__setup("ima_ahash_size=", ima_ahash_setup);
>
> /**
> * ima_kernel_read - read file content
> @@ -68,6 +86,14 @@ int ima_init_crypto(void)
> hash_algo_name[ima_hash_algo], rc);
> return rc;
> }
> + ima_ahash_tfm = crypto_alloc_ahash(hash_algo_name[ima_hash_algo], 0, 0);
> + if (IS_ERR(ima_ahash_tfm)) {
> + rc = PTR_ERR(ima_ahash_tfm);
> + crypto_free_shash(ima_shash_tfm);
> + pr_err("Can not allocate %s (reason: %ld)\n",
> +hash_algo_name[ima_hash_algo], rc);
> + return rc;
> + }
> return 0;
> }
>
> @@ -93,9 +119,143 @@ static void ima_free_tfm(struct crypto_shash *tfm)
> crypto_free_shash(tfm);
> }
>
> -/*
> - * Calculate the MD5/SHA1 file digest
> - */
> +static struct crypto_ahash *ima_alloc_atfm(enum hash_algo algo)
> +{
> + struct crypto_ahash *tfm = ima_ahash_tfm;
> + int rc;
> +
> + if (algo != ima_hash_algo && algo < HASH_ALGO__LAST) {
> + tfm = crypto_alloc_ahash(hash_algo_name[algo], 0, 0);
> + if (IS_ERR(tfm)) {
> + rc = PTR_ERR(tfm);
> + pr_err("Can not allocate %s (reason: %d)\n",
> +hash_algo_name[algo], rc);
> + }
> + }
> + return tfm;
> +}
> +
> +static void ima_free_atfm(struct crypto_ahash *tfm)
> +{
> + if (tfm != ima_ahash_tfm)
> + crypto_free_ahash(tfm);
> +}
> +
> +static void ahash_complete(struct crypto_async_request *req, int err)
> +{
> + struct ahash_completion *res = req->data;
> +
> + if (err == -EINPROGRESS)
> + return;
> + res->err = err;
> + complete(&res->completion);
> +}
> +
> +static int ahash_wait(int err, struct ahash_completion *res)
> +{
> + switch (err) {
> + case 0:
> + break;
> + case -EINPROGRESS:
> + case -EBUSY:
> + wait_for_completion(&res->completion);
> + reinit_completion(&res->completion);
> + err = res->err;
> + /* fall through */
> + default:
> + pr_crit("ahash calculation failed: err: %d\n", err);
> + }
> +
> + return err;
> +}
> +
> +static int ima_calc_file_hash_atfm(struct file *file,
> +struct ima_digest_data *hash,
> +
