On Wed, 2020-10-21 at 11:16 +0530, Sumit Garg wrote:

Thanks Mimi for your comments.

On Wed, 21 Oct 2020 at 08:51, Mimi Zohar [off-list ref] wrote:

quoted

On Wed, 2020-10-07 at 15:37 +0530, Sumit Garg wrote:

quoted

+/*
+ * trusted_destroy - clear and free the key's payload
+ */
+static void trusted_destroy(struct key *key)
+{
+     kfree_sensitive(key->payload.data[0]);
+}
+
+struct key_type key_type_trusted = {
+     .name = "trusted",
+     .instantiate = trusted_instantiate,
+     .update = trusted_update,
+     .destroy = trusted_destroy,
+     .describe = user_describe,
+     .read = trusted_read,
+};
+EXPORT_SYMBOL_GPL(key_type_trusted);
+
+static int __init init_trusted(void)
+{
+     int i, ret = 0;
+
+     for (i = 0; i < ARRAY_SIZE(trusted_key_sources); i++) {
+             if (trusted_key_source &&
+                 strncmp(trusted_key_source, trusted_key_sources[i].name,
+                         strlen(trusted_key_sources[i].name)))
+                     continue;
+
+             trusted_key_ops = trusted_key_sources[i].ops;
+
+             ret = trusted_key_ops->init();
+             if (!ret)
+                     break;
+     }

In the case when the module paramater isn't specified and both TPM and
TEE are enabled, trusted_key_ops is set to the last source initialized.

I guess there is some misunderstanding. Here it's only a single trust
source (TPM *or* TEE) is initialized and only that trust source would
be active at runtime. And trusted_key_ops would be initialized to the
first trust source whose initialization is successful (see check: "if
(!ret)").

My mistake.

quoted

After patch 2/4, the last trusted source initialized is TEE.  If the
intention is to limit it to either TPM or TEE, then trusted_key_ops
should have a default value, which could be overwritten at runtime.
That would address Luke Hind's concerns of making the decision at
compile time.

I think traversing the trust source list with the initial value being
TPM would be default value.

Agreed

quoted

trusted_key_ops should be defined as __ro_after_init, like is currently
done for other LSM structures.

Sure, will do.

Thanks

quoted

quoted

+
+     /*
+      * encrypted_keys.ko depends on successful load of this module even if
+      * trusted key implementation is not found.
+      */
+     if (ret == -ENODEV)
+             return 0;
+
+     return ret;
+}
+
+static void __exit cleanup_trusted(void)
+{
+     trusted_key_ops->exit();

If the intention is really to support both TPM and TEE trusted keys at
the same time, as James suggested, then the same "for" loop as in
init_trusted() is needed here and probably elsewhere.

Current intention is to only support a single trust source (TPM or
TEE) at runtime. But in future if there are use-cases then framework
can be extended to support multiple trust sources at runtime as well.

Ok, the last sentence of the patch description, "Also, add a module
parameter in order to select a particular trust source in case a
platform support multiple trust sources.", needs to be expanded to:
- indicate only one trust source at a time is supported
- indicate the default, if the module_param is not specified

I would also change the word from "add" to "define".   The new "source"
module parameter needs to be added to the admin-guide/kernel-parameters 
documentation.

thanks,

Mimi   



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel