Menu
▾
▴
Re: [tpmdd-devel] [PATCH v2] drivers/tpm: add xen tpmfront interface
|
From: Daniel De G. <dg...@ty...> - 2013-05-28 15:57:01
|
On 05/22/2013 03:15 PM, Konrad Rzeszutek Wilk wrote:
> On Thu, Apr 11, 2013 at 10:56:01AM -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 <dg...@ty...>
>> Acked-by: Matthew Fioravante <mat...@jh...>
>> ---
>>
>> Changes from v1:
>> - Add locality sysfs attribute
>>
>> Documentation/xen-tpmfront.txt | 116 ++++++++++
>> drivers/char/tpm/Kconfig | 11 +
>> drivers/char/tpm/Makefile | 1 +
>> drivers/char/tpm/xen-tpmfront.c | 460 +++++++++++++++++++++++++++++++++++++++
>> include/xen/interface/io/tpmif.h | 50 +++++
>> 5 files changed, 638 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..6801937
>> --- /dev/null
>> +++ b/drivers/char/tpm/xen-tpmfront.c
>> @@ -0,0 +1,460 @@
>> +/*
>> + * Implementation of the Xen vTPM device frontend
>> + *
>> + * Author: Daniel De Graaf <dg...@ty...>
>> + *
>> + * 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;
>
> Don't you need to follow this with a wmb()?
This would be clearer, although I'm pretty sure any implementation of
an event channel send will include a wmb() or mb() on its own.
> Is it OK to just write the state in the shared memory ? What if there
> are other states in it?
Writing a cancellation is always valid: the worst that can happen is an
already-finished command will be canceled and the other side will just
acknowledge it as a spurious cancel request. The TPM specification does
not require that a cancellation undo the effects of a pending command,
so the user of the cancel command must be prepared for this anyway.
> What if it has been cancelled before and has VTPM_STATE_CANCEL? Are
> the multiple event channel notifications OK? Can the backend handle
> spurious frontends?
The backend is set up to handle spurious notifications and cancellation
by the frontend; a transition from an idle state to VTPM_STATE_CANCEL
just causes the backend to transition state to VTPM_STATE_IDLE.
>> + notify_remote_via_evtchn(priv->evtchn);
>> +}
>> +
>> +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 = sizeof(*shr) + 4*shr->nr_extra_pages;
>
> The '4' is a bit odd. Could you use 'sizeof(uint32)' instead?
> And perhaps a comment: /* Look in the vtpm_shared_page for details. */
I have moved the calculation of offset into a helper function.
>> +
>> + u32 ordinal;
>> + unsigned long duration;
>> +
>> + if (count < TPM_HEADER_SIZE)
>> + return -EIO;
>> +
>
> Perhaps add a comment saying, "/* Unimplemented. Is possible
> but we don't do it yet. */
TPM commands smaller than the TPM header size are not valid. However,
I don't see any reason to enforce this here since the backend should
just generate an error response to such a truncated packet, so I'll
remove the check.
>> + 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();
>
> So the barrier is to make sure that 'lenght' is updated before 'state' right?
Yes, both length and the actual packet data.
>> + shr->state = VTPM_STATE_SUBMIT;
>
> You probably want a 'wmb()' here too.
Yes.
>> + notify_remote_via_evtchn(priv->evtchn);
>> +
>> + ordinal = be32_to_cpu(*((__be32 *) (buf + 6)));
>
> Um, + 6? Why? Is there an #define for that magic constant?
The TPM packet header is described in struct tpm_input_header:
struct tpm_input_header {
__be16 tag;
__be32 length;
__be32 ordinal;
} __packed;
The "+ 6" line was copied from tpm.c, and could be replaced with
a longer set of casts or an offsetof - I think I'll use the cast.
> Should this value be read before you do the wait_for_tpm_stat stuff?
This is taken from the packet we are sending, so it doesn't matter
when it is read.
>> + 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 = sizeof(*shr) + 4*shr->nr_extra_pages;
>
> Again, the '4'. sizeof(uint32_t).
Yes.
>> + 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, count);
>
> s/count/length in that memcpy?
Yes.
>> +
>> + 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
>
>
> Missing an ',' at the end.
OK
>> +};
>> +
>> +#define TPM_LONG_TIMEOUT (10 * 60 * HZ)
>
> Why this number and not say 1000*60*HZ ?
If the backend doesn't respond in 10 minutes, waiting 1000 minutes is
unlikely to help. This value is overwritten during the driver's startup
with values from the vTPM (in tpm_get_timeouts).
>> +
>> +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;
>> +}
>> +
>
> Can you add a comment here saying that the caller is responsible for cleaning up
> in case of errors please?
OK
>> +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)
>> +{
>
> A for priv being valid? Say
>
> if (!priv)
> return;
>
>> + if (priv->ring_ref)
>> + gnttab_end_foreign_access(priv->ring_ref, 0,
>> + (unsigned long)priv->shr);
>> +
>> + if (priv->chip && priv->chip->vendor.irq)
>> + unbind_from_irqhandler(priv->chip->vendor.irq, priv);
>> +
>> + free_page((unsigned long)priv->shr);
>
> I think you are missing a call to xenbus_free_evtchn ?
I think this is already done by unbind_from_irqhandler: it calls
unbind_from_irq which calls EVTCHNOP_close if evtchn_from_irq was
valid - which should have been set up by bind_evtchn_to_irqhandler.
>> + 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);
>
>
> Perhaps also:
> priv = NULL; ?
I think you mean 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 <dg...@ty...>");
>> +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
>>
>
> Otherwise it looks OK to me. Should this go through me or Kent?
> And if so, is Kent waiting for my feedback?
>
I wasn't sure if this patch goes in via a Xen tree or TPM driver
maintainers. I'll CC the full list from scripts/get_maintainer.pl
on my v3 submission.
--
Daniel De Graaf
National Security Agency
|