Messages in this thread | | | Subject | Re: [RFC][Patch 4/4]integrity: IMA as an integrity service provider | From | Mimi Zohar <> | Date | Thu, 22 May 2008 14:39:43 -0400 |
| |
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
| |