Re: [RFC][Patch 4/4]integrity: IMA as an integrity service provider

From: James Morris
Date: Wed May 21 2008 - 22:39:57 EST


On Wed, 21 May 2008, Mimi Zohar wrote:

> +#define IMA_HASH_KEY(digest) (hash_long(\
> + (unsigned long)(*digest), IMA_HASH_BITS));

Please make this a static inline and use hash_ptr().

> + i_size = i_size_read(file->f_dentry->d_inode);
> + while (offset < i_size) {
> + int error;
> +
> + rbuf_len = kernel_read(file, offset, rbuf, PAGE_SIZE);
> + if (rbuf_len <= 0)
> + break;

Why is the return value of kernel_read() not being propagated to the
caller?

> + error = crypto_hash_update(desc, sg, rbuf_len);
> + if (error) {
> + result = -EINVAL;
> + break;
> + }

Why are you replacing the error code from crypto_hash_update() with
-EINVAL ?

> + tfm = crypto_alloc_hash(ima_hash, 0, CRYPTO_ALG_ASYNC);
> + if (IS_ERR(tfm)) {
> + printk(KERN_INFO "%s: failed to load %s transform: %ld\n",
> + __func__, ima_hash, PTR_ERR(tfm));
> + return -ENOSYS;
> + }

Similarly, why not propagate the actual error ?

(This kind of thing happens quite a lot in the code).



> +static int __init init_ima(void)
> +{
> + int error;
> + tfm_hash = crypto_alloc_hash(ima_hash, 0, CRYPTO_ALG_ASYNC);
> +
> + error = ima_init();
> + if (error)
> + goto out;
> + ima_fixup_inodes();
> + if (ima_base_hooks)
> + error = register_integrity(&ima_base_ops);
> + else
> + error = register_integrity(&ima_integrity_ops);
> + register_template("ima", &ima_template_ops);
> +out:
> + return error;
> +}

What if crypto_alloc_hash() fails ? Actually, where is it used?


- James
--
James Morris
<jmorris@xxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/