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

On Mon, 2014-07-07 at 16:37 +0300, Dmitry Kasatkin wrote: 

On 07/07/14 14:56, Mimi Zohar wrote:

quoted

On Fri, 2014-07-04 at 15:05 +0300, Dmitry Kasatkin wrote: 

quoted

quoted

+/**

This is the kernel-doc delimiter.

quoted

+ * 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.

quoted

quoted

+ * 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.

quoted

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