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

[Mimi Zohar]

Thank you for reviewing the code.  Will fix these and add
locking around integrity_templates.

Mimi

On Thu, 2008-05-22 at 12:39 +1000, James Morris wrote:
> 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

--
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/