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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, 2014-07-02 at 21:20 +0300, Dmitry Kasatkin wrote: 
> On 2 July 2014 19:40, Mimi Zohar <zohar@xxxxxxxxxxxxxxxxxx> wrote:
> > On Tue, 2014-07-01 at 23:12 +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 '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 ...
> >
> 
> 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...

Ok, this is an acceptable reason.

thanks,

Mimi

> 
> >> Signed-off-by: Dmitry Kasatkin <d.kasatkin@xxxxxxxxxxx>
> >
> >> ---
> >>  Documentation/kernel-parameters.txt |   5 +
> >>  security/integrity/ima/ima_crypto.c | 185 +++++++++++++++++++++++++++++++++++-
> >>  2 files changed, 186 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> >> index 5a214a3..b406f5c 100644
> >> --- a/Documentation/kernel-parameters.txt
> >> +++ b/Documentation/kernel-parameters.txt
> >> @@ -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"
> >> diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
> >> index f82d1d7..5eb19b4 100644
> >> --- a/security/integrity/ima/ima_crypto.c
> >> +++ b/security/integrity/ima/ima_crypto.c
> >> @@ -25,7 +25,27 @@
> >>  #include <crypto/hash_info.h>
> >>  #include "ima.h"
> >>
> >> +
> >
> > Extra blank line not needed.
> >
> >> +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;
> >>  }
> >>
> >> +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.
> 
> 
> >> +
> >> +     /* 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..
> 
> >> +     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...
> 
> > thanks,
> >
> > Mimi
> >
> >> +     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 majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Kernel]     [Gnu Classpath]     [Gnu Crypto]     [DM Crypt]     [Netfilter]     [Bugtraq]

  Powered by Linux