2013-05-28 15:57:00

by Daniel De Graaf

[permalink] [raw]
Subject: [PATCH v3] drivers/tpm: add xen tpmfront interface

This is a complete rewrite of the Xen TPM frontend driver, taking
advantage of a simplified frontend/backend interface and adding support
for cancellation and timeouts. The backend for this driver is provided
by a vTPM stub domain using the interface in Xen 4.3.

Signed-off-by: Daniel De Graaf <[email protected]>
Acked-by: Matthew Fioravante <[email protected]>

---

Changes since v1:
* Added some wmb()s for clarity
* Removed a few magic numbers
* Fix a misused length in memcpy
* Style/comment fixes

Documentation/xen-tpmfront.txt | 116 ++++++++++
drivers/char/tpm/Kconfig | 11 +
drivers/char/tpm/Makefile | 1 +
drivers/char/tpm/xen-tpmfront.c | 470 +++++++++++++++++++++++++++++++++++++++
include/xen/interface/io/tpmif.h | 50 +++++
5 files changed, 648 insertions(+)
create mode 100644 Documentation/xen-tpmfront.txt
create mode 100644 drivers/char/tpm/xen-tpmfront.c
create mode 100644 include/xen/interface/io/tpmif.h

diff --git a/Documentation/xen-tpmfront.txt b/Documentation/xen-tpmfront.txt
new file mode 100644
index 0000000..8a61d6f
--- /dev/null
+++ b/Documentation/xen-tpmfront.txt
@@ -0,0 +1,116 @@
+Copyright (c) 2010-2012 United States Government, as represented by
+the Secretary of Defense. All rights reserved.
+November 12 2012
+Authors: Matthew Fioravante (JHUAPL), Daniel De Graaf (NSA)
+
+This document describes the virtual Trusted Platform Module (vTPM) subsystem for
+Xen. The reader is assumed to have familiarity with building and installing Xen,
+Linux, and a basic understanding of the TPM and vTPM concepts.
+
+------------------------------
+INTRODUCTION
+------------------------------
+
+The goal of this work is to provide a TPM functionality to a virtual guest
+operating system (in Xen terms, a DomU). This allows programs to interact with
+a TPM in a virtual system the same way they interact with a TPM on the physical
+system. Each guest gets its own unique, emulated, software TPM. However, each
+of the vTPM's secrets (Keys, NVRAM, etc) are managed by a vTPM Manager domain,
+which seals the secrets to the Physical TPM. If the process of creating each of
+these domains (manager, vTPM, and guest) is trusted, the vTPM subsystem extends
+the chain of trust rooted in the hardware TPM to virtual machines in Xen. Each
+major component of vTPM is implemented as a separate domain, providing secure
+separation guaranteed by the hypervisor. The vTPM domains are implemented in
+mini-os to reduce memory and processor overhead.
+
+This mini-os vTPM subsystem was built on top of the previous vTPM work done by
+IBM and Intel corporation.
+
+------------------------------
+DESIGN OVERVIEW
+------------------------------
+
+The architecture of vTPM is described below:
+
++------------------+
+| Linux DomU | ...
+| | ^ |
+| v | |
+| xen-tpmfront |
++------------------+
+ | ^
+ v |
++------------------+
+| mini-os/tpmback |
+| | ^ |
+| v | |
+| vtpm-stubdom | ...
+| | ^ |
+| v | |
+| mini-os/tpmfront |
++------------------+
+ | ^
+ v |
++------------------+
+| mini-os/tpmback |
+| | ^ |
+| v | |
+| vtpmmgr-stubdom |
+| | ^ |
+| v | |
+| mini-os/tpm_tis |
++------------------+
+ | ^
+ v |
++------------------+
+| Hardware TPM |
++------------------+
+
+ * Linux DomU: The Linux based guest that wants to use a vTPM. There many be
+ more than one of these.
+
+ * xen-tpmfront.ko: Linux kernel virtual TPM frontend driver. This driver
+ provides vTPM access to a Linux-based DomU.
+
+ * mini-os/tpmback: Mini-os TPM backend driver. The Linux frontend driver
+ connects to this backend driver to facilitate communications
+ between the Linux DomU and its vTPM. This driver is also
+ used by vtpmmgr-stubdom to communicate with vtpm-stubdom.
+
+ * vtpm-stubdom: A mini-os stub domain that implements a vTPM. There is a
+ one to one mapping between running vtpm-stubdom instances and
+ logical vtpms on the system. The vTPM Platform Configuration
+ Registers (PCRs) are normally all initialized to zero.
+
+ * mini-os/tpmfront: Mini-os TPM frontend driver. The vTPM mini-os domain
+ vtpm-stubdom uses this driver to communicate with
+ vtpmmgr-stubdom. This driver is also used in mini-os
+ domains such as pv-grub that talk to the vTPM domain.
+
+ * vtpmmgr-stubdom: A mini-os domain that implements the vTPM manager. There is
+ only one vTPM manager and it should be running during the
+ entire lifetime of the machine. This domain regulates
+ access to the physical TPM on the system and secures the
+ persistent state of each vTPM.
+
+ * mini-os/tpm_tis: Mini-os TPM version 1.2 TPM Interface Specification (TIS)
+ driver. This driver used by vtpmmgr-stubdom to talk directly to
+ the hardware TPM. Communication is facilitated by mapping
+ hardware memory pages into vtpmmgr-stubdom.
+
+ * Hardware TPM: The physical TPM that is soldered onto the motherboard.
+
+------------------------------
+INTEGRATION WITH XEN
+------------------------------
+
+Support for the vTPM driver was added in Xen using the libxl toolstack in Xen
+4.3. See the Xen documentation (docs/misc/vtpm.txt) for details on setting up
+the vTPM and vTPM Manager stub domains. Once the stub domains are running, a
+vTPM device is set up in the same manner as a disk or network device in the
+domain's configuration file.
+
+In order to use features such as IMA that require a TPM to be loaded prior to
+the initrd, the xen-tpmfront driver must be compiled in to the kernel. If not
+using such features, the driver can be compiled as a module and will be loaded
+as usual.
diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
index dbfd564..205ed35 100644
--- a/drivers/char/tpm/Kconfig
+++ b/drivers/char/tpm/Kconfig
@@ -91,4 +91,15 @@ config TCG_ST33_I2C
To compile this driver as a module, choose M here; the module will be
called tpm_stm_st33_i2c.

+config TCG_XEN
+ tristate "XEN TPM Interface"
+ depends on TCG_TPM && XEN
+ ---help---
+ If you want to make TPM support available to a Xen user domain,
+ say Yes and it will be accessible from within Linux. See
+ the manpages for xl, xl.conf, and docs/misc/vtpm.txt in
+ the Xen source repository for more details.
+ To compile this driver as a module, choose M here; the module
+ will be called xen-tpmfront.
+
endif # TCG_TPM
diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
index a3736c9..eb41ff9 100644
--- a/drivers/char/tpm/Makefile
+++ b/drivers/char/tpm/Makefile
@@ -18,3 +18,4 @@ obj-$(CONFIG_TCG_ATMEL) += tpm_atmel.o
obj-$(CONFIG_TCG_INFINEON) += tpm_infineon.o
obj-$(CONFIG_TCG_IBMVTPM) += tpm_ibmvtpm.o
obj-$(CONFIG_TCG_ST33_I2C) += tpm_i2c_stm_st33.o
+obj-$(CONFIG_TCG_XEN) += xen-tpmfront.o
diff --git a/drivers/char/tpm/xen-tpmfront.c b/drivers/char/tpm/xen-tpmfront.c
new file mode 100644
index 0000000..4c0fb12
--- /dev/null
+++ b/drivers/char/tpm/xen-tpmfront.c
@@ -0,0 +1,470 @@
+/*
+ * Implementation of the Xen vTPM device frontend
+ *
+ * Author: Daniel De Graaf <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2,
+ * as published by the Free Software Foundation.
+ */
+#include <linux/errno.h>
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <xen/events.h>
+#include <xen/interface/io/tpmif.h>
+#include <xen/grant_table.h>
+#include <xen/xenbus.h>
+#include <xen/page.h>
+#include "tpm.h"
+
+struct tpm_private {
+ struct tpm_chip *chip;
+ struct xenbus_device *dev;
+
+ struct vtpm_shared_page *shr;
+
+ unsigned int evtchn;
+ int ring_ref;
+ domid_t backend_id;
+};
+
+enum status_bits {
+ VTPM_STATUS_RUNNING = 0x1,
+ VTPM_STATUS_IDLE = 0x2,
+ VTPM_STATUS_RESULT = 0x4,
+ VTPM_STATUS_CANCELED = 0x8,
+};
+
+static u8 vtpm_status(struct tpm_chip *chip)
+{
+ struct tpm_private *priv = TPM_VPRIV(chip);
+ switch (priv->shr->state) {
+ case VTPM_STATE_IDLE:
+ return VTPM_STATUS_IDLE | VTPM_STATUS_CANCELED;
+ case VTPM_STATE_FINISH:
+ return VTPM_STATUS_IDLE | VTPM_STATUS_RESULT;
+ case VTPM_STATE_SUBMIT:
+ case VTPM_STATE_CANCEL: /* cancel requested, not yet canceled */
+ return VTPM_STATUS_RUNNING;
+ default:
+ return 0;
+ }
+}
+
+static bool vtpm_req_canceled(struct tpm_chip *chip, u8 status)
+{
+ return status & VTPM_STATUS_CANCELED;
+}
+
+static void vtpm_cancel(struct tpm_chip *chip)
+{
+ struct tpm_private *priv = TPM_VPRIV(chip);
+ priv->shr->state = VTPM_STATE_CANCEL;
+ wmb();
+ notify_remote_via_evtchn(priv->evtchn);
+}
+
+static unsigned int shr_data_offset(struct vtpm_shared_page *shr)
+{
+ return sizeof(*shr) + sizeof(u32) * shr->nr_extra_pages;
+}
+
+static int vtpm_send(struct tpm_chip *chip, u8 *buf, size_t count)
+{
+ struct tpm_private *priv = TPM_VPRIV(chip);
+ struct vtpm_shared_page *shr = priv->shr;
+ unsigned int offset = shr_data_offset(shr);
+
+ u32 ordinal;
+ unsigned long duration;
+
+ if (offset > PAGE_SIZE)
+ return -EIO;
+
+ if (offset + count > PAGE_SIZE)
+ return -EIO;
+
+ /* Wait for completion of any existing command or cancellation */
+ if (wait_for_tpm_stat(chip, VTPM_STATUS_IDLE, chip->vendor.timeout_c,
+ &chip->vendor.read_queue, true) < 0) {
+ vtpm_cancel(chip);
+ return -ETIME;
+ }
+
+ memcpy(offset + (u8 *)shr, buf, count);
+ shr->length = count;
+ barrier();
+ shr->state = VTPM_STATE_SUBMIT;
+ wmb();
+ notify_remote_via_evtchn(priv->evtchn);
+
+ ordinal = be32_to_cpu(((struct tpm_input_header*)buf)->ordinal);
+ duration = tpm_calc_ordinal_duration(chip, ordinal);
+
+ if (wait_for_tpm_stat(chip, VTPM_STATUS_IDLE, duration,
+ &chip->vendor.read_queue, true) < 0) {
+ /* got a signal or timeout, try to cancel */
+ vtpm_cancel(chip);
+ return -ETIME;
+ }
+
+ return count;
+}
+
+static int vtpm_recv(struct tpm_chip *chip, u8 *buf, size_t count)
+{
+ struct tpm_private *priv = TPM_VPRIV(chip);
+ struct vtpm_shared_page *shr = priv->shr;
+ unsigned int offset = shr_data_offset(shr);
+ size_t length = shr->length;
+
+ if (shr->state == VTPM_STATE_IDLE)
+ return -ECANCELED;
+
+ /* In theory the wait at the end of _send makes this one unnecessary */
+ if (wait_for_tpm_stat(chip, VTPM_STATUS_RESULT, chip->vendor.timeout_c,
+ &chip->vendor.read_queue, true) < 0) {
+ vtpm_cancel(chip);
+ return -ETIME;
+ }
+
+ if (offset > PAGE_SIZE)
+ return -EIO;
+
+ if (offset + length > PAGE_SIZE)
+ length = PAGE_SIZE - offset;
+
+ if (length > count)
+ length = count;
+
+ memcpy(buf, offset + (u8 *)shr, length);
+
+ return length;
+}
+
+ssize_t tpm_show_locality(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ struct tpm_chip *chip = dev_get_drvdata(dev);
+ struct tpm_private *priv = TPM_VPRIV(chip);
+ u8 locality = priv->shr->locality;
+
+ return sprintf(buf, "%d\n", locality);
+}
+
+ssize_t tpm_store_locality(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t len)
+{
+ struct tpm_chip *chip = dev_get_drvdata(dev);
+ struct tpm_private *priv = TPM_VPRIV(chip);
+ u8 val;
+
+ int rv = kstrtou8(buf, 0, &val);
+ if (rv)
+ return rv;
+
+ priv->shr->locality = val;
+
+ return len;
+}
+
+static const struct file_operations vtpm_ops = {
+ .owner = THIS_MODULE,
+ .llseek = no_llseek,
+ .open = tpm_open,
+ .read = tpm_read,
+ .write = tpm_write,
+ .release = tpm_release,
+};
+
+static DEVICE_ATTR(pubek, S_IRUGO, tpm_show_pubek, NULL);
+static DEVICE_ATTR(pcrs, S_IRUGO, tpm_show_pcrs, NULL);
+static DEVICE_ATTR(enabled, S_IRUGO, tpm_show_enabled, NULL);
+static DEVICE_ATTR(active, S_IRUGO, tpm_show_active, NULL);
+static DEVICE_ATTR(owned, S_IRUGO, tpm_show_owned, NULL);
+static DEVICE_ATTR(temp_deactivated, S_IRUGO, tpm_show_temp_deactivated,
+ NULL);
+static DEVICE_ATTR(caps, S_IRUGO, tpm_show_caps, NULL);
+static DEVICE_ATTR(cancel, S_IWUSR | S_IWGRP, NULL, tpm_store_cancel);
+static DEVICE_ATTR(durations, S_IRUGO, tpm_show_durations, NULL);
+static DEVICE_ATTR(timeouts, S_IRUGO, tpm_show_timeouts, NULL);
+static DEVICE_ATTR(locality, S_IRUGO | S_IWUSR, tpm_show_locality,
+ tpm_store_locality);
+
+static struct attribute *vtpm_attrs[] = {
+ &dev_attr_pubek.attr,
+ &dev_attr_pcrs.attr,
+ &dev_attr_enabled.attr,
+ &dev_attr_active.attr,
+ &dev_attr_owned.attr,
+ &dev_attr_temp_deactivated.attr,
+ &dev_attr_caps.attr,
+ &dev_attr_cancel.attr,
+ &dev_attr_durations.attr,
+ &dev_attr_timeouts.attr,
+ &dev_attr_locality.attr,
+ NULL,
+};
+
+static struct attribute_group vtpm_attr_grp = {
+ .attrs = vtpm_attrs,
+};
+
+#define TPM_LONG_TIMEOUT (10 * 60 * HZ)
+
+static const struct tpm_vendor_specific tpm_vtpm = {
+ .status = vtpm_status,
+ .recv = vtpm_recv,
+ .send = vtpm_send,
+ .cancel = vtpm_cancel,
+ .req_complete_mask = VTPM_STATUS_IDLE | VTPM_STATUS_RESULT,
+ .req_complete_val = VTPM_STATUS_IDLE | VTPM_STATUS_RESULT,
+ .req_canceled = vtpm_req_canceled,
+ .attr_group = &vtpm_attr_grp,
+ .miscdev = {
+ .fops = &vtpm_ops,
+ },
+ .duration = {
+ TPM_LONG_TIMEOUT,
+ TPM_LONG_TIMEOUT,
+ TPM_LONG_TIMEOUT,
+ },
+};
+
+static irqreturn_t tpmif_interrupt(int dummy, void *dev_id)
+{
+ struct tpm_private *priv = dev_id;
+
+ switch (priv->shr->state) {
+ case VTPM_STATE_IDLE:
+ case VTPM_STATE_FINISH:
+ wake_up_interruptible(&priv->chip->vendor.read_queue);
+ break;
+ case VTPM_STATE_SUBMIT:
+ case VTPM_STATE_CANCEL:
+ default:
+ break;
+ }
+ return IRQ_HANDLED;
+}
+
+static int setup_chip(struct device *dev, struct tpm_private *priv)
+{
+ struct tpm_chip *chip;
+
+ chip = tpm_register_hardware(dev, &tpm_vtpm);
+ if (!chip)
+ return -ENODEV;
+
+ init_waitqueue_head(&chip->vendor.read_queue);
+
+ priv->chip = chip;
+ TPM_VPRIV(chip) = priv;
+
+ return 0;
+}
+
+/* caller must clean up in case of errors */
+static int setup_ring(struct xenbus_device *dev, struct tpm_private *priv)
+{
+ struct xenbus_transaction xbt;
+ const char *message = NULL;
+ int rv;
+
+ priv->shr = (void *)__get_free_page(GFP_KERNEL|__GFP_ZERO);
+ if (!priv->shr) {
+ xenbus_dev_fatal(dev, -ENOMEM, "allocating shared ring");
+ return -ENOMEM;
+ }
+
+ rv = xenbus_grant_ring(dev, virt_to_mfn(priv->shr));
+ if (rv < 0)
+ return rv;
+
+ priv->ring_ref = rv;
+
+ rv = xenbus_alloc_evtchn(dev, &priv->evtchn);
+ if (rv)
+ return rv;
+
+ rv = bind_evtchn_to_irqhandler(priv->evtchn, tpmif_interrupt, 0,
+ "tpmif", priv);
+ if (rv <= 0) {
+ xenbus_dev_fatal(dev, rv, "allocating TPM irq");
+ return rv;
+ }
+ priv->chip->vendor.irq = rv;
+
+ again:
+ rv = xenbus_transaction_start(&xbt);
+ if (rv) {
+ xenbus_dev_fatal(dev, rv, "starting transaction");
+ return rv;
+ }
+
+ rv = xenbus_printf(xbt, dev->nodename,
+ "ring-ref", "%u", priv->ring_ref);
+ if (rv) {
+ message = "writing ring-ref";
+ goto abort_transaction;
+ }
+
+ rv = xenbus_printf(xbt, dev->nodename, "event-channel", "%u",
+ priv->evtchn);
+ if (rv) {
+ message = "writing event-channel";
+ goto abort_transaction;
+ }
+
+ rv = xenbus_printf(xbt, dev->nodename, "feature-protocol-v2", "1");
+ if (rv) {
+ message = "writing feature-protocol-v2";
+ goto abort_transaction;
+ }
+
+ rv = xenbus_transaction_end(xbt, 0);
+ if (rv == -EAGAIN)
+ goto again;
+ if (rv) {
+ xenbus_dev_fatal(dev, rv, "completing transaction");
+ return rv;
+ }
+
+ xenbus_switch_state(dev, XenbusStateInitialised);
+
+ return 0;
+
+ abort_transaction:
+ xenbus_transaction_end(xbt, 1);
+ if (message)
+ xenbus_dev_error(dev, rv, "%s", message);
+
+ return rv;
+}
+
+static void ring_free(struct tpm_private *priv)
+{
+ if (!priv)
+ return;
+
+ if (priv->ring_ref)
+ gnttab_end_foreign_access(priv->ring_ref, 0,
+ (unsigned long)priv->shr);
+ else
+ free_page((unsigned long)priv->shr);
+
+ if (priv->chip && priv->chip->vendor.irq)
+ unbind_from_irqhandler(priv->chip->vendor.irq, priv);
+
+ kfree(priv);
+}
+
+static int tpmfront_probe(struct xenbus_device *dev,
+ const struct xenbus_device_id *id)
+{
+ struct tpm_private *priv;
+ int rv;
+
+ priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+ if (!priv) {
+ xenbus_dev_fatal(dev, -ENOMEM, "allocating priv structure");
+ return -ENOMEM;
+ }
+
+ rv = setup_chip(&dev->dev, priv);
+ if (rv) {
+ kfree(priv);
+ return rv;
+ }
+
+ rv = setup_ring(dev, priv);
+ if (rv) {
+ tpm_remove_hardware(&dev->dev);
+ ring_free(priv);
+ return rv;
+ }
+
+ tpm_get_timeouts(priv->chip);
+
+ dev_set_drvdata(&dev->dev, priv->chip);
+
+ return rv;
+}
+
+static int tpmfront_remove(struct xenbus_device *dev)
+{
+ struct tpm_chip *chip = dev_get_drvdata(&dev->dev);
+ struct tpm_private *priv = TPM_VPRIV(chip);
+ tpm_remove_hardware(&dev->dev);
+ ring_free(priv);
+ TPM_VPRIV(chip) = NULL;
+ return 0;
+}
+
+static int tpmfront_resume(struct xenbus_device *dev)
+{
+ /* A suspend/resume/migrate will interrupt a vTPM anyway */
+ tpmfront_remove(dev);
+ return tpmfront_probe(dev, NULL);
+}
+
+static void backend_changed(struct xenbus_device *dev,
+ enum xenbus_state backend_state)
+{
+ int val;
+
+ switch (backend_state) {
+ case XenbusStateInitialised:
+ case XenbusStateConnected:
+ if (xenbus_scanf(XBT_NIL, dev->otherend,
+ "feature-protocol-v2", "%d", &val) < 0)
+ val = 0;
+ if (!val) {
+ xenbus_dev_fatal(dev, -EINVAL,
+ "vTPM protocol 2 required");
+ return;
+ }
+ xenbus_switch_state(dev, XenbusStateConnected);
+ break;
+
+ case XenbusStateClosing:
+ case XenbusStateClosed:
+ device_unregister(&dev->dev);
+ xenbus_frontend_closed(dev);
+ break;
+ default:
+ break;
+ }
+}
+
+static const struct xenbus_device_id tpmfront_ids[] = {
+ { "vtpm" },
+ { "" }
+};
+MODULE_ALIAS("xen:vtpm");
+
+static DEFINE_XENBUS_DRIVER(tpmfront, ,
+ .probe = tpmfront_probe,
+ .remove = tpmfront_remove,
+ .resume = tpmfront_resume,
+ .otherend_changed = backend_changed,
+ );
+
+static int __init xen_tpmfront_init(void)
+{
+ if (!xen_domain())
+ return -ENODEV;
+
+ return xenbus_register_frontend(&tpmfront_driver);
+}
+module_init(xen_tpmfront_init);
+
+static void __exit xen_tpmfront_exit(void)
+{
+ xenbus_unregister_driver(&tpmfront_driver);
+}
+module_exit(xen_tpmfront_exit);
+
+MODULE_AUTHOR("Daniel De Graaf <[email protected]>");
+MODULE_DESCRIPTION("Xen vTPM Driver");
+MODULE_LICENSE("GPL");
diff --git a/include/xen/interface/io/tpmif.h b/include/xen/interface/io/tpmif.h
new file mode 100644
index 0000000..92522a4
--- /dev/null
+++ b/include/xen/interface/io/tpmif.h
@@ -0,0 +1,50 @@
+/******************************************************************************
+ * tpmif.h
+ *
+ * TPM I/O interface for Xen guest OSes, v2
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to
+ * deal in the Software without restriction, including without limitation the
+ * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
+ * sell copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ *
+ */
+
+#ifndef __XEN_PUBLIC_IO_TPMIF_H__
+#define __XEN_PUBLIC_IO_TPMIF_H__
+
+enum vtpm_shared_page_state {
+ VTPM_STATE_IDLE, /* no contents / vTPM idle / cancel complete */
+ VTPM_STATE_SUBMIT, /* request ready / vTPM working */
+ VTPM_STATE_FINISH, /* response ready / vTPM idle */
+ VTPM_STATE_CANCEL, /* cancel requested / vTPM working */
+};
+/* The backend should only change state to IDLE or FINISH, while the
+ * frontend should only change to SUBMIT or CANCEL. */
+
+
+struct vtpm_shared_page {
+ uint32_t length; /* request/response length in bytes */
+
+ uint8_t state; /* enum vtpm_shared_page_state */
+ uint8_t locality; /* for the current request */
+ uint8_t pad;
+
+ uint8_t nr_extra_pages; /* extra pages for long packets; may be zero */
+ uint32_t extra_pages[0]; /* grant IDs; length in nr_extra_pages */
+};
+
+#endif
--
1.8.1.4


2013-05-28 20:33:49

by Konrad Rzeszutek Wilk

[permalink] [raw]
Subject: Re: [PATCH v3] drivers/tpm: add xen tpmfront interface

On Tue, May 28, 2013 at 11:40:32AM -0400, Daniel De Graaf wrote:
> This is a complete rewrite of the Xen TPM frontend driver, taking
> advantage of a simplified frontend/backend interface and adding support
> for cancellation and timeouts. The backend for this driver is provided
> by a vTPM stub domain using the interface in Xen 4.3.
>
> Signed-off-by: Daniel De Graaf <[email protected]>
> Acked-by: Matthew Fioravante <[email protected]>

.. snip..
> +static void ring_free(struct tpm_private *priv)
> +{
> + if (!priv)
> + return;
> +
> + if (priv->ring_ref)
> + gnttab_end_foreign_access(priv->ring_ref, 0,
> + (unsigned long)priv->shr);
> + else
> + free_page((unsigned long)priv->shr);
> +
> + if (priv->chip && priv->chip->vendor.irq)
> + unbind_from_irqhandler(priv->chip->vendor.irq, priv);

You are missing:

if (priv->evtchn != INVALID_EVTCHN)
xenbus_free_evtchn(priv->xdev, priv->evtchn);

> +
> + kfree(priv);
> +}
> +

2013-05-28 20:46:51

by Daniel De Graaf

[permalink] [raw]
Subject: Re: [PATCH v3] drivers/tpm: add xen tpmfront interface

On 05/28/2013 04:32 PM, Konrad Rzeszutek Wilk wrote:
> On Tue, May 28, 2013 at 11:40:32AM -0400, Daniel De Graaf wrote:
>> This is a complete rewrite of the Xen TPM frontend driver, taking
>> advantage of a simplified frontend/backend interface and adding support
>> for cancellation and timeouts. The backend for this driver is provided
>> by a vTPM stub domain using the interface in Xen 4.3.
>>
>> Signed-off-by: Daniel De Graaf <[email protected]>
>> Acked-by: Matthew Fioravante <[email protected]>
>
> .. snip..
>> +static void ring_free(struct tpm_private *priv)
>> +{
>> + if (!priv)
>> + return;
>> +
>> + if (priv->ring_ref)
>> + gnttab_end_foreign_access(priv->ring_ref, 0,
>> + (unsigned long)priv->shr);
>> + else
>> + free_page((unsigned long)priv->shr);
>> +
>> + if (priv->chip && priv->chip->vendor.irq)
>> + unbind_from_irqhandler(priv->chip->vendor.irq, priv);
>
> You are missing:
>
> if (priv->evtchn != INVALID_EVTCHN)
> xenbus_free_evtchn(priv->xdev, priv->evtchn);

This is already handled by unbind_from_irqhandler when it calls
unbind_from_irq, which gets the event channel from the IRQ. This
should be the same event channel as priv->evtchn, and it already
calls EVTCHNOP_close on this port. Calling xenbus_free_evtchn on
the event channel again will just double-free the port.

This is done the same way as drivers/block/xen-blkfront.c, for
reference.

>> +
>> + kfree(priv);
>> +}
>> +


--
Daniel De Graaf
National Security Agency

2013-05-29 15:19:10

by Konrad Rzeszutek Wilk

[permalink] [raw]
Subject: Re: [PATCH v3] drivers/tpm: add xen tpmfront interface

On Tue, May 28, 2013 at 04:45:54PM -0400, Daniel De Graaf wrote:
> On 05/28/2013 04:32 PM, Konrad Rzeszutek Wilk wrote:
> >On Tue, May 28, 2013 at 11:40:32AM -0400, Daniel De Graaf wrote:
> >>This is a complete rewrite of the Xen TPM frontend driver, taking
> >>advantage of a simplified frontend/backend interface and adding support
> >>for cancellation and timeouts. The backend for this driver is provided
> >>by a vTPM stub domain using the interface in Xen 4.3.
> >>
> >>Signed-off-by: Daniel De Graaf <[email protected]>
> >>Acked-by: Matthew Fioravante <[email protected]>
> >
> >.. snip..
> >>+static void ring_free(struct tpm_private *priv)
> >>+{
> >>+ if (!priv)
> >>+ return;
> >>+
> >>+ if (priv->ring_ref)
> >>+ gnttab_end_foreign_access(priv->ring_ref, 0,
> >>+ (unsigned long)priv->shr);
> >>+ else
> >>+ free_page((unsigned long)priv->shr);
> >>+
> >>+ if (priv->chip && priv->chip->vendor.irq)
> >>+ unbind_from_irqhandler(priv->chip->vendor.irq, priv);
> >
> >You are missing:
> >
> > if (priv->evtchn != INVALID_EVTCHN)
> > xenbus_free_evtchn(priv->xdev, priv->evtchn);
>
> This is already handled by unbind_from_irqhandler when it calls
> unbind_from_irq, which gets the event channel from the IRQ. This
> should be the same event channel as priv->evtchn, and it already
> calls EVTCHNOP_close on this port. Calling xenbus_free_evtchn on
> the event channel again will just double-free the port.
>

Yup, I see what you mean. Thanks for the explanation.

In this case you can stick:

Reviewed-by: Konrad Rzeszutek Wilk <[email protected]>

on the patch.

Thanks!
> This is done the same way as drivers/block/xen-blkfront.c, for
> reference.
>
> >>+
> >>+ kfree(priv);
> >>+}
> >>+
>
>
> --
> Daniel De Graaf
> National Security Agency

2013-06-04 12:51:47

by Konrad Rzeszutek Wilk

[permalink] [raw]
Subject: Re: [PATCH v3] drivers/tpm: add xen tpmfront interface

On Tue, May 28, 2013 at 11:40:32AM -0400, Daniel De Graaf wrote:
> This is a complete rewrite of the Xen TPM frontend driver, taking
> advantage of a simplified frontend/backend interface and adding support
> for cancellation and timeouts. The backend for this driver is provided
> by a vTPM stub domain using the interface in Xen 4.3.
>
> Signed-off-by: Daniel De Graaf <[email protected]>
> Acked-by: Matthew Fioravante <[email protected]>

I am OK with it and you can stick the Reviewed-by: Konrad Rzeszutek Wilk <[email protected]>

Did the maintainers of the tpm subsystem have any questions?
>
> ---
>
> Changes since v1:
> * Added some wmb()s for clarity
> * Removed a few magic numbers
> * Fix a misused length in memcpy
> * Style/comment fixes
>
> Documentation/xen-tpmfront.txt | 116 ++++++++++
> drivers/char/tpm/Kconfig | 11 +
> drivers/char/tpm/Makefile | 1 +
> drivers/char/tpm/xen-tpmfront.c | 470 +++++++++++++++++++++++++++++++++++++++
> include/xen/interface/io/tpmif.h | 50 +++++
> 5 files changed, 648 insertions(+)
> create mode 100644 Documentation/xen-tpmfront.txt
> create mode 100644 drivers/char/tpm/xen-tpmfront.c
> create mode 100644 include/xen/interface/io/tpmif.h
>
> diff --git a/Documentation/xen-tpmfront.txt b/Documentation/xen-tpmfront.txt
> new file mode 100644
> index 0000000..8a61d6f
> --- /dev/null
> +++ b/Documentation/xen-tpmfront.txt
> @@ -0,0 +1,116 @@
> +Copyright (c) 2010-2012 United States Government, as represented by
> +the Secretary of Defense. All rights reserved.
> +November 12 2012
> +Authors: Matthew Fioravante (JHUAPL), Daniel De Graaf (NSA)
> +
> +This document describes the virtual Trusted Platform Module (vTPM) subsystem for
> +Xen. The reader is assumed to have familiarity with building and installing Xen,
> +Linux, and a basic understanding of the TPM and vTPM concepts.
> +
> +------------------------------
> +INTRODUCTION
> +------------------------------
> +
> +The goal of this work is to provide a TPM functionality to a virtual guest
> +operating system (in Xen terms, a DomU). This allows programs to interact with
> +a TPM in a virtual system the same way they interact with a TPM on the physical
> +system. Each guest gets its own unique, emulated, software TPM. However, each
> +of the vTPM's secrets (Keys, NVRAM, etc) are managed by a vTPM Manager domain,
> +which seals the secrets to the Physical TPM. If the process of creating each of
> +these domains (manager, vTPM, and guest) is trusted, the vTPM subsystem extends
> +the chain of trust rooted in the hardware TPM to virtual machines in Xen. Each
> +major component of vTPM is implemented as a separate domain, providing secure
> +separation guaranteed by the hypervisor. The vTPM domains are implemented in
> +mini-os to reduce memory and processor overhead.
> +
> +This mini-os vTPM subsystem was built on top of the previous vTPM work done by
> +IBM and Intel corporation.
> +
> +------------------------------
> +DESIGN OVERVIEW
> +------------------------------
> +
> +The architecture of vTPM is described below:
> +
> ++------------------+
> +| Linux DomU | ...
> +| | ^ |
> +| v | |
> +| xen-tpmfront |
> ++------------------+
> + | ^
> + v |
> ++------------------+
> +| mini-os/tpmback |
> +| | ^ |
> +| v | |
> +| vtpm-stubdom | ...
> +| | ^ |
> +| v | |
> +| mini-os/tpmfront |
> ++------------------+
> + | ^
> + v |
> ++------------------+
> +| mini-os/tpmback |
> +| | ^ |
> +| v | |
> +| vtpmmgr-stubdom |
> +| | ^ |
> +| v | |
> +| mini-os/tpm_tis |
> ++------------------+
> + | ^
> + v |
> ++------------------+
> +| Hardware TPM |
> ++------------------+
> +
> + * Linux DomU: The Linux based guest that wants to use a vTPM. There many be
> + more than one of these.
> +
> + * xen-tpmfront.ko: Linux kernel virtual TPM frontend driver. This driver
> + provides vTPM access to a Linux-based DomU.
> +
> + * mini-os/tpmback: Mini-os TPM backend driver. The Linux frontend driver
> + connects to this backend driver to facilitate communications
> + between the Linux DomU and its vTPM. This driver is also
> + used by vtpmmgr-stubdom to communicate with vtpm-stubdom.
> +
> + * vtpm-stubdom: A mini-os stub domain that implements a vTPM. There is a
> + one to one mapping between running vtpm-stubdom instances and
> + logical vtpms on the system. The vTPM Platform Configuration
> + Registers (PCRs) are normally all initialized to zero.
> +
> + * mini-os/tpmfront: Mini-os TPM frontend driver. The vTPM mini-os domain
> + vtpm-stubdom uses this driver to communicate with
> + vtpmmgr-stubdom. This driver is also used in mini-os
> + domains such as pv-grub that talk to the vTPM domain.
> +
> + * vtpmmgr-stubdom: A mini-os domain that implements the vTPM manager. There is
> + only one vTPM manager and it should be running during the
> + entire lifetime of the machine. This domain regulates
> + access to the physical TPM on the system and secures the
> + persistent state of each vTPM.
> +
> + * mini-os/tpm_tis: Mini-os TPM version 1.2 TPM Interface Specification (TIS)
> + driver. This driver used by vtpmmgr-stubdom to talk directly to
> + the hardware TPM. Communication is facilitated by mapping
> + hardware memory pages into vtpmmgr-stubdom.
> +
> + * Hardware TPM: The physical TPM that is soldered onto the motherboard.
> +
> +------------------------------
> +INTEGRATION WITH XEN
> +------------------------------
> +
> +Support for the vTPM driver was added in Xen using the libxl toolstack in Xen
> +4.3. See the Xen documentation (docs/misc/vtpm.txt) for details on setting up
> +the vTPM and vTPM Manager stub domains. Once the stub domains are running, a
> +vTPM device is set up in the same manner as a disk or network device in the
> +domain's configuration file.
> +
> +In order to use features such as IMA that require a TPM to be loaded prior to
> +the initrd, the xen-tpmfront driver must be compiled in to the kernel. If not
> +using such features, the driver can be compiled as a module and will be loaded
> +as usual.
> diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
> index dbfd564..205ed35 100644
> --- a/drivers/char/tpm/Kconfig
> +++ b/drivers/char/tpm/Kconfig
> @@ -91,4 +91,15 @@ config TCG_ST33_I2C
> To compile this driver as a module, choose M here; the module will be
> called tpm_stm_st33_i2c.
>
> +config TCG_XEN
> + tristate "XEN TPM Interface"
> + depends on TCG_TPM && XEN
> + ---help---
> + If you want to make TPM support available to a Xen user domain,
> + say Yes and it will be accessible from within Linux. See
> + the manpages for xl, xl.conf, and docs/misc/vtpm.txt in
> + the Xen source repository for more details.
> + To compile this driver as a module, choose M here; the module
> + will be called xen-tpmfront.
> +
> endif # TCG_TPM
> diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
> index a3736c9..eb41ff9 100644
> --- a/drivers/char/tpm/Makefile
> +++ b/drivers/char/tpm/Makefile
> @@ -18,3 +18,4 @@ obj-$(CONFIG_TCG_ATMEL) += tpm_atmel.o
> obj-$(CONFIG_TCG_INFINEON) += tpm_infineon.o
> obj-$(CONFIG_TCG_IBMVTPM) += tpm_ibmvtpm.o
> obj-$(CONFIG_TCG_ST33_I2C) += tpm_i2c_stm_st33.o
> +obj-$(CONFIG_TCG_XEN) += xen-tpmfront.o
> diff --git a/drivers/char/tpm/xen-tpmfront.c b/drivers/char/tpm/xen-tpmfront.c
> new file mode 100644
> index 0000000..4c0fb12
> --- /dev/null
> +++ b/drivers/char/tpm/xen-tpmfront.c
> @@ -0,0 +1,470 @@
> +/*
> + * Implementation of the Xen vTPM device frontend
> + *
> + * Author: Daniel De Graaf <[email protected]>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2,
> + * as published by the Free Software Foundation.
> + */
> +#include <linux/errno.h>
> +#include <linux/err.h>
> +#include <linux/interrupt.h>
> +#include <xen/events.h>
> +#include <xen/interface/io/tpmif.h>
> +#include <xen/grant_table.h>
> +#include <xen/xenbus.h>
> +#include <xen/page.h>
> +#include "tpm.h"
> +
> +struct tpm_private {
> + struct tpm_chip *chip;
> + struct xenbus_device *dev;
> +
> + struct vtpm_shared_page *shr;
> +
> + unsigned int evtchn;
> + int ring_ref;
> + domid_t backend_id;
> +};
> +
> +enum status_bits {
> + VTPM_STATUS_RUNNING = 0x1,
> + VTPM_STATUS_IDLE = 0x2,
> + VTPM_STATUS_RESULT = 0x4,
> + VTPM_STATUS_CANCELED = 0x8,
> +};
> +
> +static u8 vtpm_status(struct tpm_chip *chip)
> +{
> + struct tpm_private *priv = TPM_VPRIV(chip);
> + switch (priv->shr->state) {
> + case VTPM_STATE_IDLE:
> + return VTPM_STATUS_IDLE | VTPM_STATUS_CANCELED;
> + case VTPM_STATE_FINISH:
> + return VTPM_STATUS_IDLE | VTPM_STATUS_RESULT;
> + case VTPM_STATE_SUBMIT:
> + case VTPM_STATE_CANCEL: /* cancel requested, not yet canceled */
> + return VTPM_STATUS_RUNNING;
> + default:
> + return 0;
> + }
> +}
> +
> +static bool vtpm_req_canceled(struct tpm_chip *chip, u8 status)
> +{
> + return status & VTPM_STATUS_CANCELED;
> +}
> +
> +static void vtpm_cancel(struct tpm_chip *chip)
> +{
> + struct tpm_private *priv = TPM_VPRIV(chip);
> + priv->shr->state = VTPM_STATE_CANCEL;
> + wmb();
> + notify_remote_via_evtchn(priv->evtchn);
> +}
> +
> +static unsigned int shr_data_offset(struct vtpm_shared_page *shr)
> +{
> + return sizeof(*shr) + sizeof(u32) * shr->nr_extra_pages;
> +}
> +
> +static int vtpm_send(struct tpm_chip *chip, u8 *buf, size_t count)
> +{
> + struct tpm_private *priv = TPM_VPRIV(chip);
> + struct vtpm_shared_page *shr = priv->shr;
> + unsigned int offset = shr_data_offset(shr);
> +
> + u32 ordinal;
> + unsigned long duration;
> +
> + if (offset > PAGE_SIZE)
> + return -EIO;
> +
> + if (offset + count > PAGE_SIZE)
> + return -EIO;
> +
> + /* Wait for completion of any existing command or cancellation */
> + if (wait_for_tpm_stat(chip, VTPM_STATUS_IDLE, chip->vendor.timeout_c,
> + &chip->vendor.read_queue, true) < 0) {
> + vtpm_cancel(chip);
> + return -ETIME;
> + }
> +
> + memcpy(offset + (u8 *)shr, buf, count);
> + shr->length = count;
> + barrier();
> + shr->state = VTPM_STATE_SUBMIT;
> + wmb();
> + notify_remote_via_evtchn(priv->evtchn);
> +
> + ordinal = be32_to_cpu(((struct tpm_input_header*)buf)->ordinal);
> + duration = tpm_calc_ordinal_duration(chip, ordinal);
> +
> + if (wait_for_tpm_stat(chip, VTPM_STATUS_IDLE, duration,
> + &chip->vendor.read_queue, true) < 0) {
> + /* got a signal or timeout, try to cancel */
> + vtpm_cancel(chip);
> + return -ETIME;
> + }
> +
> + return count;
> +}
> +
> +static int vtpm_recv(struct tpm_chip *chip, u8 *buf, size_t count)
> +{
> + struct tpm_private *priv = TPM_VPRIV(chip);
> + struct vtpm_shared_page *shr = priv->shr;
> + unsigned int offset = shr_data_offset(shr);
> + size_t length = shr->length;
> +
> + if (shr->state == VTPM_STATE_IDLE)
> + return -ECANCELED;
> +
> + /* In theory the wait at the end of _send makes this one unnecessary */
> + if (wait_for_tpm_stat(chip, VTPM_STATUS_RESULT, chip->vendor.timeout_c,
> + &chip->vendor.read_queue, true) < 0) {
> + vtpm_cancel(chip);
> + return -ETIME;
> + }
> +
> + if (offset > PAGE_SIZE)
> + return -EIO;
> +
> + if (offset + length > PAGE_SIZE)
> + length = PAGE_SIZE - offset;
> +
> + if (length > count)
> + length = count;
> +
> + memcpy(buf, offset + (u8 *)shr, length);
> +
> + return length;
> +}
> +
> +ssize_t tpm_show_locality(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + struct tpm_chip *chip = dev_get_drvdata(dev);
> + struct tpm_private *priv = TPM_VPRIV(chip);
> + u8 locality = priv->shr->locality;
> +
> + return sprintf(buf, "%d\n", locality);
> +}
> +
> +ssize_t tpm_store_locality(struct device *dev, struct device_attribute *attr,
> + const char *buf, size_t len)
> +{
> + struct tpm_chip *chip = dev_get_drvdata(dev);
> + struct tpm_private *priv = TPM_VPRIV(chip);
> + u8 val;
> +
> + int rv = kstrtou8(buf, 0, &val);
> + if (rv)
> + return rv;
> +
> + priv->shr->locality = val;
> +
> + return len;
> +}
> +
> +static const struct file_operations vtpm_ops = {
> + .owner = THIS_MODULE,
> + .llseek = no_llseek,
> + .open = tpm_open,
> + .read = tpm_read,
> + .write = tpm_write,
> + .release = tpm_release,
> +};
> +
> +static DEVICE_ATTR(pubek, S_IRUGO, tpm_show_pubek, NULL);
> +static DEVICE_ATTR(pcrs, S_IRUGO, tpm_show_pcrs, NULL);
> +static DEVICE_ATTR(enabled, S_IRUGO, tpm_show_enabled, NULL);
> +static DEVICE_ATTR(active, S_IRUGO, tpm_show_active, NULL);
> +static DEVICE_ATTR(owned, S_IRUGO, tpm_show_owned, NULL);
> +static DEVICE_ATTR(temp_deactivated, S_IRUGO, tpm_show_temp_deactivated,
> + NULL);
> +static DEVICE_ATTR(caps, S_IRUGO, tpm_show_caps, NULL);
> +static DEVICE_ATTR(cancel, S_IWUSR | S_IWGRP, NULL, tpm_store_cancel);
> +static DEVICE_ATTR(durations, S_IRUGO, tpm_show_durations, NULL);
> +static DEVICE_ATTR(timeouts, S_IRUGO, tpm_show_timeouts, NULL);
> +static DEVICE_ATTR(locality, S_IRUGO | S_IWUSR, tpm_show_locality,
> + tpm_store_locality);
> +
> +static struct attribute *vtpm_attrs[] = {
> + &dev_attr_pubek.attr,
> + &dev_attr_pcrs.attr,
> + &dev_attr_enabled.attr,
> + &dev_attr_active.attr,
> + &dev_attr_owned.attr,
> + &dev_attr_temp_deactivated.attr,
> + &dev_attr_caps.attr,
> + &dev_attr_cancel.attr,
> + &dev_attr_durations.attr,
> + &dev_attr_timeouts.attr,
> + &dev_attr_locality.attr,
> + NULL,
> +};
> +
> +static struct attribute_group vtpm_attr_grp = {
> + .attrs = vtpm_attrs,
> +};
> +
> +#define TPM_LONG_TIMEOUT (10 * 60 * HZ)
> +
> +static const struct tpm_vendor_specific tpm_vtpm = {
> + .status = vtpm_status,
> + .recv = vtpm_recv,
> + .send = vtpm_send,
> + .cancel = vtpm_cancel,
> + .req_complete_mask = VTPM_STATUS_IDLE | VTPM_STATUS_RESULT,
> + .req_complete_val = VTPM_STATUS_IDLE | VTPM_STATUS_RESULT,
> + .req_canceled = vtpm_req_canceled,
> + .attr_group = &vtpm_attr_grp,
> + .miscdev = {
> + .fops = &vtpm_ops,
> + },
> + .duration = {
> + TPM_LONG_TIMEOUT,
> + TPM_LONG_TIMEOUT,
> + TPM_LONG_TIMEOUT,
> + },
> +};
> +
> +static irqreturn_t tpmif_interrupt(int dummy, void *dev_id)
> +{
> + struct tpm_private *priv = dev_id;
> +
> + switch (priv->shr->state) {
> + case VTPM_STATE_IDLE:
> + case VTPM_STATE_FINISH:
> + wake_up_interruptible(&priv->chip->vendor.read_queue);
> + break;
> + case VTPM_STATE_SUBMIT:
> + case VTPM_STATE_CANCEL:
> + default:
> + break;
> + }
> + return IRQ_HANDLED;
> +}
> +
> +static int setup_chip(struct device *dev, struct tpm_private *priv)
> +{
> + struct tpm_chip *chip;
> +
> + chip = tpm_register_hardware(dev, &tpm_vtpm);
> + if (!chip)
> + return -ENODEV;
> +
> + init_waitqueue_head(&chip->vendor.read_queue);
> +
> + priv->chip = chip;
> + TPM_VPRIV(chip) = priv;
> +
> + return 0;
> +}
> +
> +/* caller must clean up in case of errors */
> +static int setup_ring(struct xenbus_device *dev, struct tpm_private *priv)
> +{
> + struct xenbus_transaction xbt;
> + const char *message = NULL;
> + int rv;
> +
> + priv->shr = (void *)__get_free_page(GFP_KERNEL|__GFP_ZERO);
> + if (!priv->shr) {
> + xenbus_dev_fatal(dev, -ENOMEM, "allocating shared ring");
> + return -ENOMEM;
> + }
> +
> + rv = xenbus_grant_ring(dev, virt_to_mfn(priv->shr));
> + if (rv < 0)
> + return rv;
> +
> + priv->ring_ref = rv;
> +
> + rv = xenbus_alloc_evtchn(dev, &priv->evtchn);
> + if (rv)
> + return rv;
> +
> + rv = bind_evtchn_to_irqhandler(priv->evtchn, tpmif_interrupt, 0,
> + "tpmif", priv);
> + if (rv <= 0) {
> + xenbus_dev_fatal(dev, rv, "allocating TPM irq");
> + return rv;
> + }
> + priv->chip->vendor.irq = rv;
> +
> + again:
> + rv = xenbus_transaction_start(&xbt);
> + if (rv) {
> + xenbus_dev_fatal(dev, rv, "starting transaction");
> + return rv;
> + }
> +
> + rv = xenbus_printf(xbt, dev->nodename,
> + "ring-ref", "%u", priv->ring_ref);
> + if (rv) {
> + message = "writing ring-ref";
> + goto abort_transaction;
> + }
> +
> + rv = xenbus_printf(xbt, dev->nodename, "event-channel", "%u",
> + priv->evtchn);
> + if (rv) {
> + message = "writing event-channel";
> + goto abort_transaction;
> + }
> +
> + rv = xenbus_printf(xbt, dev->nodename, "feature-protocol-v2", "1");
> + if (rv) {
> + message = "writing feature-protocol-v2";
> + goto abort_transaction;
> + }
> +
> + rv = xenbus_transaction_end(xbt, 0);
> + if (rv == -EAGAIN)
> + goto again;
> + if (rv) {
> + xenbus_dev_fatal(dev, rv, "completing transaction");
> + return rv;
> + }
> +
> + xenbus_switch_state(dev, XenbusStateInitialised);
> +
> + return 0;
> +
> + abort_transaction:
> + xenbus_transaction_end(xbt, 1);
> + if (message)
> + xenbus_dev_error(dev, rv, "%s", message);
> +
> + return rv;
> +}
> +
> +static void ring_free(struct tpm_private *priv)
> +{
> + if (!priv)
> + return;
> +
> + if (priv->ring_ref)
> + gnttab_end_foreign_access(priv->ring_ref, 0,
> + (unsigned long)priv->shr);
> + else
> + free_page((unsigned long)priv->shr);
> +
> + if (priv->chip && priv->chip->vendor.irq)
> + unbind_from_irqhandler(priv->chip->vendor.irq, priv);
> +
> + kfree(priv);
> +}
> +
> +static int tpmfront_probe(struct xenbus_device *dev,
> + const struct xenbus_device_id *id)
> +{
> + struct tpm_private *priv;
> + int rv;
> +
> + priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> + if (!priv) {
> + xenbus_dev_fatal(dev, -ENOMEM, "allocating priv structure");
> + return -ENOMEM;
> + }
> +
> + rv = setup_chip(&dev->dev, priv);
> + if (rv) {
> + kfree(priv);
> + return rv;
> + }
> +
> + rv = setup_ring(dev, priv);
> + if (rv) {
> + tpm_remove_hardware(&dev->dev);
> + ring_free(priv);
> + return rv;
> + }
> +
> + tpm_get_timeouts(priv->chip);
> +
> + dev_set_drvdata(&dev->dev, priv->chip);
> +
> + return rv;
> +}
> +
> +static int tpmfront_remove(struct xenbus_device *dev)
> +{
> + struct tpm_chip *chip = dev_get_drvdata(&dev->dev);
> + struct tpm_private *priv = TPM_VPRIV(chip);
> + tpm_remove_hardware(&dev->dev);
> + ring_free(priv);
> + TPM_VPRIV(chip) = NULL;
> + return 0;
> +}
> +
> +static int tpmfront_resume(struct xenbus_device *dev)
> +{
> + /* A suspend/resume/migrate will interrupt a vTPM anyway */
> + tpmfront_remove(dev);
> + return tpmfront_probe(dev, NULL);
> +}
> +
> +static void backend_changed(struct xenbus_device *dev,
> + enum xenbus_state backend_state)
> +{
> + int val;
> +
> + switch (backend_state) {
> + case XenbusStateInitialised:
> + case XenbusStateConnected:
> + if (xenbus_scanf(XBT_NIL, dev->otherend,
> + "feature-protocol-v2", "%d", &val) < 0)
> + val = 0;
> + if (!val) {
> + xenbus_dev_fatal(dev, -EINVAL,
> + "vTPM protocol 2 required");
> + return;
> + }
> + xenbus_switch_state(dev, XenbusStateConnected);
> + break;
> +
> + case XenbusStateClosing:
> + case XenbusStateClosed:
> + device_unregister(&dev->dev);
> + xenbus_frontend_closed(dev);
> + break;
> + default:
> + break;
> + }
> +}
> +
> +static const struct xenbus_device_id tpmfront_ids[] = {
> + { "vtpm" },
> + { "" }
> +};
> +MODULE_ALIAS("xen:vtpm");
> +
> +static DEFINE_XENBUS_DRIVER(tpmfront, ,
> + .probe = tpmfront_probe,
> + .remove = tpmfront_remove,
> + .resume = tpmfront_resume,
> + .otherend_changed = backend_changed,
> + );
> +
> +static int __init xen_tpmfront_init(void)
> +{
> + if (!xen_domain())
> + return -ENODEV;
> +
> + return xenbus_register_frontend(&tpmfront_driver);
> +}
> +module_init(xen_tpmfront_init);
> +
> +static void __exit xen_tpmfront_exit(void)
> +{
> + xenbus_unregister_driver(&tpmfront_driver);
> +}
> +module_exit(xen_tpmfront_exit);
> +
> +MODULE_AUTHOR("Daniel De Graaf <[email protected]>");
> +MODULE_DESCRIPTION("Xen vTPM Driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/xen/interface/io/tpmif.h b/include/xen/interface/io/tpmif.h
> new file mode 100644
> index 0000000..92522a4
> --- /dev/null
> +++ b/include/xen/interface/io/tpmif.h
> @@ -0,0 +1,50 @@
> +/******************************************************************************
> + * tpmif.h
> + *
> + * TPM I/O interface for Xen guest OSes, v2
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to
> + * deal in the Software without restriction, including without limitation the
> + * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
> + * sell copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
> + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> + * DEALINGS IN THE SOFTWARE.
> + *
> + */
> +
> +#ifndef __XEN_PUBLIC_IO_TPMIF_H__
> +#define __XEN_PUBLIC_IO_TPMIF_H__
> +
> +enum vtpm_shared_page_state {
> + VTPM_STATE_IDLE, /* no contents / vTPM idle / cancel complete */
> + VTPM_STATE_SUBMIT, /* request ready / vTPM working */
> + VTPM_STATE_FINISH, /* response ready / vTPM idle */
> + VTPM_STATE_CANCEL, /* cancel requested / vTPM working */
> +};
> +/* The backend should only change state to IDLE or FINISH, while the
> + * frontend should only change to SUBMIT or CANCEL. */
> +
> +
> +struct vtpm_shared_page {
> + uint32_t length; /* request/response length in bytes */
> +
> + uint8_t state; /* enum vtpm_shared_page_state */
> + uint8_t locality; /* for the current request */
> + uint8_t pad;
> +
> + uint8_t nr_extra_pages; /* extra pages for long packets; may be zero */
> + uint32_t extra_pages[0]; /* grant IDs; length in nr_extra_pages */
> +};
> +
> +#endif
> --
> 1.8.1.4
>

2013-06-04 22:08:29

by Peter Huewe

[permalink] [raw]
Subject: Re: [tpmdd-devel] [PATCH v3] drivers/tpm: add xen tpmfront interface

Hi Daniel,

thanks for this v3.
It's really nice to see the progress and I really like that
sparse/smatch/clang/coccicheck do not complain at all - nice job!

Konrad already did an excellent job at reviewing the driver (thanks for that),
and all previously pointed out issues are fixed.

Unfortunately I haven't had the opportunity to test it yet, but
the driver looks clean from the TPM perspective.


However I do have some minor comments from a general perspective - see below.


>From the TPM point of view I'd say it is fine.
(I'm currently _not_ the (official) maintainer of the tpm subsystem but at least
take care of the incoming stuff as an interim)


So if the comments are addressed you can add:
Acked-by: Peter Huewe <[email protected]>
Reviewed-by: Peter Huewe <[email protected]>

@Konrad: I can stage the driver and push it to James or you can take it.
As it lives in drivers/tpm maybe it should go through the tpm (interim ;)
maintainer and james' tree.


So here are my comments:

Am Dienstag, 28. Mai 2013, 17:40:32 schrieb Daniel De Graaf:
> This is a complete rewrite of the Xen TPM frontend driver, taking
> advantage of a simplified frontend/backend interface and adding support
> for cancellation and timeouts. The backend for this driver is provided
> by a vTPM stub domain using the interface in Xen 4.3.
>
> Signed-off-by: Daniel De Graaf <[email protected]>
> Acked-by: Matthew Fioravante <[email protected]>

> diff --git a/Documentation/xen-tpmfront.txt
> b/Documentation/xen-tpmfront.txt new file mode 100644
> index 0000000..8a61d6f
> --- /dev/null
> +++ b/Documentation/xen-tpmfront.txt
> @@ -0,0 +1,116 @@
> +Copyright (c) 2010-2012 United States Government, as represented by
> +the Secretary of Defense. All rights reserved.

I'm not 100% sure if this can stay this way, as it doesn't permit any changes
to the documentation itself.


> + * Linux DomU: The Linux based guest that wants to use a vTPM. There many
> be + more than one of these.
-* Linux DomU: The Linux based guest that wants to use a vTPM. There many
+* Linux DomU: The Linux based guest that wants to use a vTPM. There may
Just a minor typo


> diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
> index dbfd564..205ed35 100644
> --- a/drivers/char/tpm/Kconfig
> +++ b/drivers/char/tpm/Kconfig
> @@ -91,4 +91,15 @@ config TCG_ST33_I2C
> To compile this driver as a module, choose M here; the module will
> be called tpm_stm_st33_i2c.
>
> +config TCG_XEN
Maybe TCG_XEN_TPM would be better, but TCG_XEN is okay for me.


> +static int vtpm_send(struct tpm_chip *chip, u8 *buf, size_t count)
> +{
> + struct tpm_private *priv = TPM_VPRIV(chip);
> + struct vtpm_shared_page *shr = priv->shr;
> + unsigned int offset = shr_data_offset(shr);
> +
> + u32 ordinal;
> + unsigned long duration;
> +
> + if (offset > PAGE_SIZE)
> + return -EIO;
Maybe -EINVAL?
> +
> + if (offset + count > PAGE_SIZE)
> + return -EIO;
Maybe -EINVAL?
> +


> +/*************************************************************************
> ***** + * tpmif.h
> + *
> + * TPM I/O interface for Xen guest OSes, v2
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> copy + * of this software and associated documentation files (the
> "Software"), to + * deal in the Software without restriction, including
> without limitation the + * rights to use, copy, modify, merge, publish,
> distribute, sublicense, and/or + * sell copies of the Software, and to
> permit persons to whom the Software is + * furnished to do so, subject to
> the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included
> in + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
> OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.
> IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY
> CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT,
> TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE
> SOFTWARE OR THE USE OR OTHER + * DEALINGS IN THE SOFTWARE.
> + *
> + */

Also not sure if this license is correct/compliant with the kernel as it
indicates no clear license to me.

Peter

2013-06-05 15:17:49

by Konrad Rzeszutek Wilk

[permalink] [raw]
Subject: Re: [tpmdd-devel] [PATCH v3] drivers/tpm: add xen tpmfront interface

On Tue, Jun 04, 2013 at 03:14:28PM -0700, Peter H?we wrote:
> Hi Daniel,
>
> thanks for this v3.
> It's really nice to see the progress and I really like that
> sparse/smatch/clang/coccicheck do not complain at all - nice job!
>
> Konrad already did an excellent job at reviewing the driver (thanks for that),
> and all previously pointed out issues are fixed.

Thank you.
>
> Unfortunately I haven't had the opportunity to test it yet, but
> the driver looks clean from the TPM perspective.
>
>
> However I do have some minor comments from a general perspective - see below.
>
>
> From the TPM point of view I'd say it is fine.
> (I'm currently _not_ the (official) maintainer of the tpm subsystem but at least
> take care of the incoming stuff as an interim)
>
>
> So if the comments are addressed you can add:
> Acked-by: Peter Huewe <[email protected]>
> Reviewed-by: Peter Huewe <[email protected]>
>
> @Konrad: I can stage the driver and push it to James or you can take it.

This is James L. Morris?
> As it lives in drivers/tpm maybe it should go through the tpm (interim ;)
> maintainer and james' tree.

Entirely up to you. I am happy to stick in my queue for 3.11 and just
tack on the Acked/Reviewed by from you (once Daniel addresses the issues
and reposts the patch).

If it is going through me - naturally that means I will have to test it :-)

>
>
> So here are my comments:
>
> Am Dienstag, 28. Mai 2013, 17:40:32 schrieb Daniel De Graaf:
> > This is a complete rewrite of the Xen TPM frontend driver, taking
> > advantage of a simplified frontend/backend interface and adding support
> > for cancellation and timeouts. The backend for this driver is provided
> > by a vTPM stub domain using the interface in Xen 4.3.
> >
> > Signed-off-by: Daniel De Graaf <[email protected]>
> > Acked-by: Matthew Fioravante <[email protected]>
>
> > diff --git a/Documentation/xen-tpmfront.txt
> > b/Documentation/xen-tpmfront.txt new file mode 100644
> > index 0000000..8a61d6f
> > --- /dev/null
> > +++ b/Documentation/xen-tpmfront.txt
> > @@ -0,0 +1,116 @@
> > +Copyright (c) 2010-2012 United States Government, as represented by
> > +the Secretary of Defense. All rights reserved.
>
> I'm not 100% sure if this can stay this way, as it doesn't permit any changes
> to the documentation itself.
>
>
> > + * Linux DomU: The Linux based guest that wants to use a vTPM. There many
> > be + more than one of these.
> -* Linux DomU: The Linux based guest that wants to use a vTPM. There many
> +* Linux DomU: The Linux based guest that wants to use a vTPM. There may
> Just a minor typo
>
>
> > diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
> > index dbfd564..205ed35 100644
> > --- a/drivers/char/tpm/Kconfig
> > +++ b/drivers/char/tpm/Kconfig
> > @@ -91,4 +91,15 @@ config TCG_ST33_I2C
> > To compile this driver as a module, choose M here; the module will
> > be called tpm_stm_st33_i2c.
> >
> > +config TCG_XEN
> Maybe TCG_XEN_TPM would be better, but TCG_XEN is okay for me.
>
>
> > +static int vtpm_send(struct tpm_chip *chip, u8 *buf, size_t count)
> > +{
> > + struct tpm_private *priv = TPM_VPRIV(chip);
> > + struct vtpm_shared_page *shr = priv->shr;
> > + unsigned int offset = shr_data_offset(shr);
> > +
> > + u32 ordinal;
> > + unsigned long duration;
> > +
> > + if (offset > PAGE_SIZE)
> > + return -EIO;
> Maybe -EINVAL?
> > +
> > + if (offset + count > PAGE_SIZE)
> > + return -EIO;
> Maybe -EINVAL?
> > +
>
>
> > +/*************************************************************************
> > ***** + * tpmif.h
> > + *
> > + * TPM I/O interface for Xen guest OSes, v2
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining a
> > copy + * of this software and associated documentation files (the
> > "Software"), to + * deal in the Software without restriction, including
> > without limitation the + * rights to use, copy, modify, merge, publish,
> > distribute, sublicense, and/or + * sell copies of the Software, and to
> > permit persons to whom the Software is + * furnished to do so, subject to
> > the following conditions:
> > + *
> > + * The above copyright notice and this permission notice shall be included
> > in + * all copies or substantial portions of the Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
> > OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> > MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.
> > IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY
> > CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT,
> > TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE
> > SOFTWARE OR THE USE OR OTHER + * DEALINGS IN THE SOFTWARE.
> > + *
> > + */
>
> Also not sure if this license is correct/compliant with the kernel as it
> indicates no clear license to me.
>
> Peter

2013-06-21 18:03:34

by Konrad Rzeszutek Wilk

[permalink] [raw]
Subject: Re: [tpmdd-devel] [PATCH v3] drivers/tpm: add xen tpmfront interface

On Tue, Jun 04, 2013 at 03:14:28PM -0700, Peter H?we wrote:
> Hi Daniel,
>
> thanks for this v3.
> It's really nice to see the progress and I really like that
> sparse/smatch/clang/coccicheck do not complain at all - nice job!
>
> Konrad already did an excellent job at reviewing the driver (thanks for that),
> and all previously pointed out issues are fixed.
>
> Unfortunately I haven't had the opportunity to test it yet, but
> the driver looks clean from the TPM perspective.
>
>
> However I do have some minor comments from a general perspective - see below.
>
>
> From the TPM point of view I'd say it is fine.
> (I'm currently _not_ the (official) maintainer of the tpm subsystem but at least
> take care of the incoming stuff as an interim)
>
>
> So if the comments are addressed you can add:
> Acked-by: Peter Huewe <[email protected]>
> Reviewed-by: Peter Huewe <[email protected]>
>
> @Konrad: I can stage the driver and push it to James or you can take it.
> As it lives in drivers/tpm maybe it should go through the tpm (interim ;)
> maintainer and james' tree.

Daniel??

ping?
>
>
> So here are my comments:
>
> Am Dienstag, 28. Mai 2013, 17:40:32 schrieb Daniel De Graaf:
> > This is a complete rewrite of the Xen TPM frontend driver, taking
> > advantage of a simplified frontend/backend interface and adding support
> > for cancellation and timeouts. The backend for this driver is provided
> > by a vTPM stub domain using the interface in Xen 4.3.
> >
> > Signed-off-by: Daniel De Graaf <[email protected]>
> > Acked-by: Matthew Fioravante <[email protected]>
>
> > diff --git a/Documentation/xen-tpmfront.txt
> > b/Documentation/xen-tpmfront.txt new file mode 100644
> > index 0000000..8a61d6f
> > --- /dev/null
> > +++ b/Documentation/xen-tpmfront.txt
> > @@ -0,0 +1,116 @@
> > +Copyright (c) 2010-2012 United States Government, as represented by
> > +the Secretary of Defense. All rights reserved.
>
> I'm not 100% sure if this can stay this way, as it doesn't permit any changes
> to the documentation itself.
>
>
> > + * Linux DomU: The Linux based guest that wants to use a vTPM. There many
> > be + more than one of these.
> -* Linux DomU: The Linux based guest that wants to use a vTPM. There many
> +* Linux DomU: The Linux based guest that wants to use a vTPM. There may
> Just a minor typo
>
>
> > diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
> > index dbfd564..205ed35 100644
> > --- a/drivers/char/tpm/Kconfig
> > +++ b/drivers/char/tpm/Kconfig
> > @@ -91,4 +91,15 @@ config TCG_ST33_I2C
> > To compile this driver as a module, choose M here; the module will
> > be called tpm_stm_st33_i2c.
> >
> > +config TCG_XEN
> Maybe TCG_XEN_TPM would be better, but TCG_XEN is okay for me.
>
>
> > +static int vtpm_send(struct tpm_chip *chip, u8 *buf, size_t count)
> > +{
> > + struct tpm_private *priv = TPM_VPRIV(chip);
> > + struct vtpm_shared_page *shr = priv->shr;
> > + unsigned int offset = shr_data_offset(shr);
> > +
> > + u32 ordinal;
> > + unsigned long duration;
> > +
> > + if (offset > PAGE_SIZE)
> > + return -EIO;
> Maybe -EINVAL?
> > +
> > + if (offset + count > PAGE_SIZE)
> > + return -EIO;
> Maybe -EINVAL?
> > +
>
>
> > +/*************************************************************************
> > ***** + * tpmif.h
> > + *
> > + * TPM I/O interface for Xen guest OSes, v2
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining a
> > copy + * of this software and associated documentation files (the
> > "Software"), to + * deal in the Software without restriction, including
> > without limitation the + * rights to use, copy, modify, merge, publish,
> > distribute, sublicense, and/or + * sell copies of the Software, and to
> > permit persons to whom the Software is + * furnished to do so, subject to
> > the following conditions:
> > + *
> > + * The above copyright notice and this permission notice shall be included
> > in + * all copies or substantial portions of the Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
> > OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> > MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.
> > IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY
> > CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT,
> > TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE
> > SOFTWARE OR THE USE OR OTHER + * DEALINGS IN THE SOFTWARE.
> > + *
> > + */
>
> Also not sure if this license is correct/compliant with the kernel as it
> indicates no clear license to me.
>
> Peter

2013-07-01 21:32:54

by Daniel De Graaf

[permalink] [raw]
Subject: Re: [tpmdd-devel] [PATCH v3] drivers/tpm: add xen tpmfront interface

On 06/04/2013 06:14 PM, Peter H?we wrote:
> Hi Daniel,
>
> thanks for this v3.
> It's really nice to see the progress and I really like that
> sparse/smatch/clang/coccicheck do not complain at all - nice job!
>
> Konrad already did an excellent job at reviewing the driver (thanks for that),
> and all previously pointed out issues are fixed.
>
> Unfortunately I haven't had the opportunity to test it yet, but
> the driver looks clean from the TPM perspective.
>
>
> However I do have some minor comments from a general perspective - see below.
>
>
>>From the TPM point of view I'd say it is fine.
> (I'm currently _not_ the (official) maintainer of the tpm subsystem but at least
> take care of the incoming stuff as an interim)
>
>
> So if the comments are addressed you can add:
> Acked-by: Peter Huewe <[email protected]>
> Reviewed-by: Peter Huewe <[email protected]>

I believe I have addressed the comments in v4 (sorry for the delay, I was away
from this email for a few weeks).

>
> @Konrad: I can stage the driver and push it to James or you can take it.
> As it lives in drivers/tpm maybe it should go through the tpm (interim ;)
> maintainer and james' tree.
>
>
> So here are my comments:
>
> Am Dienstag, 28. Mai 2013, 17:40:32 schrieb Daniel De Graaf:
>> This is a complete rewrite of the Xen TPM frontend driver, taking
>> advantage of a simplified frontend/backend interface and adding support
>> for cancellation and timeouts. The backend for this driver is provided
>> by a vTPM stub domain using the interface in Xen 4.3.
>>
>> Signed-off-by: Daniel De Graaf <[email protected]>
>> Acked-by: Matthew Fioravante <[email protected]>
>
>> diff --git a/Documentation/xen-tpmfront.txt
>> b/Documentation/xen-tpmfront.txt new file mode 100644
>> index 0000000..8a61d6f
>> --- /dev/null
>> +++ b/Documentation/xen-tpmfront.txt
>> @@ -0,0 +1,116 @@
>> +Copyright (c) 2010-2012 United States Government, as represented by
>> +the Secretary of Defense. All rights reserved.
>
> I'm not 100% sure if this can stay this way, as it doesn't permit any changes
> to the documentation itself.

This copyright statement has been removed; the portions of the text under copyright
have been released under the GPL, so the kernel's COPYING statement suffices.

>
>> + * Linux DomU: The Linux based guest that wants to use a vTPM. There many
>> be + more than one of these.
> -* Linux DomU: The Linux based guest that wants to use a vTPM. There many
> +* Linux DomU: The Linux based guest that wants to use a vTPM. There may
> Just a minor typo

Fixed, thanks.

>> diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
>> index dbfd564..205ed35 100644
>> --- a/drivers/char/tpm/Kconfig
>> +++ b/drivers/char/tpm/Kconfig
>> @@ -91,4 +91,15 @@ config TCG_ST33_I2C
>> To compile this driver as a module, choose M here; the module will
>> be called tpm_stm_st33_i2c.
>>
>> +config TCG_XEN
> Maybe TCG_XEN_TPM would be better, but TCG_XEN is okay for me.
>

TCG_XEN seems to match the other Kconfig menu items better.

>> +static int vtpm_send(struct tpm_chip *chip, u8 *buf, size_t count)
>> +{
>> + struct tpm_private *priv = TPM_VPRIV(chip);
>> + struct vtpm_shared_page *shr = priv->shr;
>> + unsigned int offset = shr_data_offset(shr);
>> +
>> + u32 ordinal;
>> + unsigned long duration;
>> +
>> + if (offset > PAGE_SIZE)
>> + return -EIO;
> Maybe -EINVAL?
>> +
>> + if (offset + count > PAGE_SIZE)
>> + return -EIO;
> Maybe -EINVAL?

Both changed.

>
>> +/*************************************************************************
>> ***** + * tpmif.h
>> + *
>> + * TPM I/O interface for Xen guest OSes, v2
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a
>> copy + * of this software and associated documentation files (the
>> "Software"), to + * deal in the Software without restriction, including
>> without limitation the + * rights to use, copy, modify, merge, publish,
>> distribute, sublicense, and/or + * sell copies of the Software, and to
>> permit persons to whom the Software is + * furnished to do so, subject to
>> the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be included
>> in + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
>> OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
>> MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.
>> IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY
>> CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT,
>> TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE
>> SOFTWARE OR THE USE OR OTHER + * DEALINGS IN THE SOFTWARE.
>> + *
>> + */
>
> Also not sure if this license is correct/compliant with the kernel as it
> indicates no clear license to me.
>
> Peter

Since the portions of the header covered by copyright (the v1 interface) have been
removed from the Linux copy, the remainder is public domain and I have removed the
notice.


--
Daniel De Graaf
National Security Agency

2013-07-01 22:17:55

by Peter Huewe

[permalink] [raw]
Subject: Re: [tpmdd-devel] [PATCH v3] drivers/tpm: add xen tpmfront interface

Hi Daniel,

thanks for the rework.
I'll try to review it this week, so Konrad might take it through his tree.

Thanks,
Peter