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

[Dmitry Kasatkin <d.kasatkin@samsung.com>]

On 07/07/14 18:44, Mimi Zohar wrote:
> 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.

Driver probing code will detect that HW is not responding and driver
will not be enabled...

IMA will not be able to use it...

It is the same story as with any other HW and driver in the system.

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