Re: [PATCH v3 1/3] ima: use ahash API for file hash calculation

[Mimi Zohar]

On Mon, 2014-07-07 at 16:37 +0300, Dmitry Kasatkin wrote: 
> On 07/07/14 14:56, Mimi Zohar wrote:
> > On Fri, 2014-07-04 at 15:05 +0300, Dmitry Kasatkin wrote: 

> >>
> >> +/**
> > This is the kernel-doc delimiter.
> >
> >> + * ima_calc_file_hash - calculae file hash
> >> + *
> > Missing kernel-doc argument descriptions.  Refer to
> > Documentation/kernel-doc-nano-HOWTO.txt.

Not defining the arguments results in a kernel-doc warning.  Providing
kernel-doc is nice, but is unnecessary in this case, as it isn't an
exported loadable module, nor an externally visible function to other
kernel files.  Either remove the extra asterisk, making it a regular
comment, or add the arguments.

> 
> There is no need to explain arguments as they self-evident.
> 
> >> + * if ima_ahash_minsize parameter is non-zero, this function uses
> >> + * ahash for hash caclulation. ahash performance varies for different
> >> + * data sizes on different crypto accelerators. shash performance might
> >> + * be better for small file. 'ima.ahash_minsize' module parameter allows
> >> + * to specify the best value for the system.
> >> + *
> >> + * If ahash fails, it fallbacks to shash.
> >> + */
> >> +int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash)
> >> +{
> >> +	loff_t i_size;
> >> +	int rc;
> >> +
> >> +	i_size = i_size_read(file_inode(file));
> >> +
> >> +	if (ima_ahash_minsize && i_size >= ima_ahash_minsize) {
> >> +		rc = ima_calc_file_ahash(file, hash);
> >> +		if (!rc)
> >> +			return 0;
> >> +	}
> >> +
> >> +	return ima_calc_file_shash(file, hash);
> >> +}
> > If the crypto accelerator fails, it falls back to using shash.  Is their
> > any indication that the HW error is intermittent or persistent?  Should
> > ima_ahash_minsize be reset?
> 
> If hw constantly does not work then it is simply broken.

True

> You want to be protected from "random" failures.

> For me it is not the case either... If it works then it works...

This discussion isn't about your particular HW environment, but a
general question.  For example, suppose we were discussing a laptop with
a HW crypto accelerator.  If the HW crypto broke, I would at least want
to be able to quiesce the system properly.  I'd most likely want to be
able to continue using my laptop with software crypto.

> > If the crypto accelerator, built as a kernel module, is removed,
> > ima_ahash_minsize would still be set.  It would continue to use ahash.
> > Is this the correct behavior? Or should ima_ahash_minsize be reset?
> >
> 
> It cannot be removed, because it is used and module usage counter
> protects from removing...

Ok

Mimi

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