Re: [v9, 3/4] tpm: Initialize TPM and get durations and timeouts

On Thu, Mar 31, 2016 at 08:58:47AM -0400, Stefan Berger wrote:

On 03/31/2016 04:24 AM, Jarkko Sakkinen wrote:

quoted

On Tue, Mar 29, 2016 at 02:19:13PM -0400, Stefan Berger wrote:

quoted

Add the retrieval of TPM 1.2 durations and timeouts. Since this requires
the startup of the TPM, do this for TPM 1.2 and TPM 2.

quoted

quoted

@@ -343,6 +362,55 @@ static const struct tpm_class_ops vtpm_proxy_tpm_ops = {
 };
 /*
+ * Code related to the startup of the TPM 2 and startup of TPM 1.2 +
+ * retrieval of timeouts and durations.
+ */
+
+static void vtpm_proxy_work(struct work_struct *work)
+{
+	struct proxy_dev *proxy_dev = container_of(work, struct proxy_dev,
+						   work);
+	int rc;
+
+	if (proxy_dev->flags & VTPM_PROXY_FLAG_TPM2)
+		rc = tpm2_startup(proxy_dev->chip, TPM2_SU_CLEAR);
+	else
+		rc = tpm_get_timeouts(proxy_dev->chip);
+
+	if (rc)
+		goto err;
+
+	rc = tpm_chip_register(proxy_dev->chip);
+	if (rc)
+		goto err;
+
+	return;
+
+err:
+	vtpm_proxy_fops_undo_open(proxy_dev);
+}
+
+/*
+ * vtpm_proxy_work_stop: make sure the work has finished
+ *
+ * This function is useful when user space closed the fd
+ * while the driver still determines timeouts.
+ */
+static void vtpm_proxy_work_stop(struct proxy_dev *proxy_dev)
+{
+	vtpm_proxy_fops_undo_open(proxy_dev);
+	flush_work(&proxy_dev->work);

The main proxy driver patch should implement cancel() callback and then
these could be swapped. This whole use of OPENED callback looks like a
hack that is done because cancel is not implemented.

What OPENED callback are you referring to? We have a OPENED flag but not a
callback.

The above handles the case where the vTPM process for example dies [during
TPM_Startup()] and the file descriptor is closed.
vtpm_proxy_fops_undo_open() ensures that the vtpm thread is not stuck
waiting for the response to the TPM_Startup(). The subsequent flush_work()
ensures that the thread has finished before we continue shutting down the
instance. This cannot be swapped.

Got you.

quoted

A new flag CANCELED could be added and functions would return -ECANCEL
if it is set. This should be part of original vTPM proxy driver
functionally.

The above has nothing to do with cancellation from what I can see. We have
an OPENED flag now which is set when the driver is fully operational and
cleared when it is not. We could instead use a SHUTDOWN or CLOSED flag that
works with the reverse meaning, clearing it where OPENED is set now and
setting it where OPENED is cleared. Would this help?

quoted

The current solution is unclean and hard to follow. It looks like "it
could work" but the OPENED flag has too many hats that it wears IMHO.

It has only one meaning which can be replaced with a flag as indicated
above.

That it is tested in vtpm_tpm_req_canceled is due to it indicating that the
driver is not operational for the current command anymore, which gets it out
of the loop in tpm_transmit. It would probably be worse to return a status
flag from the status() callback and return a flag in the req_complete_mask
that would then end up calling the recv() callback when there is nothing to
receive. So the way it is now it triggers -ECANCELED in the tpm_transmit
loop, which seems appropriate.

Reviewd-by: Jarkko Sakkinen [off-list ref]

   Stefan

/Jarkko

quoted

/Jarkko

quoted

+}
+
+/*
+ * vtpm_proxy_work_start: Schedule the work for TPM 1.2 & 2 initialization
+ */
+static inline void vtpm_proxy_work_start(struct proxy_dev *proxy_dev)
+{
+	queue_work(workqueue, &proxy_dev->work);
+}
+
+/*
  * Code related to creation and deletion of device pairs
  */
 static struct proxy_dev *vtpm_proxy_create_proxy_dev(void)

@@ -357,6 +425,7 @@ static struct proxy_dev *vtpm_proxy_create_proxy_dev(void)
 	init_waitqueue_head(&proxy_dev->wq);
 	mutex_init(&proxy_dev->buf_lock);
+	INIT_WORK(&proxy_dev->work, vtpm_proxy_work);
 	chip = tpm_chip_alloc(NULL, &vtpm_proxy_tpm_ops);
 	if (IS_ERR(chip)) {

@@ -427,9 +496,7 @@ static struct file *vtpm_proxy_create_device(
 	if (proxy_dev->flags & VTPM_PROXY_FLAG_TPM2)
 		proxy_dev->chip->flags |= TPM_CHIP_FLAG_TPM2;
-	rc = tpm_chip_register(proxy_dev->chip);
-	if (rc)
-		goto err_vtpm_fput;
+	vtpm_proxy_work_start(proxy_dev);
 	vtpm_new_dev->fd = fd;
 	vtpm_new_dev->major = MAJOR(proxy_dev->chip->dev.devt);

@@ -438,12 +505,6 @@ static struct file *vtpm_proxy_create_device(
 	return file;
-err_vtpm_fput:
-	put_unused_fd(fd);
-	fput(file);
-
-	return ERR_PTR(rc);
-
 err_put_unused_fd:
 	put_unused_fd(fd);

@@ -458,6 +519,8 @@ err_delete_proxy_dev:
  */
 static void vtpm_proxy_delete_device(struct proxy_dev *proxy_dev)
 {
+	vtpm_proxy_work_stop(proxy_dev);
+
 	/*
 	 * A client may hold the 'ops' lock, so let it know that the server
 	 * side shuts down before we try to grab the 'ops' lock when

@@ -557,11 +620,24 @@ static int __init vtpm_module_init(void)
 		return rc;
 	}
+	workqueue = create_workqueue("tpm-vtpm");
+	if (!workqueue) {
+		pr_err("couldn't create workqueue\n");
+		rc = -ENOMEM;
+		goto err_vtpmx_cleanup;
+	}
+
 	return 0;
+
+err_vtpmx_cleanup:
+	vtpmx_cleanup();
+
+	return rc;
 }
 static void __exit vtpm_module_exit(void)
 {
+	destroy_workqueue(workqueue);
 	vtpmx_cleanup();
 }

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

------------------------------------------------------------------------------