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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Thu, 22 May 2008 14:39:43 -0400
From:	Mimi Zohar <zohar@...ux.vnet.ibm.com>
To:	James Morris <jmorris@...ei.org>
Cc:	linux-kernel@...r.kernel.org, safford@...son.ibm.com,
	serue@...ux.vnet.ibm.com, sailer@...son.ibm.com, zohar@...ibm.com,
	Stephen Smalley <sds@...ho.nsa.gov>,
	CaseySchaufler <casey@...aufler-ca.com>
Subject: Re: [RFC][Patch 4/4]integrity: IMA as an integrity service provider


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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/