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

[James Morris <jmorris@namei.org>]

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@namei.org>